Skip to content

Conversation

@thesuhas
Copy link
Contributor

@thesuhas thesuhas commented May 19, 2025

Exposing the GUC hooks to insert extern C code that will execute before assigning a GUC/while assigning a GUC/show of GUC.

@thesuhas thesuhas changed the title Expose guc assign hook Expose guc hooks May 19, 2025
@thesuhas thesuhas force-pushed the expose_guc_assign_hook branch from 8e7d35a to 9b79a73 Compare May 19, 2025 21:58
@thesuhas thesuhas marked this pull request as ready for review May 20, 2025 15:15
@thesuhas
Copy link
Contributor Author

@charmitro can I get a review on this whenever you get a chance?

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'll merge soon as CI passes (which I'm approving a run for now).

@usamoi
Copy link
Contributor

usamoi commented May 31, 2025

unsafe extern "C-unwind" fn f(_: *mut bool, _: *mut *mut c_void, _: u32) -> bool {
    panic!()
}
GucRegistry::define_bool_guc_with_hooks(..., f)

should cause undefined behavior, since Rust panic unwinds without being caught?

@eeeebbbbrrrr
Copy link
Contributor

eeeebbbbrrrr commented May 31, 2025

Looks like CI is unhappy, there's a conflict, and there's some review suggestions here that ought to be considered.

I'm not sure of the best way to handle @usamoi's point here: #2075 (comment)

It would be the programmer's responsibility to add a #[pg_guard] on that function -- that doesn't seem unreasonable. Maybe these new xxx_with_hooks() functions should all be declared unsafe with a "# Safety" note on each about this.

Copy link
Contributor

@charmitro charmitro left a comment

Choose a reason for hiding this comment

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

What do you think about a test case? As simple as it can be.

  1. Check hooks being called
  2. Assign hooks performing side effects
  3. Show hooks providing custom display output

@thesuhas
Copy link
Contributor Author

thesuhas commented Jun 3, 2025

Looks like CI is unhappy, there's a conflict, and there's some review suggestions here that ought to be considered.

I'm not sure of the best way to handle @usamoi's point here: #2075 (comment)

It would be the programmer's responsibility to add a #[pg_guard] on that function -- that doesn't seem unreasonable. Maybe these new xxx_with_hooks() functions should all be declared unsafe with a "# Safety" note on each about this.

Done! But won't all GUC registration calls with hooks have to be in unsafe blocks/functions?

@thesuhas
Copy link
Contributor Author

thesuhas commented Jun 3, 2025

I'll merge soon as CI passes (which I'm approving a run for now).

Can you approve another run?

@eeeebbbbrrrr
Copy link
Contributor

But won't all GUC registration calls with hooks have to be in unsafe blocks/functions?

yes, because they fundamentally are unsafe.

We can’t make them safe because the functions can’t guarantee the user declares their hook function correctly.

There’s nothing wrong with unsafe, but there is something wrong with not declaring something unsafe that actually is.

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.

Thank you for your work! Soon as CI passes I'll merge this.

@eeeebbbbrrrr
Copy link
Contributor

Looks like this latest round of changes to the tests caused Postgres to crash. :(

@eeeebbbbrrrr
Copy link
Contributor

Thanks for your work and tenacity, @thesuhas. This stuff ain’t easy!

@eeeebbbbrrrr eeeebbbbrrrr merged commit c0de15d into pgcentralfoundation:develop Jun 6, 2025
16 checks passed
@thesuhas thesuhas deleted the expose_guc_assign_hook branch June 6, 2025 14:27
eeeebbbbrrrr added a commit that referenced this pull request Jun 28, 2025
Welcome to pgrx v0.15.0. This begins a new series for pgrx that includes
support for Postgres 18. As of this release, that means Postgres
18beta1.

This release does contain a few breaking API changes but they're largely
mechanical. Don't worry, the compiler will let you know!

As always, please install our CI tool with `cargo install cargo-pgrx
--version 0.15.0 --locked` and then run `cargo pgrx upgrade` in all of
your extension crates.

If you want to start working with Postgres 18beta1, you'll also need to
re-init your pgrx environment with `cargo pgrx init`. That will
automatically detect all the latest Postgres versions, including
18beta1.

At the top here, I'd like to thank @silver-ymz for the 18beta1 support.
It was a pleasant surprise to see that work come from the community --
it's no easy task to add a new Postgres version to pgrx!

That said, as Postgres 18 is currently beta, you should consider pgrx'
support for it as beta too. Please report any problems with 18beta1 (or
discrepancies with other versions) as GitHub issues.

Also, this release requires rust v1.88.0 or greater. `if-let` chains are
now a thing and we're not afraid to use them.

# What's Changed

## Postgres 18beta1 Support

* Support Postgres 18beta1 by @silver-ymz in
#2056
* pg18 support: add header and implement `#define` by @eeeebbbbrrrr in
#2094
* improve pg_magic_func by @usamoi in
#2088


## More Headers

* Added `catalog/heap.h` binding by @ccleve in
#2072
* include `utils/pg_status.h` by @eeeebbbbrrrr in
#2091


## `cargo-pgrx` improvements

* Pass `LLVM_*` variables to `--runas` command by @theory in
#2083
* `does_db_exist()`: fix `psql` argument order by @eeeebbbbrrrr in
#2093
* `cargo pgrx regress` output is no longer fully buffered by
@eeeebbbbrrrr in #2095
* Detect `pgrx_embed` name from lib name by @YohDeadfall in
#2035
* Fixed error message if no artifact found by @YohDeadfall in
#2034
* `cargo-pgrx`: use system certificate store for HTTPS validation by
@charmitro in #2074
* Decoding command output in Windows by @if0ne in
#2084


## Breaking Changes

* fix GUC by @usamoi in
#2064
* refactor GUC by @usamoi in
#2066

## New Stuff

* Added `pg_binary_protocol` attribute to derive send and receive
functions for `PostgresType` by @LucaCappelletti94 in
#2068
* Expose guc hooks by @thesuhas in
#2075
* Allows to create multiple aggregates for the same Rust type by @if0ne
in #2078



## General Code Cleanup

* `cargo clippy --fix` by @eeeebbbbrrrr in
#2092
* Use `if-let` to unpack Options by @stuhood in
#2089
* docs: fix typo in `rust_byte_slice_to_bytea()` docs by @burmecia in
#2071
* Added a missing `#[doc(hidden)]` by @LucaCappelletti94 in
#2079

## Administrative

* Updated Fedora to latest in CI by @YohDeadfall in
#2085
* fix ci on beta rust (1.89) by @usamoi in
#2087

## New Contributors

Much thanks to our new contributors! Your work is sincerely appreciated!

* @charmitro made their first contribution in
#2074
* @thesuhas made their first contribution in
#2075
* @if0ne made their first contribution in
#2084
* @stuhood made their first contribution in
#2089

**Full Changelog**:
v0.14.3...v0.15.0
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.

5 participants