Skip to content

sql: attempt txn auto-commit before flushing txnResults#22721

Merged
nvb merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/fixDupl
Feb 15, 2018
Merged

sql: attempt txn auto-commit before flushing txnResults#22721
nvb merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/fixDupl

Conversation

@nvb
Copy link
Copy Markdown
Contributor

@nvb nvb commented Feb 14, 2018

Fixes #19269.
Unflakes #22714.

From #19269 (comment):

The problem is that we Flush the statement results for an AutoRetry transaction attempt before attempting to perform an auto-commit. The auto-commit can then throw a retryable error, forcing us to retry the entire transaction. At this point, we check ResultsSentToClient to decide whether it's safe to retry automatically on the gateway instead of sending the error up to the client. The issue is that ResultsGroup.Flush was meant to be scoped to an entire transaction and because of this, it resets the hasSentResults flag. This in turn makes ResultsSentToClient return false, which allows us to perform an auto-retry on the gateway after we've sent results to the client, resulting in duplicate results.

Release note: None

@nvb nvb requested review from a team and andreimatei February 14, 2018 23:48
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Feb 14, 2018

This change is Reviewable

@andreimatei
Copy link
Copy Markdown
Contributor

andreimatei commented Feb 15, 2018

:lgtm:

Epic find bro.

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

pkg/sql/executor.go, line 1099 at r1 (raw file):

	// transaction was only supposed to exist for the statement that we just
	// ran. This needs to happen before the txnResults Flush below (#19269).
	if autoCommit {

nit: no reason for the issue reference. Click bait.

pkg/sql/txn_restart_test.go, line 566 at r1 (raw file):

	}
	if rowsInserted != 6 {
		t.Fatalf("Expected 6 rows, got %d", rowsInserted) // fails with 10!

nit: s/rows/result sets

remove the comment

Comments from Reviewable

@nvb nvb mentioned this pull request Feb 15, 2018
Merged
@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Feb 15, 2018

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions.

pkg/sql/executor.go, line 1099 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: no reason for the issue reference. Click bait.

Done.

pkg/sql/txn_restart_test.go, line 566 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: s/rows/result sets

remove the comment

Done.

Comments from Reviewable

@nvb nvb force-pushed the nvanbenschoten/fixDupl branch from e21bc02 to 0addb8a Compare February 15, 2018 17:14
@nvb nvb merged commit cd12adf into cockroachdb:master Feb 15, 2018
@nvb nvb deleted the nvanbenschoten/fixDupl branch February 15, 2018 17:54
@nvb nvb mentioned this pull request Feb 15, 2018
Merged
@nvb nvb mentioned this pull request Mar 1, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andreimatei andreimatei Awaiting requested review from andreimatei

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nvb @cockroach-teamcity @andreimatei