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

cargo-pgx should verify compiler parity requirement #774

Closed
syvb opened this issue Oct 17, 2022 · 6 comments · Fixed by #873
Closed

cargo-pgx should verify compiler parity requirement #774

syvb opened this issue Oct 17, 2022 · 6 comments · Fixed by #873
Labels
cargo pgrx enhancement New feature or request

Comments

@syvb
Copy link
Contributor

syvb commented Oct 17, 2022

pgx requires that the compiler used to compile cargo-pgx is the same compiler used to compile extension crates (#687 (comment)), but doesn't verify that requirement in cargo-pgx. It should.

This requirement is not made obvious to users and results in user confusion since a compiler mismatch can result in weird segfaults and errors.

@workingjubilee
Copy link
Member

We could have the Rust compiler version get injected into cargo-pgx via a build.rs or something like that.

@thomcc
Copy link
Contributor

thomcc commented Oct 18, 2022

Do we need this except for the ability to return eyre::Result from that function? It might be better to just fix that requirement by returning something FFI safe.

@syvb
Copy link
Contributor Author

syvb commented Oct 18, 2022

@thomcc pgx has multiple extern "Rust" functions (mostly __pgx_internals_*) that return repr(Rust) types. SqlGraphEntity is one of those repr(Rust) types returned from all of the __pgx_internals_* functions and would be quite difficult to make FFI-safe due to its complexity.

Fixing this properly (to remove all of the unsoundness from cargo-pgx) would probably require serializing everything (to JSON or similar) that extension binaries return to cargo-pgx, so that instead of:
https://github.com/tcdi/pgx/blob/336217cc46216a7682916dfa788860bad363ae96/cargo-pgx/src/command/schema.rs#L396

it does something like

 let symbol: libloading::os::unix::Symbol<unsafe extern "C" fn() -> *const u8> = 

and deserializes the returned *const u8 (as a null-terminated string) to a SqlGraphEntity.

@thomcc
Copy link
Contributor

thomcc commented Oct 18, 2022

Ah, I see. Yeah, nevermind then.

@thomcc
Copy link
Contributor

thomcc commented Oct 18, 2022

We do generate a lot of code so we could probably do better than JSON, but still, a problem for another time.

@workingjubilee workingjubilee added enhancement New feature or request cargo pgrx labels Oct 26, 2022
@thomcc
Copy link
Contributor

thomcc commented Nov 1, 2022

FWIW, I think actually JSON would be fine here since this is only used in cargo-pgx, IIUC. I'll probably change to this soon since I've hit it many times recently, since I use nightly rust locally, and there have been changes to struct layout between stable and nightly, which caused me headache when debugging an issue that seemed to only appear on stable (it was actually just that the bug was non-deterministic).

That said, a better approach is probably to store this data in a custom #[link_section], rather than returning it from a function, and then read it out in cargo-pgx (for example, with the object crate). Then we don't have to dlopen it, which will simplify the schema subcommand, and avoid the need for some crimes I'm going to have to do to fix #731. There are also performance benefits for packaged extensions (reduces overhead from postgres forking)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cargo pgrx enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants