Round 1 of cleaning up `SpiTupleTable` by eeeebbbbrrrr · Pull Request #2 · eeeebbbbrrrr/pgrx

eeeebbbbrrrr · 2023-07-19T01:54:12Z

Create SpiTupleTable::wrap(...) to construct a new one from the global pg_sys::SPI_tuptable, update SpiCursor and Query impls accordingly. This gets rid of SpiClient::prepare_tuple_table().
Make its members private to fix cross-module leakiness
When dropped, SpiTupleTable needs to free its internal pg_sys::SPITupleTable pointer

Other changes:

Make the lifetime of what SpiCursor::fetch() returns explicit
There's no need to set pg_sys::SPI_tuptable to NULL before calling SPI_execute/SPI_cursor_fetch -- Postgres does that for us
add a test that passed before this work and still does

ping @workingjubilee for review. I wanted to PR this against my spi-remove-spi-connection branch since it's based on it.

I have plans for another PR that'll then be based on this one.

- Create `SpiTupleTable::wrap(...)` to construct a new one from the global `pg_sys::SPI_tuptable`, update `SpiCursor` and `Query` impls accordingly. This gets rid of `SpiClient::prepare_tuple_table()`. - Make its members private to fix cross-module leakiness - When dropped, `SpiTupleTable` needs to free its internal `pg_sys::SPITupleTable` pointer Other changes: - Make the lifetime of what `SpiCursor::fetch()` returns explicit - There's no need to set `pg_sys::SPI_tuptable` to NULL before calling SPI_execute/SPI_cursor_fetch -- Postgres does that for us - add a test that passed before this work and still does

eeeebbbbrrrr · 2023-07-19T01:55:25Z

pgrx/src/spi/tuple.rs

-    pub(super) current: isize,
+    // SpiTupleTable borrows global state setup by the active SpiClient.  It doesn't use the client
+    // directly, but we need to make sure we don't outlive it, so here it is
+    _client: PhantomData<&'client SpiClient>,


I had first written this as _client: &'client SpiClient, but since we don't use it I don't suppose we need to hold a real reference to it.

eeeebbbbrrrr · 2023-07-19T01:56:12Z

pgrx/src/spi/tuple.rs

+        let table = self.table.map(|table| table.as_ptr()).ok_or(SpiError::NoTupleTable)?;
+        let tupdesc = unsafe {
+            // SAFETY:  we just assured that `table` is not null
+            table.as_mut().unwrap().tupdesc
+        };
+        Ok((table, tupdesc))


was just trying to make this function readable, esp since self.table looks different now.

Is it actually possible for this pointer to be NULL?

yes. It is possible for pg_sys::SPI_tuptable to be NULL at the end of a successful statement. There's some comments around that here:

pgrx/pgrx/src/spi/tuple.rs

Lines 126 to 130 in b474516

// a query like "SELECT 1 LIMIT 0" is a valid "select"-style query that will not produce

// a SPI_tuptable. So are utility queries such as "CREATE INDEX" or "VACUUM". We might

// think that in the latter cases we'd want to produce an error here, but there's no

// way to distinguish from the former. As such, we take a gentle approach and

// processed with "no, we don't have one, but it's okay"

Maybe they're in the wrong place.

oh, but I suppose... to actually answer your question, in the cases where this function is called, no, it shouldn't be possible for for self.table to be NULL. Sorry.

But I don't know that we can make that true such that we can elide the Option around the NotNull

It would be an exceptional condition for it to be NULL -- made it should just panic in that case?

Not sure if you're asking because you see/know something I don't or just forcing me to think about it even more.

Just trying to figure out for myself if the Option is actually mandatory, here.

eeeebbbbrrrr · 2023-07-19T01:59:09Z

pgrx/src/spi/tuple.rs

+impl Drop for SpiTupleTable<'_> {
+    fn drop(&mut self) {
+        unsafe {
+            // SAFETY:  self.table was created by Postgres from whatever `pg_sys::SPI_tuptable` pointed
+            // to at the time this SpiTupleTable was constructed
+            if let Some(ptr) = self.table.take() {
+                pg_sys::SPI_freetuptable(ptr.as_ptr())
+            }
+        }
+    }
+}
+


we've been leaking pg_sys::SPI_tuptable values from SPI_execute and SPI_cursor_fetch for the duration of Spi::connect(|client| ...). This cleans that up.

This addresses part of the last bit of this comment: pgcentralfoundation#1209 (comment)

eeeebbbbrrrr · 2023-07-19T02:00:06Z

pgrx/src/spi/client.rs

-    pub(super) fn prepare_tuple_table(
-        &self,
-        status_code: i32,
-    ) -> std::result::Result<SpiTupleTable, SpiError> {
-        Ok(SpiTupleTable {
-            status_code: Spi::check_status(status_code)?,
-            // SAFETY: no concurrent access
-            table: unsafe { pg_sys::SPI_tuptable.as_mut()},
-            #[cfg(any(feature = "pg11", feature = "pg12"))]
-            size: unsafe { pg_sys::SPI_processed as usize },
-            #[cfg(not(any(feature = "pg11", feature = "pg12")))]
-            // SAFETY: no concurrent access
-            size: unsafe {
-                if pg_sys::SPI_tuptable.is_null() {
-                    pg_sys::SPI_processed as usize
-                } else {
-                    (*pg_sys::SPI_tuptable).numvals as usize
-                }
-            },
-            current: -1,
-        })
-    }
-


This code was moved over into the new SpiTupleTable::wrap() function. Doing so allows us to set its members private, which is much easier to reason about.

eeeebbbbrrrr · 2023-07-19T02:01:33Z

pgrx/src/spi/cursor.rs

-    pub fn fetch(&mut self, count: libc::c_long) -> std::result::Result<SpiTupleTable, SpiError> {
-        // SAFETY: no concurrent access
-        unsafe {
-            pg_sys::SPI_tuptable = std::ptr::null_mut();
-        }


There were a couple of these (when I suppose there "should" have been three!). It's not necessary, however. Postgres does this for us in its SPI_cursor_fetch and SPI_execute operation functions.

eeeebbbbrrrr · 2023-07-19T02:04:01Z

pgrx/src/spi/cursor.rs

-        unsafe {
-            pg_sys::SPI_tuptable = std::ptr::null_mut();
-        }
+    pub fn fetch(&mut self, count: libc::c_long) -> SpiResult<SpiTupleTable<'client>> {


I think 'client would have ended up being used here anyhow, but I wanted it to be explicit.

Note that SpiCursor::fetch() doesn't return anything that's owned by itself. It's returning a SpiTupleTable whose lifetime is governed by the active SpiClient.

I'm feeling a bit suspicious about the lifetimes here: is SpiCursor itself even actually bound to the lifetime of the SpiClient? https://www.postgresql.org/docs/current/spi-spi-cursor-open.html

Not that we cannot or even should not use a shorter lifetime, but it feels unclear here if we are.

I haven't quite gotten to thinking about cursor. It does seem right tho.

The results of SpiCursor::fetch() (the SpiTupleTable) are indeed tied to the active SpiClient.

And I think we need to assume the cursor (Portal) is too, unless SpiCursor::detach_to_name() is called. Then the magic happens inside Postgres to let it live longer.

I don't think you can read those docs as "the PortalData pointer returned by SPI_cursor_open can outlive the active SPI connection"

at least that's my understanding. I have not looked at the Postgres sources around this yet.

but detach_into_name is just allocating a String?

I don't mind if we handle the name-indexing case specially, mind, instead of using a lifetime binding for it, as it seems to be better for our sanity to handle it that way, I just want to figure out what's going on.

workingjubilee · 2023-07-19T02:17:35Z

pgrx/src/spi/tuple.rs

+            //
+            // SAFETY:  The unsafeness here is that we're accessing static globals.  Fortunately,
+            // Postgres is not multi-threaded so we're okay to do this
+            //


nervous laughter

Future Eric is gonna love this comment, if this lands.

I can't quite explain why, but this function was deadlocking with this backtrace: ``` * thread #1, name = 'cargo-pgrx', stop reason = signal SIGSTOP * frame #0: 0x00007fbe1eeb288d libc.so.6`syscall at syscall.S:38 frame #1: 0x000055aee526c5d3 cargo-pgrx`std::sys::sync::mutex::futex::Mutex::lock_contended::h6389e2305b0b005c [inlined] std::sys::pal::unix::futex::futex_wait::h30abf43e2d55aa33 at futex.rs:67:21 frame #2: 0x000055aee526c590 cargo-pgrx`std::sys::sync::mutex::futex::Mutex::lock_contended::h6389e2305b0b005c at futex.rs:57:13 frame #3: 0x000055aee4157835 cargo-pgrx`std::sync::mutex::Mutex$LT$T$GT$::lock::h4d2bb65800cc6fd3 at futex.rs:29:13 frame pgcentralfoundation#4: 0x000055aee41577ed cargo-pgrx`std::sync::mutex::Mutex$LT$T$GT$::lock::h4d2bb65800cc6fd3(self=0x000055aee6926c20) at mutex.rs:317:24 frame pgcentralfoundation#5: 0x000055aee406c779 cargo-pgrx`cargo_pgrx::command::install::get_git_hash::ha84d504db9d1bba8(manifest_path=0x00007ffdc43b83b0) at install.rs:507:9 frame pgcentralfoundation#6: 0x000055aee406d6e9 cargo-pgrx`cargo_pgrx::command::install::filter_contents::h8c710847129ba6be(manifest_path=0x00007ffdc43b8d10, input=String @ 0x00007ffdc43b8940) at install.rs:541:46 ```

…tralfoundation#1935) I can't quite explain why, but this function was deadlocking with this backtrace: ``` * thread #1, name = 'cargo-pgrx', stop reason = signal SIGSTOP * frame #0: 0x00007fbe1eeb288d libc.so.6`syscall at syscall.S:38 frame #1: 0x000055aee526c5d3 cargo-pgrx`std::sys::sync::mutex::futex::Mutex::lock_contended::h6389e2305b0b005c [inlined] std::sys::pal::unix::futex::futex_wait::h30abf43e2d55aa33 at futex.rs:67:21 frame #2: 0x000055aee526c590 cargo-pgrx`std::sys::sync::mutex::futex::Mutex::lock_contended::h6389e2305b0b005c at futex.rs:57:13 frame #3: 0x000055aee4157835 cargo-pgrx`std::sync::mutex::Mutex$LT$T$GT$::lock::h4d2bb65800cc6fd3 at futex.rs:29:13 frame pgcentralfoundation#4: 0x000055aee41577ed cargo-pgrx`std::sync::mutex::Mutex$LT$T$GT$::lock::h4d2bb65800cc6fd3(self=0x000055aee6926c20) at mutex.rs:317:24 frame pgcentralfoundation#5: 0x000055aee406c779 cargo-pgrx`cargo_pgrx::command::install::get_git_hash::ha84d504db9d1bba8(manifest_path=0x00007ffdc43b83b0) at install.rs:507:9 frame pgcentralfoundation#6: 0x000055aee406d6e9 cargo-pgrx`cargo_pgrx::command::install::filter_contents::h8c710847129ba6be(manifest_path=0x00007ffdc43b8d10, input=String @ 0x00007ffdc43b8940) at install.rs:541:46 ```

eeeebbbbrrrr commented Jul 19, 2023

View reviewed changes

workingjubilee reviewed Jul 19, 2023

View reviewed changes

	// a query like "SELECT 1 LIMIT 0" is a valid "select"-style query that will not produce
	// a SPI_tuptable. So are utility queries such as "CREATE INDEX" or "VACUUM". We might
	// think that in the latter cases we'd want to produce an error here, but there's no
	// way to distinguish from the former. As such, we take a gentle approach and
	// processed with "no, we don't have one, but it's okay"

Conversation

eeeebbbbrrrr commented Jul 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

workingjubilee Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

workingjubilee Jul 19, 2023 •

edited

Loading