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

self.param_types.len() <= (i16::MAX as usize) should change to u16::MAX #671

Closed
sify21 opened this issue Sep 4, 2020 · 5 comments
Closed
Labels
E-easy good first issue Good for newcomers help wanted Extra attention is needed

Comments

@sify21
Copy link

sify21 commented Sep 4, 2020

I'm using postgresql, sqlx="0.4.0-beta.1".
My struct is like:

#[derive(Serialize, Deserialize, Debug, PartialEq, FromRow)]
pub struct Struct1 {
    pub a1: i16,
    pub a2: String,
    pub a3: Option<String>,
    pub a4: Option<serde_json::Value>,
    pub a5: serde_json::Value,
    pub a6: Option<String>,
    pub a7: Option<serde_json::Value>,
    pub a8: Option<f32>,
    pub a9: Option<f32>,
    pub a10: Option<f32>,
    pub a11: Option<String>,
    pub a12: Option<f32>,
    pub a13: Option<f32>,
    pub a14: Option<f32>,
    pub a15: Option<String>,
    pub a16: Option<String>,
    pub a17: Option<String>,
    pub a18: Vec<String>,
}

My query string is like:

insert into table (...fields) values ($1,....),... on conflict(a2) do update set a1=excluded.a1,a2=.... returning *

I'm using this method to get results:

sqlx::query_as().bind().fetch_all()

This assertion fails:

thread 'main' panicked at 'assertion failed: self.param_types.len() <= (i16::MAX as usize)', ~/.cargo/registry/src/github.com-1ecc6299db9ec823/sqlx-core-0.4.0-beta.1/src/postgres/message/parse.rs:30:13

what's the problem here?

@sify21
Copy link
Author

sify21 commented Sep 4, 2020

I have another smaller struct using the same pattern like above which succeeds.
Is it the number of columns in the table?

#[derive(Serialize, Deserialize, Debug, PartialEq, FromRow)]
pub struct Struct2 {
    pub a1: i16,
    pub a2: String,
    pub a3: Vec<String>,
    pub a4: Value,
}

I'm transiting from diesel-rs. In diesel I need to enable a feature called 128-column-tables.

@sify21
Copy link
Author

sify21 commented Sep 4, 2020

Oh, I debugged and found the problem. It's the PARSE command's parameter length being too long.
libpq restricts to 65535. So when I'm using diesel, I insert 3000 Struct1 at a time, which means 54000 parameters.
But why does sqlx use i16::MAX? Shouldn't it be u16::MAX?

@sify21 sify21 changed the title got error: assertion failed: self.param_types.len() <= (i16::MAX as usize) assertion: self.param_types.len() <= (i16::MAX as usize) should change to u16::MAX Sep 4, 2020
@sify21 sify21 changed the title assertion: self.param_types.len() <= (i16::MAX as usize) should change to u16::MAX self.param_types.len() <= (i16::MAX as usize) should change to u16::MAX Sep 4, 2020
@abonander
Copy link
Collaborator

So this is actually due to ambiguous documentation. We're also not the only ones to make the same mistake. The Postgres JDBC driver appears to cap the parameter count at Short.MAX_VALUE which is the same as i16::MAX:

jOOQ/jOOQ#5701
https://github.com/pgjdbc/pgjdbc/blob/4a4e664a868421e4ea8283b685a8abdf4ec907ab/pgjdbc/src/main/java/org/postgresql/core/PGStream.java#L346

The Postgres protocol documentation specifies that the bind parameter count is an Int16 and that IntN is "An n-bit integer in network byte order (most significant byte first)." It doesn't specify whether that integer is signed or unsigned, and context doesn't help either because in the same message definition (Bind (F)), -1 is used as the sentinel value for NULL, suggesting a signed interpretation of the int.

https://www.postgresql.org/docs/current/protocol-message-types.html
https://www.postgresql.org/docs/current/protocol-message-formats.html

The functions in libpq that take lists of parameters use a plain int for the number of parameters, and doesn't explicitly document the maximum number of bind parameters.

https://www.postgresql.org/docs/current/libpq-exec.html

You have to dig into the implementation to find this assertion: https://github.com/postgres/postgres/blob/7559d8ebfa11d98728e816f6b655582ce41150f3/src/interfaces/libpq/fe-exec.c#L1304

@abonander
Copy link
Collaborator

Closed by #1141

@abonander
Copy link
Collaborator

I was checking Postgres' query parser to see if it placed a stricter limit on the value since I found that other databases are a little weird when it comes to bind parameter limits (SQLite is 32766 since the 1-based index must fit in a 2-byte signed integer), but the parser also just represents the index as an int: https://github.com/postgres/postgres/blob/c0d1c641cbe433d1b6304bc1e3a2d8cd38b9a8e5/src/include/nodes/parsenodes.h#L263

and simply uses atol() to parse the value: https://github.com/postgres/postgres/blob/c0d1c641cbe433d1b6304bc1e3a2d8cd38b9a8e5/src/backend/parser/scan.l#L978

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants