Add MemCx<'current, T> arguments and PBox<'current, T> returns#2210
Conversation
|
moving PBox work into this PR since it should probably work for things that aren't just FlatArray. |
&MemCx<'_> as an argumentMemCx<'current, T> arguments and PBox<'current, T> returns
fn_args: <[_]>::into_vec(::alloc::boxed::box_new([
::pgrx::pgrx_sql_entity_graph::PgExternArgumentEntity {
pattern: "memcx",
used_ty: ::pgrx::pgrx_sql_entity_graph::UsedTypeEntity {
ty_source: "& '_ MemCx < 'mcx >",
ty_id: core::any::TypeId::of::<&'_ MemCx<'mcx>>(),
full_path: core::any::type_name::<&'_ MemCx<'mcx>>(),
module_path: {
let ty_name = core::any::type_name::<&'_ MemCx<'mcx>>();
let mut path_items: Vec<_> = ty_name.split("::").collect();
let _ = path_items.pop();
path_items.join("::")
},
composite_type: None,
variadic: false,
default: None,
#[doc = r" Set via the type being an `Option`."]
optional: false,
metadata: {
use ::pgrx::pgrx_sql_entity_graph::metadata::SqlTranslatable;
<&'_ MemCx<'mcx>>::entity()
},
},
},
])),I believe it is this part of the expansion that is "buggy" for our purposes: this should be an anonymized or higher-rank lifetime in these cases. |
|
Ah, just a missing recursion for TypeReference in |
| // recurse | ||
| anonymize_lifetimes(&mut type_ref.elem); |
|
TODO:
|
|
I initially thought I could add more conveniences, more easily, but it actually has a bit of API design required to do well at it. My biggest add without much design effort is to offer a |
b23065d to
d7616e4
Compare
|
Oh, I should also include |
eeeebbbbrrrr
left a comment
There was a problem hiding this comment.
I don't like the name PBox but I don't have any better ideas that aren't annoyingly long. So I'm not going to let that hold up forward progress here. feel free to merge! Thanks!
|
|
||
| [stdbox]: alloc::boxed::Box | ||
| */ | ||
| pub struct PBox<'mcx, T: ?Sized> { |
There was a problem hiding this comment.
Hi. I don't like the name PBox but I don't have a better suggestion. I think I don't like it because it's too similar to the existing PgBox.
| } | ||
| } | ||
|
|
||
| fn is_virtual_arg() -> bool { |
There was a problem hiding this comment.
What is a "virtual arg"? I guess it's one we inject and not one the user explicitly listed in the #[pg_extern] function?
There was a problem hiding this comment.
Yes, to be clear it's one that doesn't appear in the generated SQL: it's real to Rust, "fake" to SQL, and not a Datum.
55a59ae to
74b96a3
Compare
|
The macOS failures are utterly mysterious. |
25285e4
into
pgcentralfoundation:develop
…gcentralfoundation#2210) tl;dr: `caller_picks_lifetime + macro_expansion == unsoundness`, so prevent that by adding API for new, lifetime-bound allocations. For functions exposed to PostgreSQL via `pg_extern`, signatures often resemble these: ```rust #[pg_extern] fn do_something( floats: Array<'a, f32>, hms: Time, day: TimestampTz ) -> Vec<TimestampTz> { ``` ```sql -- pgrx will generate something like this CREATE FUNCTION do_something( floats real[], hms time, day timestamptz ) RETURNS timestamptz[] ``` In SQL, we accept an array and return an array of a different type, but in Rust, we return a `Vec`. This is because we lack API to allocate and return a fresh `Array<'a, T>`s. Behind the scenes, we *do* allocate an `ArrayType` at the translation boundary, unseen to a Rust programmer. That, plus the Vec, means two allocations just to mutate an array, even for a fixed-size type! Unfortunately, pgrx-offered type translations like `Array<'a, T>`, cannot have a lifetime *and* expose a new *safe* function to make it from lifetime-free values! Such API must have the `unsafe` requirement of access-exclusivity for its lifetime, described in functions like `slice::from_raw_parts_mut`[^1], because the lifetime becomes caller-selected. This makes it easy to use a `'static` lifetime with `pg_extern` and expand `unsafe` code that trusts that lifetime. A `'static` lifetime is always wrong for `palloc`ated types, even with `TopMemoryContext`, since it dies whenever Postgres resets the context and Postgres does have conditions under which it performs such a global reset. The result is various extremely subtle soundness problems are possible, yet the root cause lies in code that most never read. We can solve this by denying the possibility of a caller-selected lifetime. This will require removing some types, ultimately, but first we need an API to migrate to. This new API is `MemCx<'mcx, T>` and `PBox<'mcx, T>`. Importantly, there is no *safe* route to creating `MemCx<'mcx, T>` with a caller-selected lifetime. One can only reach `MemCx<'mcx, T>` safely as an argument: either pass a closure to `memcx::current_context` or call a new `pg_extern` function via SQL. Either way gives you `&MemCx<'current>`. These let you create a lifetime-bound allocation that can live long enough to return it. We can already return fixed-size pass-by-reference types, so right now `PBox` is only an optimization. It allows not putting a type on the stack and then immediately moving it directly into a heap allocation. More importantly, it enables writing tests, so it is important ground work for returning de facto unsized values, as they can hide behind `PBox` which is always `Sized`. [^1]: https://doc.rust-lang.org/std/slice/fn.from_raw_parts_mut.html
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
tl;dr:
caller_picks_lifetime + macro_expansion == unsoundness,so prevent that by adding API for new, lifetime-bound allocations.
For functions exposed to PostgreSQL via
pg_extern, signatures often resemble these:In SQL, we accept an array and return an array of a different type, but in Rust, we return a
Vec. This is because we lack API to allocate and return a freshArray<'a, T>s. Behind the scenes, we do allocate anArrayTypeat the translation boundary, unseen to a Rust programmer. That, plus the Vec, means two allocations just to mutate an array, even for a fixed-size type!Unfortunately, pgrx-offered type translations like
Array<'a, T>, cannot have a lifetime and expose a new safe function to make it from lifetime-free values! Such API must have theunsaferequirement of access-exclusivity for its lifetime, described in functions likeslice::from_raw_parts_mut1, because the lifetime becomes caller-selected. This makes it easy to use a'staticlifetime withpg_externand expandunsafecode that trusts that lifetime.A
'staticlifetime is always wrong forpallocated types, even withTopMemoryContext, since it dies whenever Postgres resets the context and Postgres does have conditions under which it performs such a global reset. The result is various extremely subtle soundness problems are possible, yet the root cause lies in code that most never read. We can solve this by denying the possibility of a caller-selected lifetime. This will require removing some types, ultimately, but first we need an API to migrate to.This new API is
MemCx<'mcx, T>andPBox<'mcx, T>. Importantly, there is no safe route to creatingMemCx<'mcx, T>with a caller-selected lifetime. One can only reachMemCx<'mcx, T>safely as an argument: either pass a closure tomemcx::current_contextor call a newpg_externfunction via SQL. Either way gives you&MemCx<'current>. These let you create a lifetime-bound allocation that can live long enough to return it.We can already return fixed-size pass-by-reference types, so right now
PBoxis only an optimization. It allows not putting a type on the stack and then immediately moving it directly into a heap allocation. More importantly, it enables writing tests, so it is important ground work for returning de facto unsized values, as they can hide behindPBoxwhich is alwaysSized.Footnotes
https://doc.rust-lang.org/std/slice/fn.from_raw_parts_mut.html ↩