reimpl Drop for pgx::datum::Array<'a, T> (#706)

* re`impl Drop for pgx::datum::Array<'a, T>`

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.

* Explain the implicit allocations pgx will Drop

pgx/src/datum/array.rs

@@ -18,9 +18,11 @@ use std::marker::PhantomData;
 use std::{mem, ptr, slice};
 
 pub struct Array<'a, T: FromDatum> {
-    _ptr: Option<NonNull<pg_sys::varlena>>,
+    ptr: Option<NonNull<pg_sys::varlena>>,
     raw: Option<RawArray>,
     nelems: usize,
+    // Remove this field if/when we figure out how to stop using pg_sys::deconstruct_array
+    datum_palloc: Option<NonNull<pg_sys::Datum>>,
     elem_slice: &'a [pg_sys::Datum],
     null_slice: NullKind<'a>,
     elem_layout: Option<Layout>,
@@ -69,6 +71,36 @@ impl<'a, T: FromDatum + serde::Serialize> serde::Serialize for Array<'a, T> {
     }
 }
 
+impl<'a, T: FromDatum> Drop for Array<'a, T> {
+    fn drop(&mut self) {
+        if let Array {
+            ptr,
+            raw,
+            datum_palloc: Some(data),
+            elem_slice,
+            ..
+        } = self
+        {
+            // If Drop has arrived here, it means that this Array is backed by an allocation or two
+            // If so, the first one is guaranteed, and was created by calling pg_sys::deconstruct_array
+            // This is just a slice, dropping it doesn't "do" anything, but out of an abundance of caution:
+            mem::drop(elem_slice);
+            // Now there shouldn't be any other references to that slice, so this can be deallocated:
+            unsafe { pg_sys::pfree(data.as_ptr().cast()) };
+
+            // Detoasting the varlena may have allocated: the toasted varlena cloned as a detoasted ArrayType
+            // Checking for pointer equivalence is the only way we can truly tell
+            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()) },
+                _ => (),
+            }
+        }
+    }
+}
+
 #[deny(unsafe_op_in_unsafe_fn)]
 impl<'a, T: FromDatum> Array<'a, T> {
     /// Create an [`Array`](crate::datum::Array) over an array of [`pg_sys::Datum`](pg_sys::Datum) values and a corresponding array
@@ -100,13 +132,14 @@ impl<'a, T: FromDatum> Array<'a, T> {
         //
         // Remember to remove the Array::over tests in pgx-tests/src/tests/array_tests.rs
         // when you finally kill this off.
-        let _ptr: Option<NonNull<pg_sys::varlena>> = None;
+        let ptr: Option<NonNull<pg_sys::varlena>> = None;
         let raw: Option<RawArray> = None;
         let elem_layout: Option<Layout> = None;
         Array::<T> {
-            _ptr,
+            ptr,
             raw,
             nelems,
+            datum_palloc: None,
             elem_slice: unsafe { slice::from_raw_parts(elements, nelems) },
             null_slice: unsafe { slice::from_raw_parts(nulls, nelems) }.into(),
             elem_layout,
@@ -119,7 +152,7 @@ impl<'a, T: FromDatum> Array<'a, T> {
     /// This function requires that the RawArray was obtained in a properly-constructed form
     /// (probably from Postgres).
     unsafe fn deconstruct_from(
-        _ptr: Option<NonNull<pg_sys::varlena>>,
+        ptr: Option<NonNull<pg_sys::varlena>>,
         raw: RawArray,
         layout: Layout,
     ) -> Array<'a, T> {
@@ -138,9 +171,6 @@ impl<'a, T: FromDatum> Array<'a, T> {
         some of which are implicitly embedded in other methods (e.g. Array::over).
         It also risks leaking memory, as deconstruct_array calls palloc.
 
-        TODO(0.6.0): Start implementing Drop again when we no longer have Array::over.
-        See tcdi/pgx#627 and #633 for why this is the preferred resolution to this.
-
         SAFETY: We have already asserted the validity of the RawArray, so
         this only makes mistakes if we mix things up and pass Postgres the wrong data.
         */
@@ -168,10 +198,25 @@ impl<'a, T: FromDatum> Array<'a, T> {
             .map(|nonnull| NullKind::Bits(unsafe { &*nonnull.as_ptr() }))
             .unwrap_or(NullKind::Strict(nelems));
 
+        // The array was just deconstructed, which allocates twice: effectively [Datum] and [bool].
+        // But pgx doesn't actually need [bool] if NullKind's handling of BitSlices is correct.
+        // So, assert correctness of the NullKind implementation and cleanup.
+        // SAFETY: The pointer we got 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()) };
+
         Array {
-            _ptr,
+            ptr,
             raw: Some(raw),
             nelems,
+            datum_palloc: NonNull::new(elements),
             elem_slice: /* SAFETY: &[Datum] from palloc'd [Datum] */ unsafe { slice::from_raw_parts(elements, nelems) },
             null_slice,
             elem_layout: Some(layout),