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

Fix issue #1032 #1033

Merged
merged 8 commits into from
Feb 16, 2023
Merged

Fix issue #1032 #1033

merged 8 commits into from
Feb 16, 2023

Conversation

eeeebbbbrrrr
Copy link
Contributor

Blindly use the FMGR_ABI_EXTRA constant as part of the magic block. Hardcoding to "PostgreSQL" isn't exactly correct when using bindings from, for example, a Postgres fork that has purposely changed its ABI "name".

Blindly use the `FMGR_ABI_EXTRA` constant as part of the magic block.
Hardcoding to "PostgreSQL" isn't exactly correct when using bindings
from, for example, a Postgres fork that has purposely changed its ABI
"name".
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.

This violates the compile-time guarantees we have provided until now. That is: we have previously assumed the responsibility for making PGX work against Postgres.

We should not enable this change except conditionally on the user explicitly requesting this be overwritten at compile time, in a way that we can detect when an issue is opened against PGX. It should be the user's responsibility, not ours, for whether their extension works against a bespoke Postgres fork.

@eeeebbbbrrrr
Copy link
Contributor Author

This violates the compile-time guarantees we have provided until now. That is: we have previously assumed the responsibility for making PGX work against Postgres.

We should not enable this change except conditionally on the user explicitly requesting this be overwritten at compile time, in a way that we can detect when an issue is opened against PGX. It should be the user's responsibility, not ours, for whether their extension works against a bespoke Postgres fork.

What are you thinking here? --features this_isnt_postgres?

@workingjubilee
Copy link
Member

I think a feature name can use all of Unicode, so we could go with "unsafe-postgres-doppelgänger" so it's annoying to type.

@eeeebbbbrrrr
Copy link
Contributor Author

and do we raise a compilation error if it's not "PostgreSQL" and that feature doesn't exist?

Doing this pushes the burden of supporting Postgres forks onto pgx extension developers. Maybe that's where it belongs tho. We can't know everything.

@workingjubilee
Copy link
Member

Yes, I think so. We could also lift this checking into cargo pgx, effectively, and then it's the fault of whoever runs the compiler.

@eeeebbbbrrrr
Copy link
Contributor Author

Yes, I think so. We could also lift this checking into cargo pgx, effectively, and then it's the fault of whoever runs the compiler.

Well, one can compile a pgx extension without cargo-pgx, so I don't think that's super valuable. Lets go with a feature flag of --features unsafe-postgres and a compilation error without it.

I'll update the PR and we can see what it looks like.

…erent ABI than "PostgreSQL" and fail to compile. Unless the `--unsafe-postgres` feature flag is present
README.md Outdated Show resolved Hide resolved
@eeeebbbbrrrr
Copy link
Contributor Author

this isn't working out like I had hoped. #grr

pgx/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@thomcc thomcc left a 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 did write some of it 😅)

@eeeebbbbrrrr
Copy link
Contributor Author

You wrote all of it. In the discord input box!

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.

None yet

4 participants