sql: add support for DECLARE, FETCH, CLOSE CURSOR using internal executors

[jordanlewis]

Touches #41412.

This PR implements the SQL CURSOR methods DECLARE, FETCH and CLOSE using streaming internal executors.

Here's the Postgres docs on DECLARE: https://www.postgresql.org/docs/current/sql-declare.html

Things it implements:

• DECLARE for forward-only cursors
• FETCH forward, relative, and absolute variants for forward-only cursors
• CLOSE a cursor
• pg_catalog.pg_cursors

Things it's missing:

• BINARY CURSOR support, which returns data in the Postgres binary format
• MOVE support, which allows advancing the cursor without returning any rows. This should be quite similar to FETCH.
• WITH HOLD support (allows keeping a cursor open for longer than a transaction by writing its results into a buffer)
• Scrollable cursor (reverse fetch) support, we'd have to implement this also by savings results into a buffer.
• FOR UPDATE support
• Respect for SAVEPOINT. Cursor definitions will not disappear if rolled back to a savepoint before they were created. But we also don't do this right for settings (see sql: SET does not properly rollback in a transaction #69396) so I don't think this is a major problem.

[cockroach-teamcity]

This change is 

[jordanlewis]

Thank you to @otan for inspiring me to take a look at this on flex friday!

[jordanlewis]

Alright, I think this is ready for a look.

[ajwerner]

My first instinct is that this is going to have bad interactions with schema changes in the transaction. That is to say, it won't see them. That's pretty gross. I wonder what happens when you create cursors over various pg_catalog things and then modify them while holding the cursor open. I'm not even sure what the right thing is.

What sort of isolation are cursors supposed to have? I think because you're using the transaction object itself, it's going to be weird. Maybe it should be at some fixed sequence in the transaction?

[jordanlewis]

My first instinct is that this is going to have bad interactions with schema changes in the transaction. That is to say, it won't see them. That's pretty gross. I wonder what happens when you create cursors over various pg_catalog things and then modify them while holding the cursor open. I'm not even sure what the right thing is.

This is a good point. I will test to see how it works in PG.

What sort of isolation are cursors supposed to have? I think because you're using the transaction object itself, it's going to be weird. Maybe it should be at some fixed sequence in the transaction?

Cursors are supposed to see a snapshot at the point of time that they were created in the transaction. That is, they're not supposed to be able to read other writes from the same transaction that happen after they were created.

This behavior is implemented by the current PR, though to be honest, I don't know why it works. I assume when we make an InternalExecutor, we create a sequence point, or something? There's a logic test for it.

[ajwerner]

This behavior is implemented by the current PR, though to be honest, I don't know why it works. I assume when we make an InternalExecutor, we create a sequence point, or something? There's a logic test for it.

It'll work for queries where we construct a leaf transaction if you have flows. The leaf transaction occurs at a snapshot. However, it's really important that when this happens we make sure that we properly allow the flows to finish so that the final leaf state flows back to the root txn before committing. Otherwise you'll subtly permit serializability violations.

cockroach/pkg/sql/distsql/server.go

Lines 289 to 291 in 775c2b0

	// The flow will run in a LeafTxn because we do not want each distributed
	// Txn to heartbeat the transaction.
	return kv.NewLeafTxn(ctx, ds.DB, req.Flow.Gateway, tis), nil

I don't know enough to say how fragile this is.

[jordanlewis]

It'll work for queries where we construct a leaf transaction if you have flows.

Hmm, but the tests in this PR work even without leaf transactions - we don't use those in the local logic test test config. So there must be something else going on too, right?

[ajwerner]

It'll work for queries where we construct a leaf transaction if you have flows.

Hmm, but the tests in this PR work even without leaf transactions - we don't use those in the local logic test test config. So there must be something else going on too, right?

It'd be interesting to look at traces. Are all of the rows being fetched by the row fetcher in the first request? What if you add a lot of rows or something like that?

[jordanlewis]

It doesn't appear to have to do with that. I'll pull KV trace next, it's a bit annoying due to internal executor plumbing.

demo@127.0.0.1:26257/defaultdb> create table a (a primary key, b, c) as select g,g+1,g+2 from generate_series(1,20000) g(g);
CREATE TABLE AS
demo@127.0.0.1:26257/defaultdb> begin;
BEGIN
demo@127.0.0.1:26257/defaultdb  OPEN> declare foo cursor for select * from a;
DECLARE
demo@127.0.0.1:26257/defaultdb  OPEN> insert into a values(10000000,1,1);
INSERT 1
demo@127.0.0.1:26257/defaultdb  OPEN> fetch relative 19999 foo;
    a   |   b   |   c
--------+-------+--------
  19999 | 20000 | 20001
(1 row)
demo@127.0.0.1:26257/defaultdb  OPEN> fetch 1 foo;
    a   |   b   |   c
--------+-------+--------
  20000 | 20001 | 20002
(1 row)
demo@127.0.0.1:26257/defaultdb  OPEN> fetch 1 foo;
FETCH

Time: 0ms total (execution 0ms / network 0ms)

[jordanlewis]

Ah, I forgot that we changed the KV protocol to not stop sending until it hit a bytes limit as well, not just a row limit. I think your hypothesis is correct.

[yuzefovich]

Reviewed 16 of 16 files at r1, 6 of 6 files at r2, 4 of 4 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @jordanlewis, @otan, and @rafiss)

---

-- commits, line 5 at r1:
nit: missing release note.

---

pkg/sql/sql_cursor.go, line 132 at r2 (raw file):

			return true, nil
		case tree.FetchRelative:
			for i := int64(0); i < f.offset; i++ {

Relative fetch can also be backward, should we check that f.offset is not negative?

---

pkg/sql/sql_cursor.go, line 175 at r2 (raw file):

}

func (s *sqlCursor) Next(ctx context.Context) (bool, error) {

nit: this probably could be unexported.

---

pkg/sql/sql_cursor.go, line 55 at r3 (raw file):

	cursor := &sqlCursor{InternalRows: rows}
	if err := p.sqlCursors.addCursor(cursorName, cursor); err != nil {
		// thread at a time. But let's be diligent and clean up if it somehow does

nit: the comment is cut off.

---

pkg/sql/sql_cursor.go, line 182 at r3 (raw file):

	// closeAll closes all cursors in the set.
	closeAll()
	// closeCursors closes the named cursor, returning an error if that cursor

nit: s/closeCursors/closeCursor/.

---

pkg/sql/sql_cursor.go, line 185 at r3 (raw file):

	// didn't exist in the set.
	closeCursor(string) error
	// closeCursors returns the named cursor, returning an error if that cursor

nit: s/closeCursors/getCursor/.

---

pkg/sql/parser/sql.y, line 5092 at r1 (raw file):

// %SeeAlso: CLOSE, FETCH
declare_cursor_stmt:
	DECLARE cursor_name opt_binary opt_sensitivity opt_scroll CURSOR opt_hold FOR select_stmt

PG docs say that

The key words ASENSITIVE, BINARY, INSENSITIVE, and SCROLL can appear in any order.

it looks like we currently don't support that, right? Is the only way to support that is to have all permutations defined explicitly?

---

pkg/sql/parser/sql.y, line 5195 at r1 (raw file):

      Name: tree.Name($3),
      Count: $1.int64(),
	  }

nit: there appear to be tabs for seven options in a row here.

---

pkg/sql/sem/tree/cursor.go, line 42 at r1 (raw file):

	}
	ctx.WriteString("CURSOR ")
	if node.Hold {

Does it not matter to distinguish between unspecified hold and explicit "without hold"?

[jordanlewis]

Added the "no schema changes" limitation in another commit.

This is ready for another review! @otan any further review from you is definitely appreciated.

I'm at least somewhat convinced that the txn seqnum implementation I've created here makes sense. Using a LeafTxn seems to be impractical without a fairly large refactor of LeafTxn due to limitations I've touched on above.

@knz, as far as the test case enumeration you've proposed, I've not done it yet because I don't yet see what an interesting condition would be besides the simple cases of modify-after-cursor-creation and modify-before-cursor-creation-in-same-txn which I've already exercised. Concurrent txn isn't interesting because it's already exercised exactly the same by any of our other concurrent txn testing.

[knz]

@nvanbenschoten can you help ascertain that the seqnum dance performed here makes sense?

[otan]

looks good to me, i'm unsure how to sign off on the seq num stuff though.

[nvanbenschoten]

The seqnum dance here makes sense to me. I think it would be worthwhile to add some kv-level testing for it, like we have for stepping modes in pkg/kv/kvclient/kvcoord/txn_interceptor_seq_num_allocator_test.go.

Reviewed 12 of 16 files at r1, 2 of 15 files at r17, 15 of 15 files at r23, 6 of 6 files at r24, 4 of 4 files at r25, 7 of 7 files at r26, 6 of 6 files at r27, 2 of 2 files at r28, 3 of 3 files at r29, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @jordanlewis, @otan, @rafiss, and @yuzefovich)

---

pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 1309 at r27 (raw file):

	tc.mu.Lock()
	defer tc.mu.Unlock()
	tc.interceptorAlloc.txnSeqNumAllocator.readSeq = seq

Should we add any validation here, like that this is >= 0 and <= writeSeq?

---

pkg/sql/sql_cursor.go, line 52 at r25 (raw file):

Previously, otan (Oliver Tan) wrote…

missing a comment line by the looks of it

Yes, this looks off.

---

pkg/sql/sql_cursor.go, line 180 at r27 (raw file):

	// running FETCH on a CURSOR does not close it.

	// Reset the trasnaction's read sequence number to what it was before the

s/trasnaction/transaction/

---

pkg/sql/parser/sql.y, line 5122 at r23 (raw file):

    $$.val = tree.Insensitive
  }
| ASENSITIVE

Do we like that we won't even parse SENSITIVE? Would it be better to include that in the grammar but then throw a descriptive error?

---

pkg/sql/sem/tree/cursor.go, line 51 at r23 (raw file):

// CursorScrollOption represents the scroll option, if one was given, for a
// DECLARE statement.
type CursorScrollOption int

nit: should we make these enum types int8? That will allow DeclareCursor to pack better.

---

pkg/sql/logictest/testdata/logic_test/cursor, line 99 at r27 (raw file):

query IT
FETCH RELATIVE 12 foo

Should we perform another INSERT after this first fetch as well, to make sure the cursor is insensitive to that as well?

---

pkg/sql/logictest/testdata/logic_test/cursor, line 99 at r27 (raw file):

query IT
FETCH RELATIVE 12 foo

Should we perform a read-only operation outside of the cursor to demonstrate that it is sensitive to these INSERTs and to demonstrate the cursor hasn't corrupted the read seq num state?

[jordanlewis]

TFTR!

I added a test. I don't think it's that great. Basically it just makes sure that the txnSeqNumAllocator doesn't choke when you edit the read seqnum and send more commands. I'm just not sure if this is testing much. Did you have something else in mind as well?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @jordanlewis, @nvanbenschoten, @otan, @rafiss, and @yuzefovich)

---

pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 1309 at r27 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should we add any validation here, like that this is >= 0 and <= writeSeq?

Done.

---

pkg/sql/sql_cursor.go, line 52 at r25 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Yes, this looks off.

Done.

---

pkg/sql/parser/sql.y, line 5122 at r23 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do we like that we won't even parse SENSITIVE? Would it be better to include that in the grammar but then throw a descriptive error?

SENSITIVE isn't accepted by pg either so this would be a custom extension. I feel comfortable omitting it.

---

pkg/sql/sem/tree/cursor.go, line 51 at r23 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: should we make these enum types int8? That will allow DeclareCursor to pack better.

Done.

---

pkg/sql/logictest/testdata/logic_test/cursor, line 25 at r24 (raw file):

Previously, otan (Oliver Tan) wrote…

can you add a test for a cursor which errors, e.g. selects a column which doesn't exist in a table?

Done.

---

pkg/sql/logictest/testdata/logic_test/cursor, line 99 at r27 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should we perform another INSERT after this first fetch as well, to make sure the cursor is insensitive to that as well?

Good idea, done.

---

pkg/sql/logictest/testdata/logic_test/cursor, line 99 at r27 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should we perform a read-only operation outside of the cursor to demonstrate that it is sensitive to these INSERTs and to demonstrate the cursor hasn't corrupted the read seq num state?

Good idea, done.

[jordanlewis]

I'm going to go ahead and merge this as I've resolved all outstanding comments, see no show-stopping issues with the KV interaction thanks to @nvanbenschoten's review, and have approval from @otan. Thanks for the reviews, all!

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[rafiss]

Looks like this caused a panic in #76883 (comment)

It seems related to using a DECLARE statement in the extended protocol. One thing I noticed is this condition:

cockroach/pkg/sql/sql_cursor.go

Line 33 in 96d102a

if p.autoCommit {

It probably is not what we want to be checking when in the extended protocol. p.autocommit will be false for implicit transactions in the extended protocol.