sql: don't rewind to BEGIN when retrying txn

[rafiss]

fixes #82392

Release note (bug fix): Fixed a bug where CockroachDB would
sometimes automatically retry the BEGIN statement of an explicit
transaction.

[cockroach-teamcity]

This change is 

[ZhouXing19]

Nice catch, thanks!

[rafiss]

cc @ZhouXing19 i added the test and changed the implementation a little. please take a look whenever you have time

[ZhouXing19]

I'm not familiar with TestingKnobs (and they look so cool!) so I'm a bit confused: did we simulate retrying a txn? It seems to me that there's only one conn in the test, and SELECT 1; BEGIN; INSERT INTO foo VALUES(1); COMMIT; only ran once, and it will always pass without retrying.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @ZhouXing19)

---

pkg/sql/conn_executor_test.go line 767 at r1 (raw file):

			_, tableID, err := keys.SystemSQLCodec.DecodeTablePrefix(put.Key)
			if err != nil || tableID != fooTableId {
				err = nil

nit: we can just return nil here, I think

---

pkg/sql/conn_executor_test.go line 772 at r1 (raw file):

			if atomic.AddInt64(&failed, 1) <= numToFail {
				return roachpb.NewErrorWithTxn(
					roachpb.NewTransactionRetryError(roachpb.RETRY_REASON_UNKNOWN, "boom"), ba.Txn,

Maybe a more informative error message?

[ZhouXing19]

pkg/sql/conn_executor_test.go line 758 at r1 (raw file):

	testDB.QueryRow(t, "SELECT 'foo'::regclass::oid").Scan(&fooTableId)

	// Inject an error that will happen during execution.

I'm not sure if I understand the logic here: is it counting queries that are doing "conditional put", i.e. set the same value to a key in fooTable?. And when the count > 2, it raise an error?

[rafiss]

I'm not familiar with TestingKnobs (and they look so cool!) so I'm a bit confused: did we simulate retrying a txn? It seems to me that there's only one conn in the test, and SELECT 1; BEGIN; INSERT INTO foo VALUES(1); COMMIT; only ran once, and it will always pass without retrying.

good questions!

the way testing knobs work is that it lets you "inject" custom functionality into different parts of the system. you can view the usages of the knob to see where the behavior gets injected.

in this case, the behavior i'm injecting is to add a retryable error, which will cause the conn executor to retry the transaction. look for the canAutoRetry and eventRetriableErr usages in the conn executor.

but to make sure it retried, i should add one more assertion that the retry count was actually incremented.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ZhouXing19)

---

pkg/sql/conn_executor_test.go line 758 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

I'm not sure if I understand the logic here: is it counting queries that are doing "conditional put", i.e. set the same value to a key in fooTable?. And when the count > 2, it raise an error?

"conditional put" is the name of the KV request used to implement INSERT statements.

this actually will raise the error when the count is less than two

---

pkg/sql/conn_executor_test.go line 767 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

nit: we can just return nil here, I think

fixed

---

pkg/sql/conn_executor_test.go line 772 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Maybe a more informative error message?

nobody actually will see this error message, but i will make it more readable for people looking at the code

[ZhouXing19]

I see, thanks for the explanation!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ZhouXing19)

[rafiss]

tftr!

bors r=ZhouXing19

[craig]

Build succeeded:

• GitHub CI (Cockroach)