Add wrappers for postgres geometric types. #1332

hamiltop · 2023-10-12T02:11:12Z

For simple Copy types, just newtype them and add impls for FromDatum and ToDatum.

For more complex variable length types, create zero-copy and owned structs with impls for FromDatum and ToDatum as well as SqlTranslatable.

hamiltop · 2023-10-12T02:13:01Z

pgrx/src/datum/geo.rs

+
+// Copy types
+
+pub type Box = pg_sys::BOX;


In [pgrx/src/datum/date.rs] the pattern is to do a transparent wrapping struct and add From impls. If that's the preferred pattern I can switch these over to that approach.

Give us a few days to look this over.

I don’t know what the right answer is. I think we might prefer a newtype but then that’ll cause complications over in pl/rust land for types that are already exposed, like BOX. That’s not your responsibility of course, buts it’s something we need to take into consideration.

I've been thinking about this a bit more. I think it's fine to leave these as aliases. In fact, it's probably preferred.

A newtype wrapper would imply that pgrx has some more knowledge about the type and, for example, enforces bounds or requires some invariants that we don't.

We basically had to do that for the date/time types b/c even tho the underlying type is something basic like a u32 (or whatever), not all u32s are valid dates or timestamps or whatnot. I don't guess that's the case here.

It looks like Postgres does some validation on the string representation of these types, of course, but it doesn't care to check the actual values.

We basically had to do that for the date/time types b/c even tho the underlying type is something basic like a u32 (or whatever), not all u32s are valid dates or timestamps or whatnot.

Yes, that is exactly the case. The types have implicit invariants that aren't going to be enforced by the "raw" pg_sys side, and lifting them into Rust allowed enforcing invariants. If all pairs of points are valid BOXes (I'm talking only about Postgres's conception of validity), then it's fine to just use an alias.

eeeebbbbrrrr

Thank you very much @hamiltop. This is sincerely appreciated.

In addition to the little nit about prefixing things in pg_sys with pg_sys::, if you could update the table of types in the top-level README.md, that'd be great.

Our documentation is nowhere near where we'd like it to be, but that table is a great quick reference.

eeeebbbbrrrr · 2023-10-12T20:02:12Z

pgrx/src/datum/geo.rs

+impl IntoDatum for Point {
    fn into_datum(mut self) -> Option<pg_sys::Datum> {
        unsafe {
            let copy = PgMemoryContexts::CurrentMemoryContext
-                .copy_ptr_into(&mut self, std::mem::size_of::<pg_sys::Point>());
+                .copy_ptr_into(&mut self, std::mem::size_of::<Point>());


we tend to prefix things in pg_sys with pg_sys::. I can't say we're 100% consistent with that throughout the codebase, but my preference would be to shift towards that direction, not away.

The reason is it makes it a lot easier when reading the code to know "this thing is defined by Postgres", which is almost always an important thing to know about a type when working inside pgrx.

So, if you will, please revert this particular change, and if there's anything else in the PR that elides pg_sys::, please add it.

To be clear, this isn't quite "eliding pg_sys::". This is a change from pg_sys::Point to geo::Point (which happens to be an alias to pg_sys::Point). It's especially confusing here because they are both spelled and capitalized the same way, but other cases it's Box vs pg_sys::BOX and LineSegment vs pg_sys::LSEG.

Also, this particular line could also be rewritten as std::mem::size_of::<Self>.

I do like the more normal type names, especially in place of ALL_CAPS ones, but if pg_sys:: is preferred I can make that change.

This is a change from pg_sys::Point to geo::Point

hmm. yeah, it is.

Also, this particular line could also be rewritten as std::mem::size_of::.

Indeed. Probably good to make that little change.

but if pg_sys:: is preferred I can make that change.

No, this is correct. You're right.

eeeebbbbrrrr · 2023-10-12T20:08:54Z

pgrx/src/datum/geo.rs

+
+// Copy types
+
+pub type Box = pg_sys::BOX;


I've been thinking about this a bit more. I think it's fine to leave these as aliases. In fact, it's probably preferred.

A newtype wrapper would imply that pgrx has some more knowledge about the type and, for example, enforces bounds or requires some invariants that we don't.

We basically had to do that for the date/time types b/c even tho the underlying type is something basic like a u32 (or whatever), not all u32s are valid dates or timestamps or whatnot. I don't guess that's the case here.

It looks like Postgres does some validation on the string representation of these types, of course, but it doesn't care to check the actual values.

workingjubilee

Please split the "zero-copy" types into their own PR. We will need to evaluate them separately as they intersect with part of the MemCx project.

For simple `Copy` types, just alias them and add impls for FromDatum and ToDatum. For more complex variable length types, create owned structs with impls for FromDatum and ToDatum as well as SqlTranslatable.

hamiltop · 2023-10-13T15:20:01Z

@workingjubilee Updated with removal of zero-copy types. Should I wait for MemCx to land before re-introducing the zero-copy types? Sounds like it's close.

workingjubilee · 2023-10-30T21:43:44Z

Not that close, apparently. ^^ Sorry for the delays.

workingjubilee · 2023-10-30T21:44:42Z

pgrx/src/datum/geo.rs

    fn into_datum(mut self) -> Option<pg_sys::Datum> {
        unsafe {
            let ptr = PgMemoryContexts::CurrentMemoryContext
-                .copy_ptr_into(&mut self, std::mem::size_of::<pg_sys::BOX>());
+                .copy_ptr_into(&mut self, std::mem::size_of::<Self>());


While we're here, let's enforce this style:

Suggested change

.copy_ptr_into(&mut self, std::mem::size_of::<Self>());

.copy_ptr_into(&mut self, mem::size_of::<Self>());

workingjubilee · 2023-10-30T21:44:50Z

pgrx/src/datum/geo.rs

+    fn into_datum(mut self) -> Option<pg_sys::Datum> {
+        unsafe {
+            let ptr = PgMemoryContexts::CurrentMemoryContext
+                .copy_ptr_into(&mut self, std::mem::size_of::<Self>());


Suggested change

.copy_ptr_into(&mut self, std::mem::size_of::<Self>());

.copy_ptr_into(&mut self, mem::size_of::<Self>());

workingjubilee · 2023-10-30T21:45:04Z

pgrx/src/datum/geo.rs

+    fn into_datum(mut self) -> Option<pg_sys::Datum> {
+        unsafe {
+            let ptr = PgMemoryContexts::CurrentMemoryContext
+                .copy_ptr_into(&mut self, std::mem::size_of::<Self>());


Suggested change

.copy_ptr_into(&mut self, std::mem::size_of::<Self>());

.copy_ptr_into(&mut self, mem::size_of::<Self>());

workingjubilee · 2023-10-30T21:45:21Z

pgrx/src/datum/geo.rs

+    fn into_datum(mut self) -> Option<pg_sys::Datum> {
+        unsafe {
+            let ptr = PgMemoryContexts::CurrentMemoryContext
+                .copy_ptr_into(&mut self, std::mem::size_of::<Self>());


Suggested change

.copy_ptr_into(&mut self, std::mem::size_of::<Self>());

.copy_ptr_into(&mut self, mem::size_of::<Self>());

workingjubilee · 2023-10-30T21:49:25Z

pgrx-tests/src/tests/geo_tests.rs

+    #[pg_test]
+    fn test_lseg_datum() -> spi::Result<()> {


These tests look good, but can we also get one or two that show the same types being passed around using the fn_call module? I've noticed sometimes there's odd discrepancies between various ways of moving data in/out using literals and SPI, and in/out using other interfaces. You don't have to do all of them, just whichever type seems most quirky.

workingjubilee · 2023-10-30T21:51:43Z

pgrx/src/datum/geo.rs

+
+// Copy types
+
+pub type Box = pg_sys::BOX;


We basically had to do that for the date/time types b/c even tho the underlying type is something basic like a u32 (or whatever), not all u32s are valid dates or timestamps or whatnot.

Yes, that is exactly the case. The types have implicit invariants that aren't going to be enforced by the "raw" pg_sys side, and lifting them into Rust allowed enforcing invariants. If all pairs of points are valid BOXes (I'm talking only about Postgres's conception of validity), then it's fine to just use an alias.

workingjubilee · 2023-10-30T21:52:13Z

pgrx/src/datum/geo.rs

+    }
+}
+
+pub type Point = pg_sys::Point;


All pairs of f64s are valid Points, right?

workingjubilee · 2023-10-30T21:54:31Z

pgrx/src/datum/geo.rs

@@ -72,3 +163,214 @@ impl IntoDatum for pg_sys::Point {
        pg_sys::POINTOID
    }
 }
+
+pub type Circle = pg_sys::CIRCLE;


Same "are there any implicit invariants?" question.

workingjubilee · 2023-12-07T01:11:28Z

A rough draft for MemCx<'mcx> landed in #1342.

hamiltop force-pushed the geometric_types branch from 100a85c to 270f03d Compare October 12, 2023 02:12

hamiltop commented Oct 12, 2023

View reviewed changes

eeeebbbbrrrr requested changes Oct 12, 2023

View reviewed changes

workingjubilee requested changes Oct 12, 2023

View reviewed changes

Add wrappers for postgres geometric types.

b510864

For simple `Copy` types, just alias them and add impls for FromDatum and ToDatum. For more complex variable length types, create owned structs with impls for FromDatum and ToDatum as well as SqlTranslatable.

hamiltop force-pushed the geometric_types branch from 270f03d to b510864 Compare October 13, 2023 15:18

workingjubilee requested changes Oct 30, 2023

View reviewed changes

workingjubilee requested a review from eeeebbbbrrrr October 30, 2023 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wrappers for postgres geometric types. #1332

Add wrappers for postgres geometric types. #1332

hamiltop commented Oct 12, 2023

hamiltop Oct 12, 2023

eeeebbbbrrrr Oct 12, 2023

eeeebbbbrrrr Oct 12, 2023

workingjubilee Oct 30, 2023

eeeebbbbrrrr left a comment

eeeebbbbrrrr Oct 12, 2023

hamiltop Oct 12, 2023 •

edited

Loading

eeeebbbbrrrr Oct 13, 2023

hamiltop Oct 13, 2023

eeeebbbbrrrr Oct 12, 2023

workingjubilee left a comment

hamiltop commented Oct 13, 2023

workingjubilee commented Oct 30, 2023

workingjubilee Oct 30, 2023

workingjubilee Oct 30, 2023

workingjubilee Oct 30, 2023

workingjubilee Oct 30, 2023

workingjubilee Oct 30, 2023

workingjubilee Oct 30, 2023

workingjubilee Oct 30, 2023

workingjubilee Oct 30, 2023

workingjubilee commented Dec 7, 2023

	.copy_ptr_into(&mut self, std::mem::size_of::<Self>());
	.copy_ptr_into(&mut self, mem::size_of::<Self>());

Add wrappers for postgres geometric types. #1332

Are you sure you want to change the base?

Add wrappers for postgres geometric types. #1332

Conversation

hamiltop commented Oct 12, 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

eeeebbbbrrrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamiltop Oct 12, 2023 • edited Loading

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

workingjubilee left a comment

Choose a reason for hiding this comment

hamiltop commented Oct 13, 2023

workingjubilee commented Oct 30, 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

workingjubilee commented Dec 7, 2023

hamiltop Oct 12, 2023 •

edited

Loading