-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pg_catalog: indoption was not encoded correctly #111911
Conversation
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. |
@@ -1879,7 +1879,7 @@ https://www.postgresql.org/docs/9.5/catalog-pg-index.html`, | |||
// Also fill in indoption for each column to indicate if the index | |||
// is ASC/DESC and if nulls appear first/last. | |||
collationOids := tree.NewDArray(types.Oid) | |||
indoption := tree.NewDArray(types.Int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test that reproduces the failure without the fix and passes with the fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here's a pgwire datadriven test that repros the issue. it can be added to a file in pkg/sql/pgwire/testdata/pgtest/
send
Query {"String": "DROP TABLE IF EXISTS t"}
Query {"String": "CREATE TABLE t (a int primary key, b text)"}
----
until ignore=NoticeResponse
ReadyForQuery
ReadyForQuery
----
{"Type":"CommandComplete","CommandTag":"DROP TABLE"}
{"Type":"ReadyForQuery","TxStatus":"I"}
{"Type":"CommandComplete","CommandTag":"CREATE TABLE"}
{"Type":"ReadyForQuery","TxStatus":"I"}
send
Parse {"Query": "select i.indoption from pg_index i join pg_class c on i.indrelid = c.oid where c.relname = 't';"}
Bind {"ParameterFormatCodes": [0], "ResultFormatCodes": [1]}
Execute
Sync
----
until
ReadyForQuery
----
{"Type":"ParseComplete"}
{"Type":"BindComplete"}
{"Type":"DataRow","Values":[{"binary":"0000000100000000000000150000000100000000000000020000"}]}
{"Type":"CommandComplete","CommandTag":"SELECT 1"}
{"Type":"ReadyForQuery","TxStatus":"I"}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafiss Thanks for that I updated it to add the test
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall)
pkg/sql/pg_catalog.go
line 0 at r1 (raw file):
Previously, ecwall (Evan Wall) wrote…
(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)
Can you add a test that reproduces the failure without the fix and passes with the fix?
Done.
---- | ||
|
||
until | ||
ReadyForQuery |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add a comment that says the test is checking that the size of the binary data is returned as 2 byte ints, and maybe include a link to #111911 ?
Previously, the indoption field inside pg_index was encoded incorrectly, which could cause problems for binary clients. Specifically, an int8 was being sent across the wire, then int2vectors are supposed to be made of int2. To address this, this patch ensures that an int2 is used and adds assertion inside the conversion code (for int2vector) to avoid future problems. Fixes: cockroachdb#111907 Release note (bug fix): indoption inside pg_index was not properly encoded, causing clients to be unable to decode it as int2vector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall and @rafiss)
pkg/sql/pgwire/testdata/pgtest/int2vector
line 23 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
could you add a comment that says the test is checking that the size of the binary data is returned as 2 byte ints, and maybe include a link to #111911 ?
Done.
bors r+ |
Build succeeded: |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error setting reviewers, but backport branch blathers/backport-release-22.2-111911 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/111956/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. [] Backport to branch 22.2.x failed. See errors above. error setting reviewers, but backport branch blathers/backport-release-23.1-111911 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/111957/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. [] Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Previously, the indoption field inside pg_index was encoded incorrectly, which could cause problems for binary clients. Specifically, an int8 was being sent across the wire, then int2vectors are supposed to be made of int2. To address this, this patch ensures that an int2 is used and adds assertion inside the conversion code (for int2vector) to avoid future problems.
Fixes: #111907
Release note (bug fix): indoption inside pg_index was not properly encoded, causing clients to be unable to decode it as int2vector.