sql: savepoints are broken with slow statements on master

[jordanlewis]

While investigating #93379, @rafiss and I have discovered some very broken behavior with savepoints on master.

The following logic test fails, but it should succeed (seeing nothing in the final output statement):

statement ok
CREATE TABLE a (id INT);
CREATE TABLE b (id INT8)

statement ok
BEGIN

statement ok
SELECT pg_sleep(1.5)

statement ok
SAVEPOINT s

statement ok
SELECT pg_sleep(1.5)

statement ok
INSERT INTO a(id) VALUES (0);
INSERT INTO b(id) VALUES (0)

statement ok
ROLLBACK TO SAVEPOINT s

query I
SELECT * FROM a
----

The pg_sleep statements are important to make this fail, suggesting timing issues.

Jira issue: CRDB-22839

[rafiss]

I ran a git bisect, and found that 7ff05f1 is the first commit that causes Jordan's test to fail:

7ff05f15725617368a175cce187b15c5aba81698 is the first bad commit
commit 7ff05f15725617368a175cce187b15c5aba81698
Date:   Thu Aug 4 18:08:50 2022 -0400

    kvserver: allow certain read-only requests to drop latches before evaluation

[jordanlewis]

1.5 seconds also seems close to the minimum - 1.25 doesn't work on my machine.

[blathers-crl]

Hi @nvanbenschoten, please add branch-* labels to identify which branch(es) this release-blocker affects.

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

[arulajmani]

The issue here seems to be that we're incorrectly determining that the scan request for SELECT * FROM a does not need access to the intent history. The determination happens here:

cockroach/pkg/storage/engine.go

Lines 1777 to 1779 in e7713f1

	if seq <= meta.Txn.Sequence || ts.LessEq(meta.Timestamp.ToTimestamp()) {
	needIntentHistory = true
	}

However, this doesn't take into account ignoredSequenceNumbers for the transaction -- in this case, because the intent we scanned should be ignored because of the savepoint rollback, we do need access to the intent history. We should pass this information to ScanConflictingIntentsForDroppingLatchesEarly and resort to using the intent interleaving iterator if any of the intents were written at a sequence number that should be ignored.

---

The question still remains -- what's the deal with pg_sleep here?

In addition to the sequence numbers, we also look at the timestamp of the read request and compare it with the timestamp at which the intent is written at when deciding whether we need the intent history or not. We only determine that intent history isn't required if the read timestamp is greater than the timestamp at which the intent was written, for which the transaction needs to get pushed. Adding the pg_sleep causes this, by virtue of the closed timestamp interval.

[rafiss]

@arulajmani do you have a timeline for when this issue can be worked on? just wondering since it's still affecting nightly tests here: #95569

[arulajmani]

Thanks for the ping! It's on my plate for this milestone, but realistically, I was hoping to get to it early next week. Sorry for the trouble, I didn't realize this was actively affecting nightly tests.