Possible issue with function types in generated bindings

[Sinjo]

Hey, I've been doing some work that involves setting up an fmgr_hook and I've run into what I think is an issue with the types of the generated function bindings.

In my hook, I'm trying to change the function that Postgres calls in certain situations by setting different values in flinfo before returning from the hook. Specifically I'm calling a built-in function (pgsql_version) in those situations.

The issue I've run into is that the generated binding (pg_sys::pgsql_version) is declared as a "Rust" function rather than a "C-unwind" one. I did a little bit of digging in the generated bindings and noticed that the function is wrapped in an unsafe extern "C-unwind" block

#[pgrx_macros::pg_guard]
unsafe extern "C-unwind" {
... // a tonne of other function definitions
    pub fn pgsql_version(fcinfo: FunctionCallInfo) -> Datum;

but that the pg_guard proc macro intentionally rewrites those functions to not be extern "C-unwind". I'm not sure if this is the intended type signature for these function bindings and I'm using PGRX wrong here, or if this is an unintended interaction between the macro and the generated function bindings.

I've made a repo with a minimal reproduction of the issue here, which hopefully makes the situation clear.

[usamoi]

When pg_guard is applied to PostgreSQL functions, it uses a shim to allow PostgreSQL functions to be called by extensions.

In your case, what you need is for the PostgreSQL function to be called by PostgreSQL, so pg_guard should not be here. The correct approach is to manually write the binding as follows.

unsafe extern "C-unwind" {
    pub fn pgsql_version(fcinfo: FunctionCallInfo) -> Datum;
}
(*flinfo).fn_addr = Some(pgsql_version);

Since the bindings in pgrx_pg_sys are mainly designed for extensions to call rather than for PostgreSQL to call, I believe we will not remove pg_guard on them.

[Sinjo]

Thanks for the quick response! That makes sense now.

I agree that it doesn't make sense to change the existing bindings and introducing a separate set for this fairly niche use-case probably isn't worth it. Would you be open to a PR adding some documentation about this or do you think it's too niche even for that?

[usamoi]

Would you be open to a PR adding some documentation about this or do you think it's too niche even for that?

You could open a PR by yourself. I'm not the maintainer of this repository, and I'm not good at writing documentation.