Fixed conversion of i64 and f64 to datums on 32-bit machines #1859
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I will accept this if you also run down places like Line 569 in b810e98 |
|
Actually that specific place may be harmless even, but the point is that there's other locations that make assumptions about sizes, and I would rather not bother even with this amount of positive signaling unless we've made a cursory effort to check them. |
|
I realize "search for all instances of the number 8 and make sure they aren't obviously bad" is a lot of effort but... |
That's fine, it requires checking only the |
|
According to the PostgreSQL source code
That implies that on 32-bit architectures the following code will always fall onto Lines 108 to 122 in b810e98 Moreover, |
|
|
Cool. |
workingjubilee
requested changes
Sep 13, 2024
pgrx-pg-sys/src/submodules/datum.rs
Outdated
| if size_of::<u64>() <= size_of::<usize>() { | ||
| Datum::from(val as usize) | ||
| } else { | ||
| Datum::from(Box::into_raw(Box::new(val))) |
There was a problem hiding this comment.
This needs to be a palloc.
pgrx-pg-sys/src/submodules/datum.rs
Outdated
| if size_of::<i64>() <= size_of::<usize>() { | ||
| Datum::from(val as usize) | ||
| } else { | ||
| Datum::from(Box::into_raw(Box::new(val))) |
There was a problem hiding this comment.
This needs to be a palloc.
|
|
|
we probably have unit tests that don't happen inside |
workingjubilee
requested changes
Sep 14, 2024
pgrx-pg-sys/src/submodules/datum.rs
Outdated
| @@ -157,7 +161,11 @@ impl From<i32> for Datum { | |||
| impl From<i64> for Datum { | |||
| #[inline] | |||
| fn from(val: i64) -> Datum { | |||
| Datum::from(val as usize) | |||
| if size_of::<i64>() <= size_of::<usize>() { | |||
There was a problem hiding this comment.
Suggested change
| if size_of::<i64>() <= size_of::<usize>() { | |
| if size_of::<i64>() <= size_of::<Datum>() { |
pgrx-pg-sys/src/submodules/datum.rs
Outdated
| @@ -129,7 +129,11 @@ impl From<u32> for Datum { | |||
| impl From<u64> for Datum { | |||
| #[inline] | |||
| fn from(val: u64) -> Datum { | |||
| Datum::from(val as usize) | |||
| if size_of::<u64>() <= size_of::<usize>() { | |||
There was a problem hiding this comment.
Suggested change
| if size_of::<u64>() <= size_of::<usize>() { | |
| if size_of::<u64>() <= size_of::<Datum>() { |
pgrx/src/datum/from.rs
Outdated
| @@ -283,7 +283,12 @@ impl FromDatum for i64 { | |||
| if is_null { | |||
| None | |||
| } else { | |||
| Some(datum.value() as _) | |||
| let value = if size_of::<i64>() <= size_of::<usize>() { | |||
There was a problem hiding this comment.
Suggested change
| let value = if size_of::<i64>() <= size_of::<usize>() { | |
| let value = if size_of::<i64>() <= size_of::<pg_sys::Datum>() { |
pgrx/src/datum/from.rs
Outdated
| @@ -315,7 +320,12 @@ impl FromDatum for f64 { | |||
| if is_null { | |||
| None | |||
| } else { | |||
| Some(f64::from_bits(datum.value() as _)) | |||
| let value = if size_of::<i64>() <= size_of::<usize>() { | |||
There was a problem hiding this comment.
Suggested change
| let value = if size_of::<i64>() <= size_of::<usize>() { | |
| let value = if size_of::<f64>() <= size_of::<pg_sys::Datum>() { |
|
The |
|
sigh. |
|
Either the tests need to be moved out into pgrx-tests in a |
|
Decided to go with |
|
This seems viable-enough for now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No tests in CI at the moment because GitHub Actions are .NET based which: actions/runner/issues/1181. May spend some time later on that, but for now let's keep the warning about official support of x64 only.