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

Optionally emit versioned .so #546

Merged

Conversation

JamesGuthrie
Copy link
Contributor

@JamesGuthrie JamesGuthrie commented Apr 21, 2022

pgx now experimentally supports the option to produce a versioned
shared library. This option is activated by removing the
module_pathname parameter in the extension's .control file.

When activated, it does the following:

  • appends current package version to shared library name
  • points generated function SQL to the versioned shared library (instead
    of MODULE_PATHNAME)
  • replaces @MODULE_PATHNAME@ placeholder with the path to the versioned
    shared library

Background:

There is an implicit requirement that C extensions maintain ABI
compatibility between versions. Emitting a versioned .so allows for ABI
breaks between versions.

This functionality is experimental and that care must be taken in the
following scenarios:

  • when using shared memory
  • when using query planner hooks
  • when producing SQL schema migrations

Copy link
Contributor

@eeeebbbbrrrr eeeebbbbrrrr left a comment

Choose a reason for hiding this comment

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

I think overall this is pretty close.

Before merging, I'd like to see the cargo-pgx/README.md updated to reflect this new argument and detail what it does any why it might be necessary.

cargo-pgx/src/command/install.rs Outdated Show resolved Hide resolved
cargo-pgx/src/command/install.rs Outdated Show resolved Hide resolved
cargo-pgx/src/command/run.rs Outdated Show resolved Hide resolved
@JamesGuthrie
Copy link
Contributor Author

I think overall this is pretty close.

Before merging, I'd like to see the cargo-pgx/README.md updated to reflect this new argument and detail what it does any why it might be necessary.

I've updated the docs. I'm having difficulty seeing where somebody who is unfamiliar with these concepts will have difficulty, so if there's anything that you find unclear please let me know and I'll take another pass.

@JamesGuthrie JamesGuthrie force-pushed the jg/versioned-so branch 5 times, most recently from 3ae2ff9 to f62db52 Compare April 25, 2022 15:51
@JamesGuthrie JamesGuthrie changed the title Add option to emit versioned .so Optionally emit versioned .so Apr 25, 2022
@eeeebbbbrrrr
Copy link
Contributor

@JamesGuthrie this is super nice now. Great work and thanks for sticking through the process. When you address those three minor comments above this will be ready to merge.

Oh, please retarget the PR to the develop branch as well. We're weird and don't merge directly into master, unless you're me and are prepared to be chastised by @Hoverbear for doing it. ;)

@JamesGuthrie JamesGuthrie changed the base branch from master to develop April 25, 2022 16:05
@JamesGuthrie
Copy link
Contributor Author

@JamesGuthrie this is super nice now. Great work and thanks for sticking through the process. When you address those three minor comments above this will be ready to merge.

Oh, please retarget the PR to the develop branch as well. We're weird and don't merge directly into master, unless you're me and are prepared to be chastised by @Hoverbear for doing it. ;)

No problem, thanks for the quick review! I've fixed the three things above and retargeted against develop.

@eeeebbbbrrrr
Copy link
Contributor

Approved. Lets see what CI says and then I'll merge it.

And as promised, I'm happy to put out a v0.4.4 this week so y'all can get back to your real work.

cargo-pgx/README.md Outdated Show resolved Hide resolved
@eeeebbbbrrrr
Copy link
Contributor

eeeebbbbrrrr commented Apr 25, 2022

hmm, the new versioned_so example test failed: https://github.com/tcdi/pgx/runs/6161094219?check_suite_focus=true

Error message sucks. Did it work for you locally?

Copy link
Contributor

@Hoverbear Hoverbear left a comment

Choose a reason for hiding this comment

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

Some minor readme expansions requested, but otherwise I'm happy with how this turned out!

`pgx` now experimentally supports the option to produce a versioned
shared library. This option is activated by removing the
`module_pathname` parameter in the extension's `.control` file.

When activated, it does the following:

- appends current package version to shared library name
- points generated function SQL to the versioned shared library (instead
  of `MODULE_PATHNAME`)
- replaces `@MODULE_PATHNAME@` placeholder with the path to the versioned
  shared library

Background:

There is an implicit requirement that C extensions maintain ABI
compatibility between versions. Emitting a versioned .so allows for ABI
breaks between versions.

This functionality is experimental and that care must be taken in the
following scenarios:

- when using shared memory
- when using query planner hooks
- when producing SQL schema migrations
@JamesGuthrie
Copy link
Contributor Author

Okay, managed to get the build to work locally. Let's hope that CI accepts it.

@Hoverbear
Copy link
Contributor

This is great James. :)

@eeeebbbbrrrr eeeebbbbrrrr merged commit 6495933 into pgcentralfoundation:develop Apr 26, 2022
@eeeebbbbrrrr
Copy link
Contributor

Thanks a bunch, @JamesGuthrie! Merged. We'll get it out in a new release in the next few days.

@@ -0,0 +1,3 @@
[build]
# Postgres symbols won't be available until runtime
rustflags = ["-C", "link-args=-Wl,-undefined,dynamic_lookup"]
Copy link
Contributor

Choose a reason for hiding this comment

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

We didn't catch this during review, but this should have been removed as it causes some conflicts with the workspace override. @eeeebbbbrrrr is fixing this up before release.

Copy link
Contributor

Choose a reason for hiding this comment

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

yah. totally our bad.

In a workspace crate environment like pgx is, you only need the .cargo/config at the root workspace crate. If it's in any "child" crates, the compiler goes stupid and you spend all day trying to figure it out and then you're thankful @Hoverbear is on your team!

@eeeebbbbrrrr eeeebbbbrrrr mentioned this pull request May 8, 2022
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