pgwire: Add support for cursors with special characters

[rimadeodhar]

IIn CockroachDB and Postgres, it is possible to declare
cursors with special characters enclosed within double
quotes, for e.g. "1-2-3". Currently, we store the name
as an unescaped string which causes problems during the
pgwire DESCRIBE step for looking up the cursor. We should
be storing using the tree.Name datatype for the cursor name
while storing and looking up cursors. This PR updates the code
to start using tree.Name instead of raw strings for handling
cursor names. This fixes the issue where the pgwire DESCRIBE
step fails while attempting to look up cursors with names
containing special characters.

Resolves #84261

Release note (bug fix): The pgwire DESCRIBE step no longer
fails with an error while attempting to look up cursors
declared with names containing special characters.

[cockroach-teamcity]

This change is 

[knz]

This is incorrect.

You want to use the tree.Name type (probably also as argument to getSQLCusor and the accessor's getCursor, and as datatype for descCmd.Name above). tree.Name should take care of quoting for you.

Also don't forget to test identifiers that contain a double quote character as well.

[knz]

Also I'm seeing this:

//  it is stored within the cursor
// map as enclosed in double quotes.

That doesn't seem right. The cursor map should use tree.Name as key type, and the double quotes should not be included. This means that the code that populates the map has done something weird.

[rimadeodhar]

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

---

pkg/sql/conn_executor_prepare.go line 658 at r1 (raw file):

Previously, knz (kena) wrote…

Also I'm seeing this:

//  it is stored within the cursor
// map as enclosed in double quotes.

That doesn't seem right. The cursor map should use tree.Name as key type, and the double quotes should not be included. This means that the code that populates the map has done something weird.

Ha ok, that makes sense. Let me make those changes!

[rimadeodhar]

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

---

pkg/sql/conn_executor_prepare.go line 658 at r1 (raw file):

Previously, rimadeodhar (Rima Deodhar) wrote…

Ha ok, that makes sense. Let me make those changes!

This is done.

[knz]

declare
cursors with invalid identifiers enclosed within double
quotes

I would not keep the word "invalid" in this sentence. The identifiers are completely valid. It's just that they use special characters and thus need to be quoted.

Also, I think the remainder of the commit message does not any more describe what is happening in this PR.

Also, I recommend you add some pg_cursor logic tests.

Finally, for all the tests (pgtests, go unit tests, logic tests), please ensure each test includes at least one of the following:

• an identifier containing a space, e.g. a b (DECLARE "a b" CURSOR ...)
• an identifier containing singe and double quotes, e.g. a"b'c (DECLARE "a""b'c" CURSOR ...)
• an identifier containing a backslash character e.g. a\b (DECLARE "a\b" CURSOR ...)

Then ensure that the expected test output matches what postgres would do with it.

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

---

pkg/sql/conn_executor_prepare.go line 566 at r2 (raw file):

	switch descCmd.Type {
	case pgwirebase.PrepareStatement:
		ps, ok := ex.extraTxnState.prepStmtsNamespace.prepStmts[string(descCmd.Name)]

Any reason not to change the key type of prepStmts to be tree.Name directly?

---

pkg/sql/pg_catalog.go line 3774 at r2 (raw file):

			}
			if err := addRow(
				tree.NewDString(name.String()), /* name */

I don't believe this is correct. This .String() call will introduce quotes, where none are desired.
You can see for yourself what postgres does:

kena=> declare "a b" cursor for table t;
DECLARE CURSOR
kena=> select * from pg_cursors;
 name |             statement             | is_holdable | is_binary | is_scrollable |         creation_time
------+-----------------------------------+-------------+-----------+---------------+-------------------------------
 a b  | declare "a b" cursor for table t; | f           | f         | t             | 2022-08-13 18:16:19.357203+02
(1 row)

See the name column contains the raw identifier, there's no quoting involved.

---

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

			}

			if p.extendedEvalCtx.PreparedStatementState.HasPortal(s.Name.String()) {

I think this .String() call is misplaced as well.

---

pkg/sql/pgwire/conn_test.go line 697 at r2 (raw file):

	}

	if desc.Name.String() != expName {

I believe this .String() call should not be there.

Also please extend the test to include identifiers with special characters.

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @rimadeodhar)

---

pkg/sql/pg_catalog.go line 3774 at r2 (raw file):

Previously, knz (kena) wrote…

I don't believe this is correct. This .String() call will introduce quotes, where none are desired.
You can see for yourself what postgres does:

kena=> declare "a b" cursor for table t;
DECLARE CURSOR
kena=> select * from pg_cursors;
 name |             statement             | is_holdable | is_binary | is_scrollable |         creation_time
------+-----------------------------------+-------------+-----------+---------------+-------------------------------
 a b  | declare "a b" cursor for table t; | f           | f         | t             | 2022-08-13 18:16:19.357203+02
(1 row)

See the name column contains the raw identifier, there's no quoting involved.

so in this case we can use tree.AsStringWithFlags(name, tree.FmtBareStrings) or perhaps even string(name)

can we also add a test for a cursor name with quotes in the name

rafiss@127:postgres> declare "a"" b" cursor for table t;
DECLARE CURSOR
Time: 0.001s
rafiss@127:postgres> select * from pg_cursors;
+------+------------------------------------+-------------+-----------+---------------+-------------------------------+
| name | statement                          | is_holdable | is_binary | is_scrollable | creation_time                 |
|------+------------------------------------+-------------+-----------+---------------+-------------------------------|
| a" b | declare "a"" b" cursor for table t | False       | False     | True          | 2022-08-17 20:07:15.322335+00 |
| a b  | declare "a b" cursor for table t   | False       | False     | True          | 2022-08-17 20:07:02.611308+00 |
+------+------------------------------------+-------------+-----------+---------------+-------------------------------+

That is, let's add this test to pkg/sql/logictest/testdata/logic_test/pg_catalog

[rimadeodhar]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @rimadeodhar)

---

pkg/sql/conn_executor_prepare.go line 566 at r2 (raw file):

Previously, knz (kena) wrote…

Any reason not to change the key type of prepStmts to be tree.Name directly?

I was planning to do it in a follow up PR but probably might be worth address this within this PR itself.

---

pkg/sql/pg_catalog.go line 3774 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

so in this case we can use tree.AsStringWithFlags(name, tree.FmtBareStrings) or perhaps even string(name)

can we also add a test for a cursor name with quotes in the name

rafiss@127:postgres> declare "a"" b" cursor for table t;
DECLARE CURSOR
Time: 0.001s
rafiss@127:postgres> select * from pg_cursors;
+------+------------------------------------+-------------+-----------+---------------+-------------------------------+
| name | statement                          | is_holdable | is_binary | is_scrollable | creation_time                 |
|------+------------------------------------+-------------+-----------+---------------+-------------------------------|
| a" b | declare "a"" b" cursor for table t | False       | False     | True          | 2022-08-17 20:07:15.322335+00 |
| a b  | declare "a b" cursor for table t   | False       | False     | True          | 2022-08-17 20:07:02.611308+00 |
+------+------------------------------------+-------------+-----------+---------------+-------------------------------+

That is, let's add this test to pkg/sql/logictest/testdata/logic_test/pg_catalog

Aah I see, wasn't aware of this difference with postgres. I kept it as .String() to keep it compatible with what we do today but seems like we in CRDB we keep the double quotes within the catalog:

[email protected]:26257/defaultdb  OPEN> declare "a b" cursor for table emp;
DECLARE CURSOR


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

[email protected]:26257/defaultdb  OPEN> select * from pg_cursors;
  name  | statement | is_holdable | is_binary | is_scrollable |         creation_time
--------+-----------+-------------+-----------+---------------+--------------------------------
  "a b" | TABLE emp |      f      |     f     |       f       | 2022-08-17 21:57:15.943795+00
(1 row)

I'll fix this.

[rimadeodhar]

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

---

pkg/sql/conn_executor_prepare.go line 566 at r2 (raw file):

Previously, rimadeodhar (Rima Deodhar) wrote…

I was planning to do it in a follow up PR but probably might be worth address this within this PR itself.

Updating it in this PR is making this PR very big. I will be putting out a PR stacked on top of this one which updates the maps within prepStmts to be keyed by tree.Name instead of string.

---

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

Previously, knz (Raphael 'kena' Poss) wrote…

I think this .String() call is misplaced as well.

We need this call since prepared statement state stores the portal name using a raw string. I plan to fix this by updating the prepStmtNamespace structs to key maps using tree.Name as well in a subsequent PR.

---

pkg/sql/pgwire/conn_test.go line 697 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

I believe this .String() call should not be there.

Also please extend the test to include identifiers with special characters.

Done.

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @rimadeodhar)

---

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

Previously, rimadeodhar (Rima Deodhar) wrote…

We need this call since prepared statement state stores the portal name using a raw string. I plan to fix this by updating the prepStmtNamespace structs to key maps using tree.Name as well in a subsequent PR.

i'm not following still -- in the conn_executor_prepare.go file we are converting using string(name), but here we are converting with name.String()

---

pkg/sql/pgwire/testdata/pgtest/portals line 1492 at r3 (raw file):

# Check declaring cursor with a name enclosed in double quotes
send
Query {"String": "BEGIN"}

just to confirm -- did you run this test against postgres as well? you can do so with:

 ./dev test pkg/sql/pgwire --filter='TestPGTest/portals' --test-args='-addr=localhost:5432 -user=rafiss' --rewrite

(adjust your username/port as necessary so that it points to a locally running postgres server)

[rafiss]

i just had questions -- i'll mark it as "changes requested" just to flag them

[knz]

but i'll let rafi finalize this with you.

Reviewed 8 of 8 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz and @rimadeodhar)

[rimadeodhar]

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

---

pkg/sql/pgwire/testdata/pgtest/portals line 1492 at r3 (raw file):

./dev test pkg/sql/pgwire --filter='TestPGTest/portals' --test-args='-addr=localhost:5432 -user=rafiss' --rewrite
I ran this test against a locally running postgres server and I noticed that it updated the RowDescription part of the test to update the "DataTypeOID":23,"DataTypeSize":4 which differs from what is output when I run it against locally running CRDB. This makes sense to me since integers have a different data type size but what is the right output to put within the test by default? I'm guessing it should be the CRDB one right?

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @knz and @rimadeodhar)

---

pkg/sql/pgwire/testdata/pgtest/portals line 1492 at r3 (raw file):

Previously, rimadeodhar (Rima Deodhar) wrote…

./dev test pkg/sql/pgwire --filter='TestPGTest/portals' --test-args='-addr=localhost:5432 -user=rafiss' --rewrite
I ran this test against a locally running postgres server and I noticed that it updated the RowDescription part of the test to update the "DataTypeOID":23,"DataTypeSize":4 which differs from what is output when I run it against locally running CRDB. This makes sense to me since integers have a different data type size but what is the right output to put within the test by default? I'm guessing it should be the CRDB one right?

in this case, you can resolve it either by adding an explicit cast to the query (look for other usages of generate_series in the pgtest testdata), or you can use the ignore_data_type_sizes and ignore_type_oids flag on the until directive.

[rafiss]

lgtm!

Reviewed 7 of 8 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @knz and @rimadeodhar)

[rimadeodhar]

TFTR!

bors r=knz,rafiss

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from c0aa573 to blathers/backport-release-22.1-85859: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.

---

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