Modularize pgrx::spi #1219

workingjubilee · 2023-07-18T16:03:18Z

No description provided.

No functional changes. It was just making it too hard to read and think.

workingjubilee · 2023-07-18T16:05:32Z

pgrx/src/spi/tuple.rs

+impl<'conn> SpiTupleTable<'conn> {
+    /// `SpiTupleTable`s are positioned before the start, for iteration purposes.
+    ///
+    /// This method moves the position to the first row.  If there are no rows, this
+    /// method will silently return.
+    pub fn first(mut self) -> Self {
+        self.current = 0;
+        self
+    }
+
+    /// Restore the state of iteration back to before the start.
+    ///
+    /// This is useful to iterate the table multiple times
+    pub fn rewind(mut self) -> Self {
+        self.current = -1;
+        self
+    }


Suggested change

impl<'conn> SpiTupleTable<'conn> {

/// `SpiTupleTable`s are positioned before the start, for iteration purposes.

///

/// This method moves the position to the first row. If there are no rows, this

/// method will silently return.

pub fn first(mut self) -> Self {

self.current = 0;

self

}

/// Restore the state of iteration back to before the start.

///

/// This is useful to iterate the table multiple times

pub fn rewind(mut self) -> Self {

self.current = -1;

self

}

impl<'conn> SpiTupleTable<'conn> {

/// `SpiTupleTable`s are positioned before the start, for iteration purposes.

///

/// This method moves the position to the first row. If there are no rows, this

/// method will silently return.

pub fn first(mut self) -> Self {

self.current = 0;

self

}

/// Restore the state of iteration back to before the start.

///

/// This is useful to iterate the table multiple times

pub fn rewind(mut self) -> Self {

self.current = -1;

self

}

These functions should probably instead use &mut self. They could still return &mut Self if we really like that style of API.

workingjubilee · 2023-07-18T16:06:13Z

pgrx/src/spi/tuple.rs

+impl<'conn> Iterator for SpiTupleTable<'conn> {
+    type Item = SpiHeapTupleData<'conn>;
+
+    /// # Panics
+    ///
+    /// This method will panic if for some reason the underlying heap tuple cannot be retrieved
+    #[inline]
+    fn next(&mut self) -> Option<Self::Item> {
+        self.current += 1;
+        if self.current >= self.size as isize {
+            None
+        } else {
+            assert!(self.current >= 0);
+            self.get_heap_tuple().report()
+        }
+    }
+
+    #[inline]
+    fn size_hint(&self) -> (usize, Option<usize>) {
+        (0, Some(self.size))
+    }
+
+    #[inline]
+    fn count(self) -> usize
+    where
+        Self: Sized,
+    {
+        self.size
+    }
+}


I am not confident that it's appropriate for this type to implement Iterator itself.

L306 seems wrong, at least.

Why do you not think it's appropriate? It's fairly natural to work with the results of a query as a forward-only stream of rows.

Because it allows rewinding, so it's not very "forward-only".

Also good catch.

Because it allows rewinding, so it's not very "forward-only".

It only allows rewinding because it can. I agree that's definitely not an inherent property of "a forward-only stream of rows".

It's usually better to implement IntoIterator if it normally allows going backwards like that.

workingjubilee · 2023-07-18T16:07:55Z

pgrx/src/spi/tuple.rs

+}
+
+impl<'conn> SpiHeapTupleDataEntry<'conn> {
+    pub fn value<T: IntoDatum + FromDatum>(&self) -> SpiResult<Option<T>> {


Part of the problem really does go down all the way here, where it becomes possible to extract types with only FromDatum as the effective boundary on them.

workingjubilee · 2023-07-18T16:11:57Z

pgrx/src/spi/cursor.rs

+pub struct SpiCursor<'client> {
+    pub(crate) ptr: NonNull<pg_sys::PortalData>,
+    pub(crate) __marker: PhantomData<&'client SpiClient<'client>>,
+}


It's not entirely obvious to me why this is the lifetime marker instead of e.g. only allowing access to SpiCursor via borrowing it from an SpiClient.

In general, pgrx::spi has a problem where the historical implementations were either slightly clumsy, which is to be expected, or too clever by half. This is a bad combination, as it became "idiomatic", as it were, to follow the existing codebase style and also construct more peculiar implementations that rely on either unnecessary or over-clever idioms, instead of trying to use a much more direct and "boring" approach. We'd still have to fix things either way, however, so it's not worth dwelling on.

workingjubilee · 2023-07-18T16:30:28Z

pgrx/src/spi/client.rs

+        // SPI_connect() is documented as being able to return SPI_ERROR_CONNECT, so we have to
+        // assume it could.  The truth seems to be that it never actually does.  The one user
+        // of SpiConnection::connect() returns `spi::Result` anyways, so it's no big deal


It used to do so in the "already connected" case, but now yeah, it just assumes you might call it recursively, and uses an internal stack.

eeeebbbbrrrr

CI seems to like this as much as it likes anything these days. Let’s merge

eeeebbbbrrrr · 2023-07-18T17:08:21Z

pgrx/src/spi.rs

@@ -182,6 +188,8 @@ pub enum Error {
    NoTupleTable,
 }

+pub type Error = SpiError;


It is almost certainly wrong.

@eeeebbbbrrrr

This is the third beta in the pgrx v0.10.x series. It contains a number of soundness fixes, better error handling, more testing, and other general code cleanup. ## Soundness Issues * `AnyNumeric` is no longer backed by Postgres-allocated memory by @eeeebbbbrrrr in #1216 ## CI and general Testing Support * Testing help by @eeeebbbbrrrr in #1203 * Type testability cleanup by @eeeebbbbrrrr in #1204 * Type roundtrip tests by @eeeebbbbrrrr in #1185 * Stop SpiClient soundness from regressing by @workingjubilee in #1214 * Initial valgrind support by @thomcc in #1218 * Add a env flag that can be set to skip `#[pg_test]`-generated tests. by @thomcc in #1239 * Ignores UI tests for MUSL environments by @BradyBonnette in #1235 * Changes GHA workflows to use new upgraded runners by @BradyBonnette in #1225 ## General Improvements * Add support for handling SIGINT and SIGCHLD from bgworker by @JelteF in #1229 * Fix issue #1076: Properly handle dependency graph of `Result<T, _>` by @eeeebbbbrrrr in #1241 ## Improved Error Reporting * Try to smartly propagate fs errors by @workingjubilee in #1186 * Addresses cargo-pgrx error reporting by @BradyBonnette in #1238 * Cleanup the error when cargo-pgrx version doesn't match Cargo.toml by @eeeebbbbrrrr in #1240 ## Additional Postgres Headers * Add operator and cache related api by @VoVAllen in #1242 * Add foreign table headers by @workingjubilee in #1226 * Add postmaster related api by @JelteF in #1237 ## Internal Code Organization * Modularize pgrx::spi by @workingjubilee in #1219 * Modularize the interior of pgrx-pg-sys by @workingjubilee in #1227 ## Postgres 16-motivated Changes * Add a workaround for the pg16/homebrew/icu4c situation by @thomcc in #1206 ## General Project Stuff * Add security policy by @johnrballard in #1207 ## New Contributors * @johnrballard made their first contribution in #1207 * @VoVAllen made their first contribution in #1242 **Full Changelog**: v0.10.0-beta.1...v0.10.0-beta.2

@workingjubilee

This is the final release of v0.10.0. Thanks everyone for the beta testing, pull requests, issues, and patience. As always, install `cargo-pgrx` with `cargo install cargo-pgrx --locked` and update your extension Cargo.toml files to use the `0.10.0` pgrx dependencies. This release includes support for Postgres 16RC1. Support for the previous betas has been removed. As such, a fresh `cargo pgrx init` is required. ## What's Changed Since v0.10.0-beta.4 * Fix `GetMemoryChunkContext` port by @workingjubilee in #1273 * Better error messages when `pg_config` isn't found. by @eeeebbbbrrrr in #1271 * Make `PostgresHash` also need `Eq` by @workingjubilee in #1264 * Memoize git hash and extension metadata by @levkk in #1274 * move to pg16rc1 by @eeeebbbbrrrr in #1276 * Fix bgworker template up to 0.10.0-beta.4 by @workingjubilee in #1270 ## New Contributors * @levkk made their first contribution in #1274 **Changelog**: v0.10.0-beta.4...v0.10.0 --- v0.10.0's full set of changes throughout the entire beta period are: * Postgres 16beta1 Support by @eeeebbbbrrrr in #1169 * Support building against macOS universal binaries by @clowder in #1166 * list specific versions in feature gates by @eeeebbbbrrrr in #1175 * Fix bug with converting a `pg_sys::Datum` into a `pgrx::Date` by @eeeebbbbrrrr in #1177 * Fix Arrays with leading nulls by @eeeebbbbrrrr in #1180 * Disable hello_versioned_so test by @workingjubilee in #1192 * doc: fix link broken by @yihong0618 in #1181 * fcinfo: fix incorrect length set in unsafe code by @Sasasu in #1190 * update to pg16beta2 support by @eeeebbbbrrrr in #1188 * Array-walking is aligned by @workingjubilee in #1191 * Implement PGRXSharedMemory for Deque by @feikesteenbergen in #1170 * Include security labels header by @daamien in #1189 * Fixes macos-11 tests by @BradyBonnette in #1197 * Pgcentralfoundation updates again by @eeeebbbbrrrr in #1200 * Update version to 0.10.0-beta.0 by @eeeebbbbrrrr in #1201 * Testing help by @eeeebbbbrrrr in #1203 * Type testability cleanup by @eeeebbbbrrrr in #1204 * Try to smartly propagate fs errors by @workingjubilee in #1186 * Fix issue #1209 by @eeeebbbbrrrr in #1210 * Type roundtrip tests by @eeeebbbbrrrr in #1185 * Update version to 0.10.0-beta.1 by @eeeebbbbrrrr in #1213 * Add a workaround for the pg16/homebrew/icu4c situation by @thomcc in #1206 * Add security policy by @johnrballard in #1207 * `AnyNumeric` is no longer backed by Postgres-allocated memory by @eeeebbbbrrrr in #1216 * Modularize pgrx::spi by @workingjubilee in #1219 * Stop SpiClient soundness from regressing by @workingjubilee in #1214 * Add foreign table headers by @workingjubilee in #1226 * Modularize the interior of pgrx-pg-sys by @workingjubilee in #1227 * Initial valgrind support by @thomcc in #1218 * Add support for handling SIGINT and SIGCHLD from bgworker by @JelteF in #1229 * Ignores UI tests for MUSL environments by @BradyBonnette in #1235 * Add a env flag that can be set to skip `#[pg_test]`-generated tests. by @thomcc in #1239 * Fix issue #1076: Properly handle dependency graph of `Result<T, _>` by @eeeebbbbrrrr in #1241 * Cleanup the error when cargo-pgrx version doesn't match Cargo.toml by @eeeebbbbrrrr in #1240 * Add operator and cache related api by @VoVAllen in #1242 * Addresses cargo-pgrx error reporting by @BradyBonnette in #1238 * Update version to 0.10.0-beta.2 by @eeeebbbbrrrr in #1244 * Bump cargo-metadata and clap-cargo by @thomcc in #1246 * Derive Clone for Inet by @JelteF in #1251 * Correct docs for datetime `From` impls by @workingjubilee in #1253 * Only enable line tables for profile.dev by @thomcc in #1249 * Remove references to master branch by @thomcc in #1243 * Ensure bindgen gets all the `cppflags` it needs (on macOS, anyway) by @thomcc in #1247 * update for pg16beta3 support by @eeeebbbbrrrr in #1254 * Update version to 0.10.0-beta.3 by @eeeebbbbrrrr in #1255 * Add proptest support by @workingjubilee in #1258 * Misc reformatting and typo fixes by @workingjubilee in #1260 * spi: simplify (optimize?) Datum preparation by @vrmiguel in #1256 * Assume commutation when deriving PostgresEq by @workingjubilee in #1261 * Demand Ord for PostgresOrd by @workingjubilee in #1262 * Fix pgrx install causing postgresql coredump by @Sasasu in #1263 * Update version to 0.10.0-beta.4 by @workingjubilee in #1267 ## New Contributors * @clowder made their first contribution in #1166 * @yihong0618 made their first contribution in #1181 * @Sasasu made their first contribution in #1190 * @daamien made their first contribution in #1189 * @johnrballard made their first contribution in #1207 * @VoVAllen made their first contribution in #1242 * @vrmiguel made their first contribution in #1256 **Full Changelog**: v0.9.8...v0.10.0

workingjubilee added 4 commits July 17, 2023 14:12

Remove unnecessary abstraction in Spi

f859fdc

No functional changes. It was just making it too hard to read and think.

Push out SpiClient

01cddea

Modularize pgrx::spi

796f56c

Unify SpiClient impls

d2eeea8

workingjubilee commented Jul 18, 2023

View reviewed changes

eeeebbbbrrrr approved these changes Jul 18, 2023

View reviewed changes

pgrx/src/spi.rs

@@ -182,6 +188,8 @@ pub enum Error {

NoTupleTable,

}

pub type Error = SpiError;

Copy link

Contributor

eeeebbbbrrrr Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

workingjubilee added 4 commits July 18, 2023 10:14

Remove count() from SpiTupleTable

da580b1

It is almost certainly wrong.

Merge imports in spi

70b68ff

Use SpiResult in spi/tuple.rs

ca148f1

fmt

4092989

workingjubilee merged commit d333053 into pgcentralfoundation:develop Jul 18, 2023

workingjubilee deleted the clean-up-spi branch July 25, 2023 03:27

eeeebbbbrrrr mentioned this pull request Aug 1, 2023

Update version to 0.10.0-beta.2 #1244

Merged

eeeebbbbrrrr mentioned this pull request Sep 5, 2023

Update version to 0.10.0 #1280

Merged

bayandin mentioned this pull request Sep 6, 2023

pgrx 0.10.0 bayandin/homebrew-tap#36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modularize pgrx::spi #1219

Modularize pgrx::spi #1219

workingjubilee commented Jul 18, 2023

workingjubilee Jul 18, 2023

eeeebbbbrrrr Jul 18, 2023

workingjubilee Jul 18, 2023

eeeebbbbrrrr Jul 18, 2023

workingjubilee Jul 18, 2023

workingjubilee Jul 18, 2023

eeeebbbbrrrr Jul 18, 2023

workingjubilee Jul 18, 2023

workingjubilee Jul 18, 2023

workingjubilee Jul 18, 2023

workingjubilee Jul 18, 2023

workingjubilee Jul 18, 2023

eeeebbbbrrrr left a comment

eeeebbbbrrrr Jul 18, 2023

Modularize pgrx::spi #1219

Modularize pgrx::spi #1219

Conversation

workingjubilee commented Jul 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eeeebbbbrrrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment