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

Regression: Postgres' limit on bind parameters is u16::MAX, not i16::MAX #3464

Closed
abonander opened this issue Aug 26, 2024 · 1 comment · Fixed by #3465
Closed

Regression: Postgres' limit on bind parameters is u16::MAX, not i16::MAX #3464

abonander opened this issue Aug 26, 2024 · 1 comment · Fixed by #3465
Labels
bug db:postgres Related to PostgreSQL

Comments

@abonander
Copy link
Collaborator

In #3441 I added this bit of code to silence the clippy::cast_possible_truncation warning:

let num_params = i16::try_from(arguments.len()).map_err(|_| {
err_protocol!(
"PgConnection::run(): too many arguments for query: {}",
arguments.len()
)
})?;

This will error if arguments.len() > 32767 (2^15 - 1).

We had modeled Bind::num_params as i16 so without any other context, this made sense: https://github.com/launchbadge/sqlx/blob/main/sqlx-postgres/src/message/bind.rs#L24

And this seems to be supported by the documentation which gives the field type as Int16: https://www.postgresql.org/docs/current/protocol-message-formats.html#PROTOCOL-MESSAGE-FORMATS-BIND

However, a key piece of information I had forgotten was that whether an integer is interpreted as signed or unsigned is context-dependent! And in fact, Postgres interprets this Int16 as unsigned, meaning the max number of parameters supported is actually 65535 (2^16 - 1). This is confirmed by Appendix K of the manual: https://www.postgresql.org/docs/current/limits.html

A quick, easy and correct fix would be to just change num_params to u16, but I think this should also be clarified in Postgres' docs.

@abonander abonander added bug db:postgres Related to PostgreSQL labels Aug 26, 2024
@abonander
Copy link
Collaborator Author

abonander commented Aug 26, 2024

On searching the Postgres mailings lists, it seems like the buck just keeps getting passed:

https://www.postgresql.org/message-id/1381.1388688071%40sss.pgh.pa.us

If you think it's worth being more precise, feel free to submit a patch.

Over 10 years ago.

https://www.postgresql.org/message-id/502E06DE.7090206%40enterprisedb.com

The sentence used to be factually correct, when it didn't mention
whether they're signed or unsigned. If we want to do better than that,
we'd need to go through all the mentions of IntN in the docs and
explicitly say which ones are signed and which ones unsigned. Perhaps
use Uint16 or Uint32 for the unsigned ones.

Over 12 years ago.

It seems to me that adjusting the description of the fields themselves either wasn't considered, or was an unsatisfactory solution.

It seems like the desired fix would involve going through all message formats in https://www.postgresql.org/docs/current/protocol-message-formats.html and changing unsigned values to a new unsigned integer type. The kind of busywork that no one wants to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug db:postgres Related to PostgreSQL
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@abonander and others