remove dlopen hack #1468

usamoi · 2024-01-08T16:04:44Z

Resolves #1369

Motivation of this PR:

no need to compile cargo-pgrx after upgrading compiler
make it possible to support binstall

This PR:

recompilation occurs even if we use a nightly toolchain.
schema is generated by cargo run --bin pgrx_embed_${ext_name} , instead of using libloading crate (unused symbols are removed by linker, so it compiles)
removes compiler version check
removes codes about stub
removes unnecessary metadata traits

This is a breaking change: users need to write pgrx::pgrx_embed!(); to src/pgrx_embed.rs and add that [[bin]] to the Cargo.toml, otherwise schema generation will report an error.

eeeebbbbrrrr · 2024-01-08T16:23:45Z

This is... this is exciting!

workingjubilee · 2024-01-08T20:31:50Z

find all entities using ctor, instead of using object crate (macro_magic does not support items that are not accessible)

Curious, why do we want to support non-accessible items?

workingjubilee · 2024-01-08T23:58:52Z

Using linker sections again would be annoying, but could be plausible.

However:

Rust's philosophy is that nothing happens before or after main and this library explicitly subverts that. The code that runs in the ctor and dtor functions should be careful to limit itself to libc functions and code that does not rely on Rust's stdlib services.

This sounds like it will eventually prove to be an unreasonable expectation, if it is not one already?

workingjubilee · 2024-01-09T00:12:12Z

Does this support cross-compilation?

cargo-pgrx/src/templates/cargo_toml

workingjubilee · 2024-01-09T00:53:46Z

Curious, why do we want to support non-accessible items?

To be clearer: when pgrx performs code expansion, even if the programmer doesn't put pub on their function, their function is linked into a publicly-accessible wrapper. The fact that we accept non-pub functions is mostly a relic of implementation details, they are still publicly-accessible in a fairly meaningful sense.

workingjubilee · 2024-01-09T02:57:10Z

Does this support cross-compilation?

I suppose schema gen has to happen on the build host anyways, so we're hoping programmers don't do anything silly like combining pg_extern with cfg(target_...)...

usamoi · 2024-01-09T07:31:49Z

Does this support cross-compilation?

I suppose schema gen has to happen on the build host anyways, so we're hoping programmers don't do anything silly like combining pg_extern with cfg(target_...)...

They can take advantages of qemu. We had the idea in #981 (comment)

usamoi · 2024-01-09T09:38:48Z

We can use cargo rustc --lib -- --Zunpretty=expanded and syn to get all #[no_mangle] extern "Rust" fn __pgrx_internals, do some codegen for binary schema in cargo-pgrx and run binary schema. Is it an acceptable solution? Non-accessible and private items can be accessed (my code marks many modules containing pg_extern private and I do not want to break my code) and ctor is removed.

workingjubilee · 2024-01-09T17:49:58Z

We can use cargo rustc --lib -- --Zunpretty=expanded and syn to get all #[no_mangle] extern "Rust" fn __pgrx_internals, do some codegen for binary schema in cargo-pgrx and run binary schema. Is it an acceptable solution? Non-accessible and private items can be accessed (my code marks many modules containing pg_extern private and I do not want to break my code) and ctor is removed.

Yes, I think that sounds acceptable. My main concern was that anything based on linker sections is historically prone to dying when it slams into the rocks of platform-specific nonsense. So we simply don't have sufficient contributors to reasonably work through that and fix while also maintaining the other parts of pgrx, and they always cause issues in ways that are hard to even identify as a problem with the platform-specific thing.

I understand the concern about breakage. 0.12 is likely to have quite enough to fix to update to as-is, because it will land fixes for lifetime issues, so I agree we don't want to have even more breakage in this cycle.

usamoi · 2024-01-10T05:02:00Z

CI passed and I will continue to finish unfinished work.

workingjubilee · 2024-01-10T19:49:56Z

This looks very interesting!

I discussed this briefly with the other maintainers. Another note against the ctor crate: @thomcc mentions that rustc has problems with... accidentally discarding ctors. Only #[no_mangle] pub extern "C" fn can really be trusted to land in the binary. Hypothetically these ctor issues are bugs and rustc should have fixed them by now, but we feel uncertain that there aren't more bugs underneath, and they seem like good to avoid.

A concern of build performance regression was brought up, but at the same time, we don't really feel a regression that isn't "literally doubling build time" is worth blocking this over: pgrx has a bunch of low-hanging fruit on build time, and having a much less cursed build setup is worth sacrificing at least some performance.

pgrx-examples/aggregate/embed.rs

usamoi · 2024-01-11T01:15:44Z

~~blocked by rust-lang/cargo#13280~~ use cfg as a workaround

workingjubilee · 2024-01-11T08:08:56Z

Hmm. That currently requires a build.rs in order to work, for rerun-if-env-changed. Is there a chance we could use something like cfg options?

usamoi · 2024-01-11T09:51:21Z

cargo expand causes recompliation even if we use a nightly toolchain. Also using cargo expand is just moving from a trick to another trick since this is unstable. So I just add object crate back.

workingjubilee · 2024-01-18T23:21:06Z

I opened rust-lang/cargo#13312 and got some interesting feedback: using the same bin name for all of these can definitely cause problems.

thomcc · 2024-01-27T17:54:10Z

Just to check, we no longer use ctor right? (It won't work because rustc doesn't compile things as whole-archive, so it would lead to silently dropped exports in large crates).

usamoi · 2024-01-27T18:06:54Z

Just to check, we no longer use ctor right? (It won't work because rustc doesn't compile things as whole-archive, so it would lead to silently dropped exports in large crates).

Yes, we no longer use ctor.

workingjubilee · 2024-01-27T23:31:38Z

It looks like my todo tests were a bad idea in actuality, since they have many tiny spurious failures, so the current CI status doesn't reflect on this PR. I will PR an ignore for them (on Monday, probably), if nobody beats me to it. Handled, it was actually just one test that was prone to spuriousness.

pgrx-sql-entity-graph/src/extension_sql/mod.rs

pgrx/src/lib.rs

cargo-pgrx/src/command/schema.rs

pgrx-sql-entity-graph/src/used_type.rs

Signed-off-by: usamoi <[email protected]>

usamoi · 2024-02-07T10:29:02Z

pgrx_embed is renamed to pgrx_embed_{name} now.

workingjubilee

re: 5566cea yeah that can get a bit long...

I might take a look later at adjusting the exact way that is handled before 0.12.0 actual... We may want to use the package metadata table to record a name, for future evolution purposes, and use that to make it more concise. It also would let people to rearrange their repo how they want, assigning whatever binary and file names they want, without confusing the pgrx tooling.

That's not a blocking concern for this, however. This looks great and a new extension passes cargo pgrx test all after doing the path hack and rerunning cargo pgrx init, which means that all we need to make this reusable is to publish it. This opens up so many possibilities! Thank you.

eeeebbbbrrrr · 2024-02-07T20:58:50Z

@usamoi: Thanks for all your work here. You too, of course, @workingjubilee.

It's really special that someone from the community helped solve this problem. Much appreciation and respect!

I noticed #1468 would allow us to also remove a dependency or two from cargo-pgrx, so I started work by removing once_cell from it and a few other spots in our tree, although we use it so pervasively it's not realistic to remove it entirely. It was actually secretly in our public API, behind a `#[doc(hidden)]`, for some reason. I then noticed a bunch of other easy removals from cargo-pgrx: - serde_derive should not be used directly - flate2, prettyplease, syn, and unescape are not directly used anymore - nix crate maintenance has gotten better, but it is a big dep for 1 bool

@usamoi

Welcome to pgrx 0.12.0-alpha.1! Say the magic words with me! ```shell cargo install cargo-pgrx --locked --version 0.12.0-alpha.1 ``` # Breaking Changes ## No more dlopen! Perhaps the most exciting change this round is @usamoi's contribution in #1468 which means that we no longer perform a `dlopen` in order to generate the schema. The cost, such as it is, is that your pgrx extensions now require a `src/bin/pgrx_embed.rs`, which will be used to generate the schema. This has much less cross-platform issues and will enable supporting things like `cargo binstall` down the line. It may be a bit touchy on first-time setup for transitioning older repos. If necessary, you may have to directly add a `src/bin/pgrx_embed.rs` and add the following code (which should be the only code in the file, though you can add comments if you like?): ```rust ::pgrx::pgrx_embed!(); ``` Your Cargo.toml will also want to update its crate-type key for the library: ```toml [lib] crate-type = ["cdylib", "lib"] ``` ## Library Code - pgrx-pg-sys will now use `ManuallyDropUnion` thanks to @NotGyro in #1547 - VARHDRSZ `const`s are no longer `fn`, thanks to @workingjubilee in #1584 - We no longer have `Interval::is_finite` since #1594 - We translate more `*_tree_walker` functions to the same signature their `*_impl` version in Postgres 16 has: #1596 - Thanks to @eeeebbbbrrrr in #1591 we no longer have the `pg_sql_graph_magic!()` macro, which should help with more things in the future! # What's New We have quite a lot of useful additions to our API: - `SpiClient::prepare_mut` was added thanks to @XeniaLu in #1275 - @usamoi also contributed bindings subscripting code in #1562 - For `#[pg_test]`, you have been able to use `#[should_panic(expected = "string")]` to anticipate a panic that contains that string in that test. For various reasons, `#[pg_test(error = "string")]` is much the same. Now, you can also use `#[pg_test(expected = "string")]`, in the hopes that is easier to stumble across, as of #1570 ## `Result<composite_type!("..."), E>` support - In #1560 @NotGyro contributed support for using `Result<composite_type!("Name"), E>`, as a case that had not been handled before. ## Significantly expanded docs Thanks to @rjuju, @NotGyro, and @workingjubilee, we now have significantly expanded docs for cargo-pgrx and pgrx in general. Some of these are in the API docs on https://docs.rs or the READMEs, but there's also a guide, now! It's not currently published, but is available as an [mdbook](https://github.com/rust-lang/mdBook) in the repo. Some diagnostic information that is also arguably documentation, like comments and the suggestion to `cargo install`, have also been improved, thanks to @workingjubilee in - #1579 - #1573 ## `#[pg_cast]` An experimental macro for a `CREATE CAST` was contributed by @xwkuang5 in #1445! ## Legal Stuff Thanks to @the-kenny in #1490 and @workingjubilee in #1504, it was brought to our attention that some dependencies had unusual legal requirements. So we fixed this with CI! We now check our code included into pgrx-using binaries is MIT/Apache 2.0 licensed, as is common across crates.io, using `cargo deny`!. The build tools will have more flexible legal requirements (partly due to the use of Mozilla Public License code in rustls). # Internal Changes Many internal cleanups were done thanks to - @workingjubilee in too many PRs to count! - @thomcc found a needless condition in #1501 - @nyurik in too many PRs to count! In particular: - we now actually `pfree` our `Array`s we detoasted as-of #1571 - creating a `RawArray` is now low-overhead due to #1587 ## Soundness Fixes We had a number of soundness issues uncovered or have added more tests to catch them. - Bounds-checking debug assertions for array access by @NotGyro in #1514 - Fix unsound `&` and `&mut` in `fcinfo.rs` by @workingjubilee in #1595 ## Less Deps Part of the cleanup by @workingjubilee was reducing the number of deps we compile: * cargo-pgrx: reduce trivial dep usages in #1499 * Update 2 syn in #1557 Hopefully it will reduce compile time and disk usage! ## New Contributors * @the-kenny made their first contribution in #1490 * @xwkuang5 made their first contribution in #1445 * @rjuju made their first contribution in #1516 * @nyurik made their first contribution in #1533 * @NotGyro made their first contribution in #1514 * @XeniaLu made their first contribution in #1275 **Full Changelog**: v0.12.0-alpha.0...v0.12.0-alpha.1

Resolves pgcentralfoundation#1369 Motivation of this PR: * no need to compile `cargo-pgrx` after upgrading compiler * make it possible to support `binstall` This PR: * recompilation occurs even if we use a nightly toolchain * schema is generated by `cargo run --bin pgrx_embed_${ext_name}` , instead of using `libloading` crate (unused symbols are removed by linker, so it compiles) * removes compiler version check * removes codes about stub * removes unnecessary metadata-related traits This is a breaking change: users need to write `pgrx::pgrx_embed!();` to `src/pgrx_embed.rs` and add that `[[bin]]` to the `Cargo.toml`, otherwise schema generation will report an error. --------- Signed-off-by: usamoi <[email protected]>

I noticed pgcentralfoundation#1468 would allow us to also remove a dependency or two from cargo-pgrx, so I started work by removing once_cell from it and a few other spots in our tree, although we use it so pervasively it's not realistic to remove it entirely. It was actually secretly in our public API, behind a `#[doc(hidden)]`, for some reason. I then noticed a bunch of other easy removals from cargo-pgrx: - serde_derive should not be used directly - flate2, prettyplease, syn, and unescape are not directly used anymore - nix crate maintenance has gotten better, but it is a big dep for 1 bool

@usamoi

Welcome to pgrx 0.12.0-alpha.1! Say the magic words with me! ```shell cargo install cargo-pgrx --locked --version 0.12.0-alpha.1 ``` # Breaking Changes ## No more dlopen! Perhaps the most exciting change this round is @usamoi's contribution in pgcentralfoundation#1468 which means that we no longer perform a `dlopen` in order to generate the schema. The cost, such as it is, is that your pgrx extensions now require a `src/bin/pgrx_embed.rs`, which will be used to generate the schema. This has much less cross-platform issues and will enable supporting things like `cargo binstall` down the line. It may be a bit touchy on first-time setup for transitioning older repos. If necessary, you may have to directly add a `src/bin/pgrx_embed.rs` and add the following code (which should be the only code in the file, though you can add comments if you like?): ```rust ::pgrx::pgrx_embed!(); ``` Your Cargo.toml will also want to update its crate-type key for the library: ```toml [lib] crate-type = ["cdylib", "lib"] ``` ## Library Code - pgrx-pg-sys will now use `ManuallyDropUnion` thanks to @NotGyro in pgcentralfoundation#1547 - VARHDRSZ `const`s are no longer `fn`, thanks to @workingjubilee in pgcentralfoundation#1584 - We no longer have `Interval::is_finite` since pgcentralfoundation#1594 - We translate more `*_tree_walker` functions to the same signature their `*_impl` version in Postgres 16 has: pgcentralfoundation#1596 - Thanks to @eeeebbbbrrrr in pgcentralfoundation#1591 we no longer have the `pg_sql_graph_magic!()` macro, which should help with more things in the future! # What's New We have quite a lot of useful additions to our API: - `SpiClient::prepare_mut` was added thanks to @XeniaLu in pgcentralfoundation#1275 - @usamoi also contributed bindings subscripting code in pgcentralfoundation#1562 - For `#[pg_test]`, you have been able to use `#[should_panic(expected = "string")]` to anticipate a panic that contains that string in that test. For various reasons, `#[pg_test(error = "string")]` is much the same. Now, you can also use `#[pg_test(expected = "string")]`, in the hopes that is easier to stumble across, as of pgcentralfoundation#1570 ## `Result<composite_type!("..."), E>` support - In pgcentralfoundation#1560 @NotGyro contributed support for using `Result<composite_type!("Name"), E>`, as a case that had not been handled before. ## Significantly expanded docs Thanks to @rjuju, @NotGyro, and @workingjubilee, we now have significantly expanded docs for cargo-pgrx and pgrx in general. Some of these are in the API docs on https://docs.rs or the READMEs, but there's also a guide, now! It's not currently published, but is available as an [mdbook](https://github.com/rust-lang/mdBook) in the repo. Some diagnostic information that is also arguably documentation, like comments and the suggestion to `cargo install`, have also been improved, thanks to @workingjubilee in - pgcentralfoundation#1579 - pgcentralfoundation#1573 ## `#[pg_cast]` An experimental macro for a `CREATE CAST` was contributed by @xwkuang5 in pgcentralfoundation#1445! ## Legal Stuff Thanks to @the-kenny in pgcentralfoundation#1490 and @workingjubilee in pgcentralfoundation#1504, it was brought to our attention that some dependencies had unusual legal requirements. So we fixed this with CI! We now check our code included into pgrx-using binaries is MIT/Apache 2.0 licensed, as is common across crates.io, using `cargo deny`!. The build tools will have more flexible legal requirements (partly due to the use of Mozilla Public License code in rustls). # Internal Changes Many internal cleanups were done thanks to - @workingjubilee in too many PRs to count! - @thomcc found a needless condition in pgcentralfoundation#1501 - @nyurik in too many PRs to count! In particular: - we now actually `pfree` our `Array`s we detoasted as-of pgcentralfoundation#1571 - creating a `RawArray` is now low-overhead due to pgcentralfoundation#1587 ## Soundness Fixes We had a number of soundness issues uncovered or have added more tests to catch them. - Bounds-checking debug assertions for array access by @NotGyro in pgcentralfoundation#1514 - Fix unsound `&` and `&mut` in `fcinfo.rs` by @workingjubilee in pgcentralfoundation#1595 ## Less Deps Part of the cleanup by @workingjubilee was reducing the number of deps we compile: * cargo-pgrx: reduce trivial dep usages in pgcentralfoundation#1499 * Update 2 syn in pgcentralfoundation#1557 Hopefully it will reduce compile time and disk usage! ## New Contributors * @the-kenny made their first contribution in pgcentralfoundation#1490 * @xwkuang5 made their first contribution in pgcentralfoundation#1445 * @rjuju made their first contribution in pgcentralfoundation#1516 * @nyurik made their first contribution in pgcentralfoundation#1533 * @NotGyro made their first contribution in pgcentralfoundation#1514 * @XeniaLu made their first contribution in pgcentralfoundation#1275 **Full Changelog**: pgcentralfoundation/pgrx@v0.12.0-alpha.0...v0.12.0-alpha.1

usamoi marked this pull request as draft January 8, 2024 16:07

usamoi force-pushed the nohack branch 3 times, most recently from 216303e to 9c2b706 Compare January 8, 2024 17:46

workingjubilee reviewed Jan 9, 2024

View reviewed changes

cargo-pgrx/src/templates/cargo_toml Show resolved Hide resolved

workingjubilee mentioned this pull request Jan 9, 2024

Deprecate pg_extern on non-pub fn? #1471

Open

usamoi force-pushed the nohack branch 5 times, most recently from 11255f3 to f214499 Compare January 10, 2024 04:24

usamoi marked this pull request as ready for review January 10, 2024 04:51

workingjubilee reviewed Jan 10, 2024

View reviewed changes

pgrx-examples/aggregate/embed.rs Outdated Show resolved Hide resolved

usamoi force-pushed the nohack branch 2 times, most recently from ff58e46 to cb893fc Compare January 11, 2024 09:48

usamoi force-pushed the nohack branch from cb893fc to 9da17f5 Compare January 11, 2024 10:05

usamoi force-pushed the nohack branch 2 times, most recently from d11dbbc to d4fbdad Compare January 27, 2024 08:54

workingjubilee reviewed Jan 27, 2024

View reviewed changes

pgrx-sql-entity-graph/src/extension_sql/mod.rs Show resolved Hide resolved

pgrx/src/lib.rs Outdated Show resolved Hide resolved

cargo-pgrx/src/command/schema.rs Outdated Show resolved Hide resolved

pgrx-sql-entity-graph/src/used_type.rs Show resolved Hide resolved

workingjubilee mentioned this pull request Jan 29, 2024

Remove flaky serde_serialize_array todo test #1503

Merged

workingjubilee closed this Jan 29, 2024

workingjubilee reopened this Jan 29, 2024

This was referenced Jan 30, 2024

cargo-pgrx: reduce trivial dep usages #1499

Merged

Add full SQL-gen tests #1509

Open

remove dlopen hack

c10fa1a

Signed-off-by: usamoi <[email protected]>

usamoi force-pushed the nohack branch from d4fbdad to c10fa1a Compare February 7, 2024 09:51

reduce bin filename length

5566cea

Signed-off-by: usamoi <[email protected]>

workingjubilee approved these changes Feb 7, 2024

View reviewed changes

workingjubilee added the breaks-API label Feb 7, 2024

workingjubilee merged commit 03e97ee into pgcentralfoundation:develop Feb 7, 2024
8 checks passed

workingjubilee mentioned this pull request Feb 7, 2024

chore: lots of lints and fixes #1523

Merged

NotGyro mentioned this pull request Feb 21, 2024

Use shlex whenever we touch shell-argument-shaped-things directly #1455

Closed

workingjubilee mentioned this pull request Mar 1, 2024

Release pgrx 0.12.0-alpha.1 #1598

Merged

Weijun-H mentioned this pull request Aug 21, 2024

feat: Upgrade pgrx to 0.12.1 paradedb/paradedb#1550

Merged

workingjubilee mentioned this pull request Sep 3, 2024

Compilation speed #1835

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove dlopen hack #1468

remove dlopen hack #1468

usamoi commented Jan 8, 2024 •

edited by workingjubilee

Loading

eeeebbbbrrrr commented Jan 8, 2024

workingjubilee commented Jan 8, 2024

workingjubilee commented Jan 8, 2024 •

edited

Loading

workingjubilee commented Jan 9, 2024

workingjubilee commented Jan 9, 2024 •

edited

Loading

workingjubilee commented Jan 9, 2024

usamoi commented Jan 9, 2024

usamoi commented Jan 9, 2024

workingjubilee commented Jan 9, 2024 •

edited

Loading

usamoi commented Jan 10, 2024

workingjubilee commented Jan 10, 2024 •

edited

Loading

usamoi commented Jan 11, 2024 •

edited

Loading

workingjubilee commented Jan 11, 2024 •

edited

Loading

usamoi commented Jan 11, 2024 •

edited

Loading

workingjubilee commented Jan 18, 2024

thomcc commented Jan 27, 2024

usamoi commented Jan 27, 2024 •

edited

Loading

workingjubilee commented Jan 27, 2024 •

edited

Loading

usamoi commented Feb 7, 2024

workingjubilee left a comment

eeeebbbbrrrr commented Feb 7, 2024

remove dlopen hack #1468

remove dlopen hack #1468

Conversation

usamoi commented Jan 8, 2024 • edited by workingjubilee Loading

eeeebbbbrrrr commented Jan 8, 2024

workingjubilee commented Jan 8, 2024

workingjubilee commented Jan 8, 2024 • edited Loading

workingjubilee commented Jan 9, 2024

workingjubilee commented Jan 9, 2024 • edited Loading

workingjubilee commented Jan 9, 2024

usamoi commented Jan 9, 2024

usamoi commented Jan 9, 2024

workingjubilee commented Jan 9, 2024 • edited Loading

usamoi commented Jan 10, 2024

workingjubilee commented Jan 10, 2024 • edited Loading

usamoi commented Jan 11, 2024 • edited Loading

workingjubilee commented Jan 11, 2024 • edited Loading

usamoi commented Jan 11, 2024 • edited Loading

workingjubilee commented Jan 18, 2024

thomcc commented Jan 27, 2024

usamoi commented Jan 27, 2024 • edited Loading

workingjubilee commented Jan 27, 2024 • edited Loading

usamoi commented Feb 7, 2024

workingjubilee left a comment

Choose a reason for hiding this comment

eeeebbbbrrrr commented Feb 7, 2024

usamoi commented Jan 8, 2024 •

edited by workingjubilee

Loading

workingjubilee commented Jan 8, 2024 •

edited

Loading

workingjubilee commented Jan 9, 2024 •

edited

Loading

workingjubilee commented Jan 9, 2024 •

edited

Loading

workingjubilee commented Jan 10, 2024 •

edited

Loading

usamoi commented Jan 11, 2024 •

edited

Loading

workingjubilee commented Jan 11, 2024 •

edited

Loading

usamoi commented Jan 11, 2024 •

edited

Loading

usamoi commented Jan 27, 2024 •

edited

Loading

workingjubilee commented Jan 27, 2024 •

edited

Loading