-
-
Notifications
You must be signed in to change notification settings - Fork 246
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
Fix Issue #792 #795
Fix Issue #792 #795
Conversation
The crux of the issue here is that `pgx_sys::Datum::from_datum(d, is_null)` can't ask the input Datum if it thinks it's null (via Datum::is_null(&self)), as nullness isn't a concept Datums encode. A Datum is technically only ever considered null if the accompanying `is_null` argument tells us so. It so happens that if the Datum type is pass-by-reference, and its value is zero, then it's pointing to a NULL, but Postgres doesn't even guarantee us that that is the nullness indicator. Again, only the `is_null` argument does that. As such, when converting a Datum into an Option<Datum>, only listen to the arguments and don't try to infer anything from the input datum's underlying value. This PR includes a few simple tests that are now green with the change to `from.rs`. Thanks to @sumerman for catching this!
The crux of the issue here is that `pgx_sys::Datum::from_datum(d, is_null)` can't ask the input Datum if it thinks it's null (via Datum::is_null(&self)), as nullness isn't a concept Datums encode. A Datum is technically only ever considered null if the accompanying `is_null` argument tells us so. It so happens that if the Datum type is pass-by-reference, and its value is zero, then it's pointing to a NULL, but Postgres doesn't even guarantee us that that is the nullness indicator. Again, only the `is_null` argument does that. As such, when converting a Datum into an Option<Datum>, only listen to the arguments and don't try to infer anything from the input datum's underlying value. This PR includes a few simple tests that are now green with the change to `from.rs`. Thanks to @sumerman for catching this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but I do feel like Datum is a bit of a footgun as it is. Oh well, this seems like a strict improvement on that front.
That check was added because the prior state of affairs was causing segfaults. |
This specific part of it was probably incorrect, however. |
The crux of the issue here is that
pgx_sys::Datum::from_datum(d, is_null)
can't ask the input Datum if it thinks it's null (viaDatum::is_null(&self)
), as nullness isn't a concept Datums encode.A Datum is technically only ever considered null if the accompanying
is_null
argument tells us so.It so happens that if the
Datum
type is pass-by-reference, and its value is zero, then it's pointing to a NULL, but Postgres doesn't even guarantee us that that is the nullness indicator. Again, only theis_null
argument does that.As such, when converting a
Datum
into anOption<Datum>
, only listen to the arguments and don't try to infer anything from the input datum's underlying value.This PR includes a few simple tests that are now green with the change to
from.rs
.Thanks to @sumerman for catching this!