reimpl Drop for pgx::datum::Array<'a, T>
#706
Conversation
While discussing the 0.5.0 release with @Hoverbear, the current situation regarding leaking memory stuck out as a sore spot. We discussed ways to make the `Drop` impl sound again, and I found this implementation after a bit of thought. It should actually be correct this time.
There was a problem hiding this comment.
Ah, so, the issue is that when pg_sys::deconstruct_array is called, Postgres builds an alternate "expanded" representation. This was selected by PGX because it is more... "user-friendly". However, in doing so, it allocates: once for the [Datum]s, once for the [bool]s.
pgx/src/datum/array.rs
Outdated
| // Now we check for detoasting clones | ||
| let raw = raw.take().map(|r| r.into_ptr()); | ||
| let ptr = ptr.take(); | ||
| match (ptr, raw) { | ||
| // SAFETY: if pgx detoasted a clone of this varlena, pfree the clone | ||
| (Some(p), Some(r)) if r.cast() != p => unsafe { pg_sys::pfree(r.as_ptr().cast()) }, | ||
| _ => (), |
There was a problem hiding this comment.
Postgres also may allocate when you call pg_detoast_datum: the canonical way to check if it did is checking pointer equivalence. 🫠 So this .take()s both the varlena pointer from earlier and the RawArray from earlier, and checks that they are both Some, mapping RawArray to *mut ArrayType, casting from there to *mut pg_sys::varlena to compare, then casting again to *mut c_void to free it.
pgx/src/datum/array.rs
Outdated
| // Assert correctness of our nullness checks and cleanup. | ||
| // SAFETY: Should be correctly constructed for slice validity. | ||
| let pallocd_null_slice = unsafe { slice::from_raw_parts(nulls, nelems) }; | ||
| for i in 0..nelems { | ||
| assert_eq!(null_slice.get(i).unwrap(), pallocd_null_slice[i]); | ||
| } | ||
|
|
||
| // Throw away the slice we made. | ||
| mem::drop(pallocd_null_slice); | ||
| // SAFETY: We made it, we can break it. Or Postgres can, at least. | ||
| unsafe { pg_sys::pfree(nulls.cast()) }; | ||
|
|
There was a problem hiding this comment.
This code continues to call deconstruct_array to maintain compatibility for current schemes for now, but it immediately frees the pallocated slice of bool as unnecessary after checking that our NullKind-wrapped BitSlice "understands" the null mapping in the same way.
pgx/src/datum/array.rs
Outdated
| if let Array { | ||
| ptr, | ||
| raw, | ||
| datum_palloc: Some(data), | ||
| elem_slice, | ||
| .. | ||
| } = self | ||
| { | ||
| // It's just a slice, dropping it doesn't "do" anything, but out of an abundance of caution: | ||
| mem::drop(elem_slice); | ||
| // No conflict possible with ditching this. | ||
| unsafe { pg_sys::pfree(data.as_ptr().cast()) }; |
There was a problem hiding this comment.
While dropping the Array, check if the datum_palloc field is Some, and then if so, drop the elem_slice (it's a no-op, but it means there's no other pointers) and pfree the pointer to the allocation of Datums.
|
Understand better now, thanks, :) I didn't get much chance to dig into arrays yet. |
While discussing the 0.5.0 release with @Hoverbear, the current situation regarding leaking memory stuck out as a sore spot. We discussed ways to make the
Dropimpl sound again, and I found this implementation after a bit of thought. It should actually be correct this time.This effectively reverses #633 but is more correct than the prior state.