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

Support extensions on i686-unknown-linux-gnu #1014

Merged
merged 2 commits into from
Jan 26, 2023

Conversation

syvb
Copy link
Contributor

@syvb syvb commented Jan 19, 2023

pgx assumes it's running on a 64-bit processor in a few places:

  • it assumes long and time_t are always an i64
  • it assumes doubles always have a different alignment than ints

This PR fixes pgx to work on 32-bit systems. I've verified that pgx successfully compiles on i686-unknown-linux-gnu with this.

@syvb syvb force-pushed the sv/i386-fixes branch 2 times, most recently from 3c511db to c33cd5a Compare January 19, 2023 19:28
@eeeebbbbrrrr
Copy link
Contributor

Do tests pass? We definitely want @workingjubilee to review this and think about it.

I'm concerned about future maintainability. I feel like we probably have other hidden assumptions in the code that the compiler won't pickup.

Where did the desire for 32bit support come from?

@syvb
Copy link
Contributor Author

syvb commented Jan 19, 2023

@eeeebbbbrrrr I'm trying to run a pgx extension on v86, an in-browser virtualized CPU (mostly for demonstration purposes).

I just ran the tests on a 32-bit Docker image and most of them passed, but the datetime tests failed, probably because there's an assumption that time_t is an i64 somewhere. I'll see if I can fix that.

It might be possible to an i386 Docker image to the will-it-blend nightly tests.

@eeeebbbbrrrr
Copy link
Contributor

At the end of the day I think'll merge this. I don't really see any reason not to. I just have some concern right now that if we say "pgx also runs in 32bit!" and we missed a few things that'd be embarrassing.

Also, because I just don't know any better, is there a better type than libc::c_long we can use?

@eeeebbbbrrrr
Copy link
Contributor

Also, because I just don't know any better, is there a better type than libc::c_long we can use?

Like, is isize the equivalent built-in Rust type?

@syvb
Copy link
Contributor Author

syvb commented Jan 19, 2023

I used libc::c_long for Postgres functions with an argument of type long. libc::c_long is a type alias to either i32 or i64 depending on the target triple, and bindgen maps longs in C to libc::c_long. The length of a C long is compiler-defined but in practice all compilers usually agree on the lengths of types for a given target triple. Generally on Unix systems, the length of a long is the same size as isize, but on Windows longs are always i32s (for historical reasons).

So for all platforms pgx supports, isize is the same as whatever libc::c_long is a type alias to, but still a different type since isize is a distinct type and not a type alias.

@syvb
Copy link
Contributor Author

syvb commented Jan 19, 2023

Also: I've done some more experimenting, and while you can get pgx extensions to compile and run with this, they are very prone to crashing and producing incorrect results. It's only really useful for further improving pgx's 32-bit support right now.

@eeeebbbbrrrr
Copy link
Contributor

It's only really useful for further improving pgx's 32-bit support right now.

That's kinda why I asked what prompted this. How invested are you (y'all) in the idea of pgx supporting 32bit? We're not opposed to it, by any means, but I don't see us doing much heavy lifting ourselves. We'll definitely help out where we can and merge and add things to CI and whatever.

@syvb
Copy link
Contributor Author

syvb commented Jan 20, 2023

I'm just doing some experimenting at this point - I'm not super invested in 32-bit support at this point.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also review the implementations in pgx-pg-sys/src/submodules/datum.rs for 32-bit correctness. I'm not too concerned if some of the conversions are actually secretly lossy so much as they should be checked for correctness due to sign-extension or zero-extension and then truncation that may happen differently.

Comment on lines +93 to +95
let secs_per_day: libc::time_t =
pg_sys::SECS_PER_DAY.try_into().expect("couldn't fit time into time_t");
libc::time_t::from(self.to_unix_epoch_days()) * secs_per_day
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, whether a system uses a 32-bit or 64-bit value for time_t is actually only incidentally historically related. Sometimes a 32-bit system will use a 64-bit time_t. Sometimes a 64-bit system will use a 32-bit time_t. Sometimes time_t is an unsigned integer. Sometimes it is a signed integer.

I don't think this actually affects this code's current correctness, it just felt worthy of note.

@workingjubilee
Copy link
Member

workingjubilee commented Jan 22, 2023

We may pick up some side benefits like supporting virtual wasm32 environments from using a 32-bit-correct implementation, and that may make future usages of pgx in virtual/sandboxed environments easier. We should invest no real effort to support the more general inadequacies of 32-bit hardware, like "boohoo the x87 FPU sucks". If that's the problem, then a user should emulate a real CPU with an actually useful floating-point unit. But the usize and isize nuances are embedded in the Rust language so we shouldn't find this to be a significant maintenance burden, especially if we maintain a "usize is at least 32 bits" assumption and prefer to use strict-provenance APIs instead of wildly casting integers to pointers and pointers to integers (and, afaik, we mostly do the right thing now). There are many problems introduced by assuming usize may be as small as 16 bits, and Rust allows for that possibility. By comparison, 32-bit correctness is slightly annoying but trivial.

Sometime in the next 20 years, if Postgres and pgx still exist in anything resembling their current form, being multiple-usize-correct would also make it easier to add "usize is u128" support. Some "big data" workloads already push against even Intel's 57-bit 5-level paging, and the murmurs of a "ZettaLinux" have begun.

@syvb
Copy link
Contributor Author

syvb commented Jan 26, 2023

I added a test to pgx-pg-sys/src/submodules/datum.rs (that passes on 32-bit and 64-bit machines) that verifies round-tripping integer values from a Datum works as I expect it to.

Trying to stick a i64/u64 in a Datum on a 32-bit machine will cause silent truncation, which might be unexpected. We could remove the From<*64> impls on 32-bit machines, although that might cause problems with user code assuming that it can put a *64 into a Datum.

@syvb syvb requested a review from workingjubilee January 26, 2023 02:06
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If your experiments with this bear more fruit (i.e. we get past "extensions mostly just crash on 32-bit") and we get some interesting results with them, or something like a wasm32-wasi Postgres or whatever, we might consider something like that, but I don't think that's something we currently want to do right now. It's just way too convenient to be able to shove u64 into a pg_sys::Datum since that's usually legal, and for many integers, it's not the end of the world if it truncates.

This PR looks good as-is, and since Eric also approves, I'm gonna merge this.

@workingjubilee workingjubilee merged commit 62468d7 into pgcentralfoundation:master Jan 26, 2023
@syvb syvb deleted the sv/i386-fixes branch January 26, 2023 12:31
workingjubilee pushed a commit that referenced this pull request Jan 26, 2023
* Support pgx extensions on i386 processors
* Add test for roundtripping datum
workingjubilee added a commit that referenced this pull request Jan 26, 2023
Support extensions on i686-unknown-linux-gnu (#1014)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants