Skip to content

Manually bind static MyProcPort and struct Port#2162

Merged
workingjubilee merged 8 commits into
pgcentralfoundation:developfrom
workingjubilee:add-myprocport
Nov 3, 2025
Merged

Manually bind static MyProcPort and struct Port#2162
workingjubilee merged 8 commits into
pgcentralfoundation:developfrom
workingjubilee:add-myprocport

Conversation

@workingjubilee

@workingjubilee workingjubilee commented Oct 23, 2025

Copy link
Copy Markdown
Member

This is my proposed alternative to #2117 building on @usamoi's suggestion. In order to enable interacting with static MyProcPort, we introduce a definition of the struct Port type in mod libpq that is available on all versions of Postgres. Several fields are left private to discourage access to them, as in general enabling access to Postgres's cryptography APIs for authentication and authorization is outside the desired project scope of PGRX.

In addition, in one (but only one) version, Postgres 17, the result of size_of::<Port>() is not even guaranteed to be correct. This is because we don't immediately know, without implementing cfg(accessible(..)), whether or not these fields should exist or not. Because of these difficulties, we leave those dubious fields deprecated on Postgres 17 so that no one interacts with them until such time as we gain more confidence in that.

Fixes #2157

cc @cbandy

@workingjubilee workingjubilee force-pushed the add-myprocport branch 3 times, most recently from 1477ed3 to 9e613a3 Compare October 23, 2025 21:50

@cbandy cbandy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's one more field that needs an annotation for pg13, but everything else LGTM.

It was tedious to check, but I suppose we'll only need to compare when adding a major version.

Thank you for working on this!

Comment thread pgrx-pg-sys/src/libpq.rs
Comment thread pgrx-pg-sys/src/libpq.rs
Comment thread pgrx-pg-sys/src/libpq.rs Outdated
Comment thread pgrx-pg-sys/src/libpq.rs
Comment thread pgrx-pg-sys/src/libpq.rs
Comment thread pgrx-pg-sys/src/libpq.rs Outdated
Comment thread pgrx-pg-sys/src/libpq.rs
Comment thread pgrx-pg-sys/src/libpq.rs Outdated
Comment thread pgrx-pg-sys/src/libpq.rs Outdated
Comment on lines +91 to +96
#[cfg(feature = "pg18")]
raw_buf: *mut core::ffi::c_char,
#[cfg(feature = "pg18")]
raw_buf_consumed: isize,
#[cfg(feature = "pg18")]
raw_buf_remaining: isize,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍🏻

Comment thread pgrx-pg-sys/src/libpq.rs Outdated
last_read_was_eof: bool,

// NOTE: 5 fields remain on PG17, but two are `#ifdef USE_OPENSSL` in PG17, so treat all
// as conditioned on PG18, even if that is not strictly accurate for PG17

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤔 So we're too small in pg17. I'm not sure what to do about that or how to better explain it to the reader. 👍🏻

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm currently inclined to write it off as "if someone used size_of on a random Postgres struct it was probably wrong anyways" because of Postgres gleefully (ab)using the "pointer-to-header" pattern.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If I'm tracking correctly... can we add some _padding bytes in the correct spots?

Or would it be better to just define this struct once for every Postgres version where it's different? It is a bit difficult to reason about as currently written whereas defining it N times would be easier to see what's going on at the expense of duplicating portions of the struct. I think that's probably a fair compromise, yeah?

@workingjubilee workingjubilee Nov 3, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think for Postgres 17 we can get as close as making sure there's some padding to at least match the minimum size, but it will be inexact. I'll add some with a comment.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hm. Yeah, I'm going to split this along the Postgres 17 "divider".

workingjubilee and others added 4 commits October 27, 2025 21:42
Co-authored-by: Chris Bandy <bandy.chris@gmail.com>
Co-authored-by: Chris Bandy <bandy.chris@gmail.com>
@workingjubilee

Copy link
Copy Markdown
Member Author

@eeeebbbbrrrr I don't intend to merge this without your review because we ought to be aligned on this: if this feels like it's flying too close to the sun, then let's not. But I figure that this works out well-enough for our purposes.

Comment thread pgrx-pg-sys/src/libpq.rs Outdated
Comment thread pgrx-pg-sys/src/libpq.rs Outdated
Comment on lines +98 to +103
#[cfg(feature = "pg18")]
raw_buf: *mut core::ffi::c_char,
#[cfg(feature = "pg18")]
raw_buf_consumed: isize,
#[cfg(feature = "pg18")]
raw_buf_remaining: isize,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment thread pgrx-pg-sys/src/libpq.rs Outdated
Comment thread pgrx-pg-sys/src/libpq.rs
// NOTE: 5 fields remain on PG17, but two are `#ifdef USE_OPENSSL` in PG17, so treat all
// as conditioned on PG18, even if that is not strictly accurate for PG17

// as if USE_OPENSSL == false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can implement #[cfg(accessible(crate::USE_OPENSSL))] here through bindgen::callbacks::ParseCallbacks.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

hm, that can fix the size on pg17, I guess

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not sure how, at least not right away, @usamoi. I'm not seeing an obvious header to include that doesn't also try to pull in OpenSSL.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure how, at least not right away, @usamoi. I'm not seeing an obvious header to include that doesn't also try to pull in OpenSSL.

It's pg_config.h, which is already included.

Screenshot_20251028_140609

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

huh. then why doesn't our code define USE_OPENSSL as a constant?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

huh. then why doesn't our code define USE_OPENSSL as a constant?

It's undefined if PostgreSQL is not compiled with OpenSSL. If we set the environment variable PGRX_PG_CONFIG_PATH to the pg_config of a PostgreSQL build compiled with OpenSSL, then pgrx_pg_sys::USE_OPENSSL will be present.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

oh right, duh.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

in ParseCallbacks::int_macro, right? I see, I see.

@eeeebbbbrrrr

Copy link
Copy Markdown
Contributor

@eeeebbbbrrrr I don't intend to merge this without your review because we ought to be aligned on this: if this feels like it's flying too close to the sun, then let's not. But I figure that this works out well-enough for our purposes.

I'm aligned in spirit. I don't really have any care about how this is done, other than maybe this comment. It seems to me that size_of::<T>() ought to be correct for this struct across the various Postgres versions.

Comment thread pgrx-pg-sys/src/libpq.rs Outdated
Comment thread pgrx-pg-sys/src/libpq.rs Outdated
@workingjubilee workingjubilee changed the title Manually bind MyProcPort Manually bind static MyProcPort and struct Port Nov 3, 2025
@workingjubilee workingjubilee merged commit 4b980d1 into pgcentralfoundation:develop Nov 3, 2025
14 checks passed
@workingjubilee workingjubilee deleted the add-myprocport branch November 3, 2025 21:04

@cbandy cbandy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍🏻 I like carving pg17 out as the... bad one from upstream.

🔧 I think I see two missing fields, but I may be mistaken.

Comment thread pgrx-pg-sys/src/libpq.rs
Comment on lines +113 to +117
ssl_in_use: bool,
peer_cn: *mut core::ffi::c_char,
peer_cert_valid: bool,

alpn_used: bool,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this have a peer_dn field here?

https://github.com/postgres/postgres/blob/REL_17_0/src/include/libpq/libpq-be.h#L199-L206

	/*
	 * SSL structures.
	 */
	bool		ssl_in_use;
	char	   *peer_cn;
	char	   *peer_dn;
	bool		peer_cert_valid;
	bool		alpn_used;

Comment thread pgrx-pg-sys/src/libpq.rs
Comment on lines +187 to +192
ssl_in_use: bool,
peer_cn: *mut core::ffi::c_char,
peer_cert_valid: bool,

alpn_used: bool,
last_read_was_eof: bool,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here; peer_dn field?

https://github.com/postgres/postgres/blob/REL_18_0/src/include/libpq/libpq-be.h#L205-L213

	/*
	 * SSL structures.
	 */
	bool		ssl_in_use;
	char	   *peer_cn;
	char	   *peer_dn;
	bool		peer_cert_valid;
	bool		alpn_used;
	bool		last_read_was_eof;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for the catch.

daamien pushed a commit to daamien/pgrx that referenced this pull request Dec 15, 2025
…ion#2162)

This is my proposed alternative to pgcentralfoundation#2117
building on @usamoi's suggestion. In order to enable interacting with
`static MyProcPort`, we introduce a definition of the `struct Port` type
in `mod libpq` that is available on all versions of Postgres. Several
fields are left private to discourage access to them, as in general
enabling access to Postgres's cryptography APIs for authentication and
authorization is outside the desired project scope of PGRX.

In addition, in one (but only one) version, Postgres 17, the result of
`size_of::<Port>()` is not even guaranteed to be correct. This is
because we don't immediately know, without implementing
`cfg(accessible(..))`, whether or not these fields should exist or not.
Because of these difficulties, we leave those dubious fields deprecated
on Postgres 17 so that no one interacts with them until such time as we
gain more confidence in that.

Fixes pgcentralfoundation#2157

---------

Co-authored-by: Chris Bandy <bandy.chris@gmail.com>
Co-authored-by: usamoi <usamoi@outlook.com>
eeeebbbbrrrr added a commit that referenced this pull request Feb 9, 2026
Welcome to pgrx v0.17.0. This is a new minor release that brings a bunch
of internal code refactoring, cleanup, new Postgres headers, and
additional `cargo-pgrx regress` CLI options.

As always, please install the latest `cargo-pgrx` with `cargo install
cargo-pgrx --version 0.17.0 --locked` and also update your extension
crate dependencies with `cargo pgrx upgrade`.

## What's Changed


### New Headers/Symbols

* include `access/tsmapi.h` by @usamoi in
#2155
* Include access/subtrans.h & access/commit_ts.h by @isdaniel in
#2147
* include `utils/guc_tables.h` by @usamoi in
#2243
* Include `scanner.h` in Rust bindings by @piki in
#2163
* Add `commands/publicationcmds.h` to generated `pg_sys` bindings by
@Sinjo in #2221
* Add the ClientAuthentication_hook by @daamien in
#2231
* add storage/dsm_*.h headers by @eeeebbbbrrrr in
#2244
* pg-sys: add catalog/heap.h bindings for PG18 by @charmitro in
#2150
* Add back `peer_dn` field to `Port` on `pg17..` by @workingjubilee in
#2200
* Add `repr(C)` to `struct Port` on `pg13..=16` by @workingjubilee in
#2201

### `cargo-pgrx` improvements

* Add a psql_verbosity parameter for cargo pgrx regress by @daamien in
#2230


### Code Cleanup

* Replace with let-chains in pgrx-bindgen by @workingjubilee in
#2168
* Replace with let-chains in pgrx-sql-entity-graph by @workingjubilee in
#2169
* Restore so-called needless lifetimes by @workingjubilee in
#2167
* Drop stray Postgres 12 support in cargo-pgrx by @workingjubilee in
#2173
* Remove deprecated enum##member constants by @workingjubilee in
#2170
* Remove `variadic!` macro by @workingjubilee in
#2171
* Semi-automated code cleanup by @workingjubilee in
#2189
* Deref instead of borrowing `Option<&String>` by @workingjubilee in
#2185
* Refactor schema.rs using a cargo command builder by @workingjubilee in
#2187
* Replace `impl AsRef<T>` with `&T` by @workingjubilee in
#2182
* Make `PgRelation::heap_relation` unsafe by @workingjubilee in
#2176
* Move `cargo_pgrx::env` to `cargo_pgrx::cargo` by @workingjubilee in
#2186
* Move `CargoProfile` into `cargo_pgrx::cargo` by @workingjubilee in
#2188
* Add Scalar trait for generic handling of primitive arrays by
@workingjubilee in #2153
* Set BGWORKER_SHMEM_ACCESS when building background workers by @cbandy
in #2161
* Stop referencing `pg_config` in schema generation by @workingjubilee
in #2174
* Show line number of failed asserts by @piki in
#2175
* Ghostbust `used_type.rs` a bit by @workingjubilee in
#2190
* Manually bind `static MyProcPort` and `struct Port` by @workingjubilee
in #2162
* Make datetime types import each other via `use super` by
@workingjubilee in #2198
* Migrate datetime types into `pgrx::datetime` by @workingjubilee in
#2199
* Use diagnostic attrs for ABI/SQL-related traits by @workingjubilee in
#2203
* Make `Scalar` provide `pg_sys::Oid` constant by @workingjubilee in
#2209
* Hoist varlena encoding fn by @workingjubilee in
#2208
* Recurse through `T` while anonymizing `&T` by @workingjubilee in
#2212
* `impl BorrowDatum for TimeTz` by @workingjubilee in
#2213
* Change `MemCx::alloc_bytes` to return `NonNull` by @workingjubilee in
#2214
* Add `MemCx<'current, T>` arguments and `PBox<'current, T>` returns by
@workingjubilee in #2210
* Further improve diagnostics for signatures with no SQL form by
@workingjubilee in #2215
* Add `array::Element` and `callconv::DatumPass` by @workingjubilee in
#2218
* Remove unused line in `memcxt_tests.rs` by @workingjubilee in
#2220
* Add `FlatArray<'_, T>` by @workingjubilee in
#2207
* Allow explicit handling of allocation errors by @workingjubilee in
#2226
* Place lbound before updating array len product by @workingjubilee in
#2236
* introduce `#[pg_guard(unsafe_entry_thread)]` by @eeeebbbbrrrr in
#2242


### Project Administrativa 

* Use Rust Edition 2024 by @workingjubilee in
#2165
* Remove testing for deprecated Intel Macs by @workingjubilee in
#2156
* Remove unused Cargo.lock for version-updater by @workingjubilee in
#2180
* Add rust-analyzer.toml by @workingjubilee in
#2179
* Note pgrx requires an unknown minimum Xcode version by @workingjubilee
in #2164

## New Contributors

* @isdaniel made their first contribution in
#2147
* @cbandy made their first contribution in
#2161
* @piki made their first contribution in
#2163
* @Sinjo made their first contribution in
#2221

**Full Changelog**:
v0.16.1...v0.17.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.

Bindings for client information in Port

4 participants