Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

nvanbenschoten
Copy link
Member

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

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

This change is Reviewable

@andreimatei
Copy link
Contributor

: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

@nvanbenschoten
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants