Skip to content
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

sql: Different handling of prepared statement arguments compared to Postgresql #86375

Closed
Tracked by #8788
plcplc opened this issue Aug 18, 2022 · 2 comments · Fixed by #86904
Closed
Tracked by #8788

sql: Different handling of prepared statement arguments compared to Postgresql #86375

plcplc opened this issue Aug 18, 2022 · 2 comments · Fixed by #86904
Assignees
Labels
A-tools-hasura C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-community Originated from the community X-blathers-triaged blathers was able to find an owner

Comments

@plcplc
Copy link

plcplc commented Aug 18, 2022

Describe the problem

CockroachDB handles arguments to prepared statements differently from Postgres.
CockroachDB requires all specified arguments to a prepared statement are used in the statement being prepared, while Postgresql doesn't.

To Reproduce

On CockroachDB:

defaultdb=> prepare args_test(int) as select 42;
ERROR:  too many types provided

Expected behavior

On Postgresql:

postgres=# prepare args_test(int) as select 42;
PREPARE

Environment:

  • CockroachDB version v22.1.5
  • Server OS: Linux/NixOS
  • Client app: psql

Additional context

While I don't think this policy is unreasonable in and of itself, it does introduce an incompatibility with Postgresql.

This incompatibility surfaced for me in the context of adding support for CockroachDB to the Hasura GraphQL Engine.

Parallel hasura issue: hasura/graphql-engine#8792

Jira issue: CRDB-18726
Epic CRDB-11916

@plcplc plcplc added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Aug 18, 2022
@blathers-crl
Copy link

blathers-crl bot commented Aug 18, 2022

Hello, I am Blathers. I am here to help you get the issue triaged.

Hoot - a bug! Though bugs are the bane of my existence, rest assured the wretched thing will get the best of care here.

I have CC'd a few people who may be able to assist you:

  • @cockroachdb/sql-experience (found keywords: Hasura)

If we have not gotten back to your issue within a few business days, you can try the following:

  • Join our community slack channel and ask on #cockroachdb.
  • Try find someone from here if you know they worked closely on the area and CC them.

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

@blathers-crl blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Aug 18, 2022
@rafiss
Copy link
Collaborator

rafiss commented Aug 23, 2022

Thanks for this issue.

For when we work on this, here are some tests we should add that verify the Postgres behavior that we should match.

# Regression test for #86375 - make sure there can be more provided types than
# placeholders during PREPARE (and vice-versa). During EXECUTE, the counts
# of placeholders and arguments must match.

statement ok
PREPARE args_test_many(int, int) as select $1

query I
EXECUTE args_test_many(1, 2)
----
1

query error wrong number of parameters for prepared statement "args_test_many": expected 2, got 1
EXECUTE args_test_many(1)

statement ok
PREPARE args_test_few(int) as select $1, $2::int

query II
EXECUTE args_test_few(1, 2)
----
1  2

query error wrong number of parameters for prepared statement "args_test_few": expected 2, got 1
EXECUTE args_test_few(1)

@rafiss rafiss changed the title Different handling of prepared statement arguments compared to Postgresql sql: Different handling of prepared statement arguments compared to Postgresql Aug 23, 2022
ZhouXing19 added a commit to ZhouXing19/cockroach that referenced this issue Aug 25, 2022
Previously, we only allow having the same number of parameters and placeholders
in a `PREPARE` statement. This is not compatible with Postgres14's behavior.

This commit is to loosen the restriction and enable this compatibility.

fixes cockroachdb#86375

Release justification:
Release note: allow mismatch type numbers in `PREPARE` statement
ZhouXing19 added a commit to ZhouXing19/cockroach that referenced this issue Aug 26, 2022
Previously, we only allow having the same number of parameters and placeholders
in a PREPARE statement. This is not compatible with Postgres14's behavior.

This commit is to loosen the restriction and enable this compatibility.
We now take max(#placeholders, #parameters) as the true length
of parameters of the prepare statement. For each parameter, we first
have the type hint as the default value for each type. Then if the type can be
deduced from the query stmt, we let the deduced value override the type hint.

fixes cockroachdb#86375

Release justification: Low risk, high benefit changes to existing functionality
Release note: allow mismatch type numbers in PREPARE statement
ZhouXing19 added a commit to ZhouXing19/cockroach that referenced this issue Aug 29, 2022
Previously, we only allow having the same number of parameters and placeholders
in a PREPARE statement. This is not compatible with Postgres14's behavior.

This commit is to loosen the restriction and enable this compatibility.
We now take max(#placeholders, #parameters) as the true length
of parameters of the prepare statement. For each parameter, we first
have the type hint as the default value for each type. Then if the type can be
deduced from the query stmt, we let the deduced value override the type hint.

fixes cockroachdb#86375

Release justification: Low risk, high benefit changes to existing functionality
Release note: allow mismatch type numbers in PREPARE statement
ZhouXing19 added a commit to ZhouXing19/cockroach that referenced this issue Aug 30, 2022
Previously, we only allow having the same number of parameters and placeholders
in a PREPARE statement. This is not compatible with Postgres14's behavior.

This commit is to loosen the restriction and enable this compatibility.
We now take max(#placeholders, #parameters) as the true length
of parameters of the prepare statement. For each parameter, we first
have the type hint as the default value for each type. Then if the type can be
deduced from the query stmt, we let the deduced value override the type hint.

fixes cockroachdb#86375

Release justification: Low risk, high benefit changes to existing functionality
Release note: allow mismatch type numbers in PREPARE statement
craig bot pushed a commit that referenced this issue Aug 30, 2022
86563: ts: fix the pretty-printing of tsd keys r=abarganier a=knz

Found while working on #86524.

Release justification: bug fix

Release note (bug fix): When printing keys and range start/end
boundaries for time series, the displayed structure of keys
was incorrect. This is now fixed.

86904: sql: allow mismatch type numbers in `PREPARE` statement r=rafiss a=ZhouXing19

Previously, we only allow having the same number of parameters and placeholders
in a `PREPARE` statement. This is not compatible with Postgres14's behavior.

This commit is to loosen the restriction and enable this compatibility.
We now take `max(#placeholders, #parameters)` as the true length
 of parameters of the prepare statement. For each parameter, we first
look at the type deduced from the query stmt. If we can't deduce it, 
we take the type hint for this param.

I.e. we now allow queries such as 

```
PREPARE args_test_many(int, int) as select $1
// 2 parameters, but only 1 placeholder in the query.

PREPARE args_test_few(int) as select $1, $2::int
// 1 parameter, but 2 placeholders in the query.
```

fixes #86375

Release justification: Low risk, high benefit changes to existing functionality
Release note: allow mismatch type numbers in `PREPARE` statement

87105: roachtest: skip flaky acceptance/version-upgrade r=tbg a=adityamaru

Skipping the flaky roachtest while we stabilize it.

Informs: #87104

Release note: None

Release justification: testing only change

87117: bazci: fix output path computation r=rail a=rickystewart

These updates were happening in-place so `bazci` was constructing big,
silly paths like `backupccl_test/shard_6_of_16/shard_7_of_16/shard_13_of_16/...`
We just need to copy the variable here.

Release justification: Non-production code changes
Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Jane Xing <[email protected]>
Co-authored-by: adityamaru <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
@craig craig bot closed this as completed in be62a76 Aug 30, 2022
ZhouXing19 added a commit to ZhouXing19/cockroach that referenced this issue Aug 31, 2022
Previously, we only allow having the same number of parameters and placeholders
in a PREPARE statement. This is not compatible with Postgres14's behavior.

This commit is to loosen the restriction and enable this compatibility.
We now take max(#placeholders, #parameters) as the true length
of parameters of the prepare statement. For each parameter, we first
have the type hint as the default value for each type. Then if the type can be
deduced from the query stmt, we let the deduced value override the type hint.

fixes cockroachdb#86375

Release justification: Low risk, high benefit changes to existing functionality
Release note (sql change): allow mismatch type numbers in PREPARE statement
ZhouXing19 added a commit to ZhouXing19/cockroach that referenced this issue Aug 31, 2022
Previously, we only allow having the same number of parameters and placeholders
in a PREPARE statement. This is not compatible with Postgres14's behavior.

This commit is to loosen the restriction and enable this compatibility.
We now take max(#placeholders, #parameters) as the true length
of parameters of the prepare statement. For each parameter, we first
have the type hint as the default value for each type. Then if the type can be
deduced from the query stmt, we let the deduced value override the type hint.

fixes cockroachdb#86375

Release justification: Low risk, high benefit changes to existing functionality
Release note (sql change): allow mismatch type numbers in PREPARE statement
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tools-hasura C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-community Originated from the community X-blathers-triaged blathers was able to find an owner
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants