Fix toasted FlatArray argument borrowing#2355
Conversation
6c391bd to
3703a43
Compare
e60a8e4 to
0a8732d
Compare
0a8732d to
c5281a6
Compare
|
I designed the type specifically to avoid this kind of memory overhead for creating a referent. |
| /// released with `pg_sys::pfree`. Passing it transfers ownership, so the implementation must | ||
| /// not free it or register the caller-owned `ptr`. | ||
| #[doc(hidden)] | ||
| unsafe fn borrow_arg_unchecked<'dat>( |
There was a problem hiding this comment.
This is entirely unsuitably named.
| } | ||
| } | ||
|
|
||
| unsafe fn borrow_arg_unchecked<'dat>( |
| fn register_detoasted_arg(&mut self, ptr: NonNull<u8>) { | ||
| // Set-returning functions unbox their arguments in a multi-call context so an iterator | ||
| // may retain borrows across calls. That context owns its detoast allocations and releases | ||
| // them when the SRF finishes. Ordinary functions unbox in the entry context, where we can | ||
| // release fresh detoast copies as soon as their result has been boxed. | ||
| assert_eq!( | ||
| unsafe { pg_sys::CurrentMemoryContext }, | ||
| self.argument_memory_context, | ||
| "detoasted argument was allocated outside its selected memory context" | ||
| ); | ||
| if self.argument_memory_context != self.entry_memory_context { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This is do-nothing assertion code that does not improve correctness.
| /// not cloneable. | ||
| pub struct FcInfo<'fcx> { | ||
| ptr: pgrx_pg_sys::FunctionCallInfo, | ||
| detoasted_arg_allocations: Vec<NonNull<u8>>, |
There was a problem hiding this comment.
No, this is not appropriate. This type is intended to be 1 to 1 with FunctionCallInfo, such that we can transmute if necessary, and this breaks that.
| impl Drop for FcInfo<'_> { | ||
| fn drop(&mut self) { | ||
| for allocation in self.detoasted_arg_allocations.drain(..) { | ||
| // SAFETY: `BorrowDatum::borrow_arg_unchecked` requires registered pointers to be | ||
| // PostgreSQL allocations that can be released after the function result is boxed. | ||
| unsafe { pg_sys::pfree(allocation.as_ptr().cast()) } | ||
| } | ||
| } |
There was a problem hiding this comment.
No, this is not an appropriate way to handle this. It WILL introduce problems, because it means that we are terminating something before the expiry of 'fcx, which strictly outlives FcInfo per se, and the model for our code depends on not doing that.
|
Remember that your machine pet cannot perform logic so it cannot give a correct answer to a question, just a likely one. |
|
Speaking purely observationally, it is also loathe, unless given instructions otherwise, to rewrite or redesign code because most code it is interacting with is working code. But that, of course, is a merely probabilistic statement. |
|
If |
|
@workingjubilee Yeah, there was basically nothing about this that I liked which is why I didn't merge it. I would like to fix the crash reported in the bug but I didn't see an obvious solution and my boy Claude made it things worse. I appreciate you looking at this.
I don't know how we'd even do that. Any of postgres' varlena-style types could be toasted. Part of the problem, I think is this: unsafe impl<T: ?Sized> BorrowDatum for FlatArray<'_, T> {
const PASS: layout::PassBy = layout::PassBy::Ref;
unsafe fn point_from(ptr: ptr::NonNull<u8>) -> ptr::NonNull<Self> {We can pass in any old pointer here but I guess what we need is a way to enforce that it's a "detoasted pointer"? OTOH, the original bug report was about SPI doing things wrong, so maybe we need to just make sure the Datum is detoasted before (I won't merge this PR). |
|
cool! I do understand putting up the PR. It certainly at least attracted my notice. :P |
2 participants
Summary
FlatArrayspecifically while unboxing borrowed PostgreSQL function argumentsBorrowDatum::point_fromallocation-free for already-shaped owned pointersFixes #2350.
Root cause
FlatArrayrequires a contiguous, alignedArrayType, but the blanket borrowed-argument ABI passed the raw Datum pointer directly toFlatArray::point_from. For an external TOAST pointer, that code interpreted the TOAST representation as anArrayTypeand the backend segfaulted.This fix deliberately does not detoast in
point_fromas suggested in the issue. Pointer shaping remains pure forPBox<FlatArray>and other already-valid allocations. Instead, a defaultedBorrowDatumargument hook letsFlatArrayreport fresh detoast copies to the call frame.For ordinary scalar calls,
FcInfofrees those copies after the Rust result has been converted to a PostgreSQL Datum. Set-returning functions are different: theirRetAbiswitches argument construction intomulti_call_memory_ctx, and their iterators may borrow inputs across calls. The generated wrapper records the exact context selected byRetAbibefore decoding arguments, and registration fails fast if detoasting occurred in any other context. SRF copies remain context-owned until PostgreSQL ends the SRF. ExistingBorrowDatumimplementations and#[pg_extern]signatures remain unchanged.Cleanup uses exclusive ownership rather than interior mutability:
FcInfoowns a plainVecof scalar-call allocations, and its argument decoder borrows that call frame through&mut. As a result, the low-level call-convention API is intentionally tightened:FcInfois no longerClone,FcInfo::argsrequires&mut self, andArgsis a lending-style decoder rather than a standardIterator. Normal#[pg_extern]functions and downstreamArgAbiimplementation signatures are unchanged. Keeping the old low-level shapes would require shared mutable cleanup ownership and would make it possible to free an allocation while a borrow from a cloned frame was still live.Verification
Rc,RefCell, or other interior-mutability primitive is usedRetAbicontext into the decoder and registration asserts it is currentarray_borrowedmodule: 22 passedpgrx-unit-testslibrary suite: 570 passed, 1 ignoredpgrxtests: 19 unit tests and 72 doctests passedpgrxcompile checks passed with pg13 and pg19Formatting and diff checks pass. A seven-technique deep review of the call graph, scalar/SRF traces, invariants, ownership, failure modes, and diff minimality has no open findings after tightening the context-selection and unsafe-pointer contracts.
The unfiltered workspace test command still has an unrelated trybuild snapshot mismatch in
arrays-dont-leak.stderrunder Rust 1.96, and strict Clippy is blocked by existing warnings in unchanged files. Neither failure points into this change.