sql: support #typeHints greater than #placeholders for prepare stmt
#87535
Conversation
|
This change is |
ZhouXing19
changed the title
#typeHints greater than #placeholders for prepare stmt
|
The failed tests in CI don't seem related so RFAL! |
| @@ -38,11 +38,6 @@ describe('select', () => { | |||
|
|
|||
| describe('error cases', () => { | |||
| const cases = [{ | |||
| name: 'not enough params', | |||
There was a problem hiding this comment.
did you confirm that postgres does not expect this behavior?
There was a problem hiding this comment.
Added a test here to verify.
ZhouXing19
force-pushed
the
fix-prepare-0906
branch
from
14dc93d to
4e42e72
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @ZhouXing19)
pkg/acceptance/testdata/node/base-test.js line 42 at r2 (raw file):
const cases = [{ name: 'not enough params', query: { text: 'SELECT 3', values: ['foo'] },
hm actually i tried running this against postgres (using the node-pg driver), and it does fail. i think this is a pgtest to reproduce it:
Parse {"Name": "s", "Query": "select 3"}
Bind {"DestinationPortal": "p", "PreparedStatement": "s", "ParameterFormatCodes": [0], "Parameters": [{"text":"foo"}]}
Describe {"ObjectType": "P", "Name": "p"}
Execute {"Portal": "p"}
Sync
pkg/sql/conn_executor_prepare.go line 389 at r2 (raw file):
return retErr( pgwirebase.NewProtocolViolationErrorf( "expected %d arguments, got %d", numQArgs, len(bindCmd.Args)))
i see in revision 2 this was added back in. are there already tests for it?
Previous, we only support pgwire prepare stmt with the number of typehints
equal or smaller than the number of the placeholders in the query. E.g. the
following usage are not supported:
```
Parse {"Name": "s2", "Query": "select $1", "ParameterOIDs":[1043, 1043, 1043]}
```
Where there are 1 placeholder in the query, but 3 type hints.
This commit is to allow mismatching #typeHints and #placeholders. The former
can be larger than the latter now.
Release justification: Low risk, high benefit changes to existing functionality
Release note (sql change): For pgwire-level prepare statements, support the case
where the number of the type hints is greater than the number of placeholders
in the given query.
ZhouXing19
force-pushed
the
fix-prepare-0906
branch
from
4e42e72 to
f1ce34c
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor_prepare.go line 389 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i see in revision 2 this was added back in. are there already tests for it?
Yes, it was because removing this part made this test pass:
Bind {"DestinationPortal": "p2", "PreparedStatement": "s2", "ParameterFormatCodes": [0], "Parameters": [{"text":"whitebear"}]}
Execute {"Portal": "p2"}
Sync
which is incorrect. I've added this test to the pgtest too.
pkg/acceptance/testdata/node/base-test.js line 42 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
hm actually i tried running this against postgres (using the node-pg driver), and it does fail. i think this is a pgtest to reproduce it:
Parse {"Name": "s", "Query": "select 3"} Bind {"DestinationPortal": "p", "PreparedStatement": "s", "ParameterFormatCodes": [0], "Parameters": [{"text":"foo"}]} Describe {"ObjectType": "P", "Name": "p"} Execute {"Portal": "p"} Sync
Yeah good point, I added this one to the pgtest, and put back what we had here.
There was a problem hiding this comment.
thanks for the fix!
Reviewed 11 of 14 files at r1, 1 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status:
|
TFTR! |
|
Build succeeded: |
Previous, we only support pgwire prepare stmt with the number of typehints equal or smaller than the number of the placeholders in the query. E.g. the following usage are not supported:
Where there is 1 placeholder in the query, but 3 type hints.
This commit is to allow mismatching #typeHints and #placeholders. The former can be larger than the latter now.
This feature is needed for CRDB's support for Hasura GraphQL Engine.
Release justification: Low risk, high benefit changes to existing functionality
Release note: For pgwire-level prepare statements, add support for the case where the number of the type hints is greater than the number of placeholders in the given query.