Manually bind static MyProcPort and struct Port#2162
Conversation
1477ed3 to
9e613a3
Compare
cbandy
left a comment
There was a problem hiding this comment.
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!
| #[cfg(feature = "pg18")] | ||
| raw_buf: *mut core::ffi::c_char, | ||
| #[cfg(feature = "pg18")] | ||
| raw_buf_consumed: isize, | ||
| #[cfg(feature = "pg18")] | ||
| raw_buf_remaining: isize, |
| 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 |
There was a problem hiding this comment.
🤔 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. 👍🏻
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hm. Yeah, I'm going to split this along the Postgres 17 "divider".
Co-authored-by: Chris Bandy <bandy.chris@gmail.com>
Co-authored-by: Chris Bandy <bandy.chris@gmail.com>
e108d8c to
bff6d29
Compare
|
@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. |
| #[cfg(feature = "pg18")] | ||
| raw_buf: *mut core::ffi::c_char, | ||
| #[cfg(feature = "pg18")] | ||
| raw_buf_consumed: isize, | ||
| #[cfg(feature = "pg18")] | ||
| raw_buf_remaining: isize, |
There was a problem hiding this comment.
It's also included in PostgreSQL 17.
https://github.com/postgres/postgres/blob/REL_17_0/src/include/libpq/libpq-be.h#L226-L228
| // 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 |
There was a problem hiding this comment.
We can implement #[cfg(accessible(crate::USE_OPENSSL))] here through bindgen::callbacks::ParseCallbacks.
There was a problem hiding this comment.
hm, that can fix the size on pg17, I guess
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
huh. then why doesn't our code define USE_OPENSSL as a constant?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
in ParseCallbacks::int_macro, right? I see, I see.
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 |
static MyProcPort and struct Port
cbandy
left a comment
There was a problem hiding this comment.
👍🏻 I like carving pg17 out as the... bad one from upstream.
🔧 I think I see two missing fields, but I may be mistaken.
| ssl_in_use: bool, | ||
| peer_cn: *mut core::ffi::c_char, | ||
| peer_cert_valid: bool, | ||
|
|
||
| alpn_used: bool, |
There was a problem hiding this comment.
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;| ssl_in_use: bool, | ||
| peer_cn: *mut core::ffi::c_char, | ||
| peer_cert_valid: bool, | ||
|
|
||
| alpn_used: bool, | ||
| last_read_was_eof: bool, |
There was a problem hiding this comment.
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;There was a problem hiding this comment.
Thanks for the catch.
…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>
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
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 thestruct Porttype inmod libpqthat 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 implementingcfg(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