sql: don't bump RC txn read timestamp before commit

[nvanbenschoten]

Fixes #109628.

This commit removes the bumping of the read committed transactions' read timestamp in connExecutor.commitSQLTransactionInternal. Bumping the transaction's external read timestamp is not needed before committing, and it causes the transaction to commit at a higher timestamp than necessary. On highly contended workloads like the one from #109628, this can cause unnecessary contention by inflating the contention footprint of each transaction (i.e. the duration measured in the MVCC time domain that the transaction holds locks).

By not bumping the read timestamp immediately before committed, we improve the performance of contended workloads. For example, on the workload from #109628, we see the following improvement:

Summary

Serializable:            232.6 tps
Read Committed (before): 225.3 tps
Read Committed (after):  236.0 tps

Read Committed improves by 4.7% and is now 1.5% faster than Serializable on the workload.

Raw

### Serializable

transaction type: tpcb-cockroach.sql                  transaction type: tpcb-cockroach.sql                  transaction type: tpcb-cockroach.sql
scaling factor: 10                                    scaling factor: 10                                    scaling factor: 10
query mode: simple                                    query mode: simple                                    query mode: simple
number of clients: 25                                 number of clients: 25                                 number of clients: 25
number of threads: 10                                 number of threads: 10                                 number of threads: 10
duration: 120 s                                       duration: 120 s                                       duration: 120 s
number of transactions actually processed: 27935      number of transactions actually processed: 27584      number of transactions actually processed: 28380
number of failed transactions: 0 (0.000%)             number of failed transactions: 0 (0.000%)             number of failed transactions: 0 (0.000%)
number of serialization failures: 0 (0.000%)          number of serialization failures: 0 (0.000%)          number of serialization failures: 0 (0.000%)
number of deadlock failures: 0 (0.000%)               number of deadlock failures: 0 (0.000%)               number of deadlock failures: 0 (0.000%)
number of transactions retried: 0 (0.000%)            number of transactions retried: 0 (0.000%)            number of transactions retried: 0 (0.000%)
total number of retries: 0                            total number of retries: 0                            total number of retries: 0
latency average = 107.483 ms                          latency average = 108.829 ms                          latency average = 105.776 ms
latency stddev = 264.163 ms                           latency stddev = 263.132 ms                           latency stddev = 244.713 ms
initial connection time = 12.315 ms                   initial connection time = 11.692 ms                   initial connection time = 9.448 ms
tps = 232.157370 (without initial connection time)    tps = 229.458565 (without initial connection time)    tps = 236.039695 (without initial connection time)


### Read Committed (before)

transaction type: tpcb-cockroach.sql                  transaction type: tpcb-cockroach.sql                  transaction type: tpcb-cockroach.sql
scaling factor: 10                                    scaling factor: 10                                    scaling factor: 10
query mode: simple                                    query mode: simple                                    query mode: simple
number of clients: 25                                 number of clients: 25                                 number of clients: 25
number of threads: 10                                 number of threads: 10                                 number of threads: 10
duration: 120 s                                       duration: 120 s                                       duration: 120 s
number of transactions actually processed: 27220      number of transactions actually processed: 27143      number of transactions actually processed: 26966
number of failed transactions: 0 (0.000%)             number of failed transactions: 0 (0.000%)             number of failed transactions: 0 (0.000%)
number of serialization failures: 0 (0.000%)          number of serialization failures: 0 (0.000%)          number of serialization failures: 0 (0.000%)
number of deadlock failures: 0 (0.000%)               number of deadlock failures: 0 (0.000%)               number of deadlock failures: 0 (0.000%)
number of transactions retried: 0 (0.000%)            number of transactions retried: 0 (0.000%)            number of transactions retried: 0 (0.000%)
total number of retries: 0                            total number of retries: 0                            total number of retries: 0
latency average = 110.293 ms                          latency average = 110.646 ms                          latency average = 111.354 ms
latency stddev = 173.640 ms                           latency stddev = 180.792 ms                           latency stddev = 179.226 ms
initial connection time = 8.911 ms                    initial connection time = 9.894 ms                    initial connection time = 10.605 ms
tps = 226.389120 (without initial connection time)    tps = 225.427664 (without initial connection time)    tps = 224.059547 (without initial connection time)


### Read Committed (after)

transaction type: tpcb-cockroach.sql                  transaction type: tpcb-cockroach.sql                  transaction type: tpcb-cockroach.sql
scaling factor: 10                                    scaling factor: 10                                    scaling factor: 10
query mode: simple                                    query mode: simple                                    query mode: simple
number of clients: 25                                 number of clients: 25                                 number of clients: 25
number of threads: 10                                 number of threads: 10                                 number of threads: 10
duration: 120 s                                       duration: 120 s                                       duration: 120 s
number of transactions actually processed: 28526      number of transactions actually processed: 28564      number of transactions actually processed: 28039
number of failed transactions: 0 (0.000%)             number of failed transactions: 0 (0.000%)             number of failed transactions: 0 (0.000%)
number of serialization failures: 0 (0.000%)          number of serialization failures: 0 (0.000%)          number of serialization failures: 0 (0.000%)
number of deadlock failures: 0 (0.000%)               number of deadlock failures: 0 (0.000%)               number of deadlock failures: 0 (0.000%)
number of transactions retried: 0 (0.000%)            number of transactions retried: 0 (0.000%)            number of transactions retried: 0 (0.000%)
total number of retries: 0                            total number of retries: 0                            total number of retries: 0
latency average = 105.221 ms                          latency average = 105.114 ms                          latency average = 107.065 ms
latency stddev = 196.386 ms                           latency stddev = 197.031 ms                           latency stddev = 203.784 ms
initial connection time = 12.549 ms                   initial connection time = 11.212 ms                   initial connection time = 7.715 ms
tps = 237.329979 (without initial connection time)    tps = 237.417802 (without initial connection time)    tps = 233.194327 (without initial connection time)

Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[arulajmani]

This was a cool find!

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rafiss)

[rafiss]

if there's a way to write a test without too much effort that demonstrates avoiding the extra bump, that could be nice to have. I don't think it's strictly necessary

[nvanbenschoten]

if there's a way to write a test without too much effort that demonstrates avoiding the extra bump, that could be nice to have. I don't think it's strictly necessary

Good idea. I added a test for this.

[nvanbenschoten]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)