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

32-bit support #231

Closed
anuraaga opened this issue Dec 27, 2023 · 1 comment · Fixed by #232
Closed

32-bit support #231

anuraaga opened this issue Dec 27, 2023 · 1 comment · Fixed by #232

Comments

@anuraaga
Copy link
Contributor

When building for Wasm in go-pgquery, I hadn't noticed the values in pg_config.h that are hard-coded for 64-bit architectures (Wasm is 32-bit).

https://github.com/pganalyze/libpg_query/blob/16-latest/src/postgres/include/pg_config.h#L680

With PG 15, this didn't seem to cause any issues, but when updating to PG 16 I found compilation errors relating to sizeof(DATUM) being the same as sizeof(int) which makes sense given that configuration. So I patched the config with various values for a 32-bit architecture

https://github.com/wasilibs/go-pgquery/blob/main/buildtools/wasm/patch.txt#L67

I was a bit surprised to see I had to implement pg_popcount32 - though IIUC, the source import process effectively compiles the code, meaning that only code referenced from the config end up in this repo, so 32-bit codepaths get erased.

I was wondering if it wouldn't be possible to include at least pg_popcount32 in the source? That allows only needing to do the config change for 32-bit, without actual business logic changes.

If it's not a priority to support 32-bit, no worries just wanted to check, and also leave a record for anyone else compiling to Wasm.

@lfittl lfittl mentioned this issue Dec 30, 2023
@lfittl
Copy link
Member

lfittl commented Dec 30, 2023

I was wondering if it wouldn't be possible to include at least pg_popcount32 in the source? That allows only needing to do the config change for 32-bit, without actual business logic changes.

Thanks for raising this!

I had forgotten that WASM uses 32-bit, which may be an argument to pay at least a bit more attention to it.

See #232 -- I think that should make things work (at least it does for me on a regular emulated 32-bit container running the latest Debian).

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 a pull request may close this issue.

2 participants