sql,metrics: do not increment ROLLBACK counter if in CommitWait

[rafiss]

fixes #50780

Release note (bug fix): Previously if RELEASE SAVEPOINT cockroach_restart
was followed by ROLLBACK, the sql.txn.rollback.count
metric would be incremented. This was incorrect, since the txn had already
committed. Now that metric is not incremented in this case.

[cockroach-teamcity]

This change is 

[rafiss]

Are there tests for this anywhere?

[otan]

Probably have to add some in https://github.com/cockroachdb/cockroach/tree/master/pkg/sql/testdata/telemetry

[rafiss]

those tests are for telemetry i believe right? but this change is for metrics for the DB console

[otan]

ah i see

[rafiss]

i added a test that goes through the HTTP _status/vars interface since i couldn't find any other place it would go

RFAL

[rafiss]

@arulajmani I requested your review you since you steered me in the general direction of those tests -- lmk what you think

[arulajmani]

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

---

pkg/server/status_test.go, line 1195 at r1 (raw file):

		}
		if !bytes.Contains(body, []byte("sql_txn_rollback_count 0")) {
			return errors.Errorf("expected `sql_txn_rollback_count 0`, got: %s", body)

Should we check that the sql.txn.commit.count has been incremented to 1 as well?

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @nvanbenschoten)

---

pkg/server/status_test.go, line 1195 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Should we check that the sql.txn.commit.count has been incremented to 1 as well?

yes good call. though actually it is expected to be 0 since it only counts COMMIT statements. in this case, sql_restart_savepoint_release_count is used instead of COMMIT, but i will still add the assertion we expect

[arulajmani]

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

---

pkg/server/status_test.go, line 1195 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

yes good call. though actually it is expected to be 0 since it only counts COMMIT statements. in this case, sql_restart_savepoint_release_count is used instead of COMMIT, but i will still add the assertion we expect

Maybe I'm misunderstanding something, but @nvanbenschoten's comment on the issue seems to imply that sql.txn.commit.count should be incremented.

[rafiss]

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

---

pkg/server/status_test.go, line 1195 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Maybe I'm misunderstanding something, but @nvanbenschoten's comment on the issue seems to imply that sql.txn.commit.count should be incremented.

yikes i missed that! yes i will make the code do that then

[arulajmani]

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

[rafiss]

thanks for reviewing!

bors r=arulajmani

[craig]

Build succeeded:

• GitHub CI (Cockroach)