/diesel

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

Passing too much data results in a fatal error #446

Closed
sgrif opened this Issue Sep 17, 2016 · 0 comments

Comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
1 participant
@sgrif
@sgrif
Member

sgrif commented Sep 17, 2016

The largest number of bind parameters a single query can contain is 34464 (1/2 of the value of a short, as it uses a signed integer for the length). Attempting to pass more than that will result in a fatal error, which gives no meaningful information back. However, since we know this case will always occur, we can check for it and bubble up that error.

On SQLite it's a bit more complex, as it's based on the SQLITE_MAX_VARIABLE_NUMBER variable, which is 999 by default, but can change at runtime. I'm fine with just assuming it's 999 for now.

sgrif added a commit that referenced this issue Dec 10, 2016

@sgrif
Construct error messages properly when libpq returns `NULL` result 
I had originally thought that this was some weird condition that was the
result of the server getting a query with more than `34464` (`i16::MAX`)
bind parameters, where we got back an error but it had no message. We
panic in that case, as PG is documented that all errors have a message.
It turns out I was wrong for two reasons.

First, the number is 65535 (`u16::MAX`), not `34464`. (This might have
been something changed on the PG side in 9.6 which was released since I
last looked at this). Second, we were not getting back an error with no
message, we were getting a `NULL` return value from libpq. From [their
docs][]:

[their docs]: https://www.postgresql.org/docs/9.6/static/libpq-exec.html#LIBPQ-PQPREPARE

> A null result indicates out-of-memory or inability to send the command
> at all. Use PQerrorMessage to get more information about such errors.

`PQerrorMessage` means the error is on the connection. The reason that
we had no result was that the PG wire protocol uses a `u16` for the
number of bind parameters, so there's no way for the driver to have even
sent the message to the server.

Once we realized the error condition that we were seeing is more general
than just the bind parameter case, I've refactored the code to perform a
not null check as early as possible, and hide the `*mut PGresult`.

The test itself is written in a somewhat strange fashion. We're
constructing a query that has 70_000 bind parameters, as this is the
only case that I know of which will always cause libpq to return null. I
don't care what the error message is, just that it's not an empty
string. I've also written the test to explicitly match instead of unwrap
and use `#[should_panic]` because I want to be sure that it *didn't*
panic.

Ideally we would never see `*mut PGresult` anywhere in the code, and use
`Unique<PGresult>` instead. However, it is currently unstable. I had
originally implemented this to use `Unique` if `feature = "unstable"`
was set, but deleted it as the `#[cfg]` branches made the code harder to
comprehend. The only real benefit `Unique` gives us here is clarity of
intent. For the not-null and non-aliased hints to result in any
optimization by the compiler would require libpq to be statically
linked, and LTO to be happening at the LLVM layer, neither of which are
commonly true.

Fixes #446.
1d3d1b0

sgrif added a commit that referenced this issue Dec 10, 2016

@sgrif
Construct error messages properly when libpq returns `NULL` result 
I had originally thought that this was some weird condition that was the
result of the server getting a query with more than `34464` (`i16::MAX`)
bind parameters, where we got back an error but it had no message. We
panic in that case, as PG is documented that all errors have a message.
It turns out I was wrong for two reasons.

First, the number is 65535 (`u16::MAX`), not `34464`. (This might have
been something changed on the PG side in 9.6 which was released since I
last looked at this). Second, we were not getting back an error with no
message, we were getting a `NULL` return value from libpq. From [their
docs][]:

[their docs]: https://www.postgresql.org/docs/9.6/static/libpq-exec.html#LIBPQ-PQPREPARE

> A null result indicates out-of-memory or inability to send the command
> at all. Use PQerrorMessage to get more information about such errors.

`PQerrorMessage` means the error is on the connection. The reason that
we had no result was that the PG wire protocol uses a `u16` for the
number of bind parameters, so there's no way for the driver to have even
sent the message to the server.

Once we realized the error condition that we were seeing is more general
than just the bind parameter case, I've refactored the code to perform a
not null check as early as possible, and hide the `*mut PGresult`.

The test itself is written in a somewhat strange fashion. We're
constructing a query that has 70_000 bind parameters, as this is the
only case that I know of which will always cause libpq to return null. I
don't care what the error message is, just that it's not an empty
string. I've also written the test to explicitly match instead of unwrap
and use `#[should_panic]` because I want to be sure that it *didn't*
panic.

Ideally we would never see `*mut PGresult` anywhere in the code, and use
`Unique<PGresult>` instead. However, it is currently unstable. I had
originally implemented this to use `Unique` if `feature = "unstable"`
was set, but deleted it as the `#[cfg]` branches made the code harder to
comprehend. The only real benefit `Unique` gives us here is clarity of
intent. For the not-null and non-aliased hints to result in any
optimization by the compiler would require libpq to be statically
linked, and LTO to be happening at the LLVM layer, neither of which are
commonly true.

We know that `Send` and `Sync` are safe to implement on our new struct,
because the pointer is unaliased. We can delete those impls if we switch
to using `Unique` in the future, as it already implements those traits.

Fixes #446.
c96efa5

@sgrif sgrif referenced this issue Dec 10, 2016

Merged

Construct error messages properly when libpq returns `NULL` result #541

sgrif added a commit that referenced this issue Dec 10, 2016

@sgrif
Construct error messages properly when libpq returns `NULL` result 
I had originally thought that this was some weird condition that was the
result of the server getting a query with more than `34464` (`i16::MAX`)
bind parameters, where we got back an error but it had no message. We
panic in that case, as PG is documented that all errors have a message.
It turns out I was wrong for two reasons.

First, the number is 65535 (`u16::MAX`), not `34464`. (This might have
been something changed on the PG side in 9.6 which was released since I
last looked at this). Second, we were not getting back an error with no
message, we were getting a `NULL` return value from libpq. From [their
docs][]:

[their docs]: https://www.postgresql.org/docs/9.6/static/libpq-exec.html#LIBPQ-PQPREPARE

> A null result indicates out-of-memory or inability to send the command
> at all. Use PQerrorMessage to get more information about such errors.

`PQerrorMessage` means the error is on the connection. The reason that
we had no result was that the PG wire protocol uses a `u16` for the
number of bind parameters, so there's no way for the driver to have even
sent the message to the server.

Once we realized the error condition that we were seeing is more general
than just the bind parameter case, I've refactored the code to perform a
not null check as early as possible, and hide the `*mut PGresult`.

The test itself is written in a somewhat strange fashion. We're
constructing a query that has 70_000 bind parameters, as this is the
only case that I know of which will always cause libpq to return null. I
don't care what the error message is, just that it's not an empty
string. I've also written the test to explicitly match instead of unwrap
and use `#[should_panic]` because I want to be sure that it *didn't*
panic.

Ideally we would never see `*mut PGresult` anywhere in the code, and use
`Unique<PGresult>` instead. However, it is currently unstable. I had
originally implemented this to use `Unique` if `feature = "unstable"`
was set, but deleted it as the `#[cfg]` branches made the code harder to
comprehend. The only real benefit `Unique` gives us here is clarity of
intent. For the not-null and non-aliased hints to result in any
optimization by the compiler would require libpq to be statically
linked, and LTO to be happening at the LLVM layer, neither of which are
commonly true.

We know that `Send` and `Sync` are safe to implement on our new struct,
because the pointer is unaliased. We can delete those impls if we switch
to using `Unique` in the future, as it already implements those traits.

Fixes #446.
58940c3

sgrif added a commit that referenced this issue Dec 10, 2016

@sgrif
Construct error messages properly when libpq returns `NULL` result 
I had originally thought that this was some weird condition that was the
result of the server getting a query with more than `34464` (`i16::MAX`)
bind parameters, where we got back an error but it had no message. We
panic in that case, as PG is documented that all errors have a message.
It turns out I was wrong for two reasons.

First, the number is 65535 (`u16::MAX`), not `34464`. (This might have
been something changed on the PG side in 9.6 which was released since I
last looked at this). Second, we were not getting back an error with no
message, we were getting a `NULL` return value from libpq. From [their
docs][]:

[their docs]: https://www.postgresql.org/docs/9.6/static/libpq-exec.html#LIBPQ-PQPREPARE

> A null result indicates out-of-memory or inability to send the command
> at all. Use PQerrorMessage to get more information about such errors.

`PQerrorMessage` means the error is on the connection. The reason that
we had no result was that the PG wire protocol uses a `u16` for the
number of bind parameters, so there's no way for the driver to have even
sent the message to the server.

Once we realized the error condition that we were seeing is more general
than just the bind parameter case, I've refactored the code to perform a
not null check as early as possible, and hide the `*mut PGresult`.

The test itself is written in a somewhat strange fashion. We're
constructing a query that has 70_000 bind parameters, as this is the
only case that I know of which will always cause libpq to return null. I
don't care what the error message is, just that it's not an empty
string. I've also written the test to explicitly match instead of unwrap
and use `#[should_panic]` because I want to be sure that it *didn't*
panic.

Ideally we would never see `*mut PGresult` anywhere in the code, and use
`Unique<PGresult>` instead. However, it is currently unstable. I had
originally implemented this to use `Unique` if `feature = "unstable"`
was set, but deleted it as the `#[cfg]` branches made the code harder to
comprehend. The only real benefit `Unique` gives us here is clarity of
intent. For the not-null and non-aliased hints to result in any
optimization by the compiler would require libpq to be statically
linked, and LTO to be happening at the LLVM layer, neither of which are
commonly true.

We know that `Send` and `Sync` are safe to implement on our new struct,
because the pointer is unaliased. We can delete those impls if we switch
to using `Unique` in the future, as it already implements those traits.

Fixes #446.
7e378c4

@sgrif sgrif closed this in #541 Dec 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment