Implement `RetAbi` and `BoxRet` #1701

workingjubilee · 2024-05-14T21:17:44Z

This completely reworks the way we handle returning types from functions, so that we no longer have to rely on a macro expansion behavior that has to already know the types at expansion time (and thus has to parse them somehow, which it cannot realistically do because type aliases exist). Instead, we simply expand into code that asks the types themselves to modify the FunctionCallInfo appropriately and then return a raw Datum to Postgres.

This breaks support for certain returns because it was difficult to do this and also support arbitrary nesting, because Postgres does not support arbitrary nesting. For instance, you can no longer return:

SetOfIterator<'a, Result<T, E>>
TableIterator<'a, (Result<T, E>, Result<U, D>)>
Option<TableIterator<'a, Tuple>>

It's expected that this will improve in the near-ish future.

This also breaks returning values from #[pg_extern] functions that were relying on IntoDatum implementations being enough. It is not expected this will improve, for the reasons described on the documentation of the new traits, which can be summarized as "IntoDatum should never have been used for that bound". This change blocks off several latent correctness problems from affecting pgrx going forward.

Fixes #1484

pgrx/src/callconv.rs

pgrx/src/iter.rs

workingjubilee

As the iterator conveniences are basically independent, I'll rebase this on #1703 after that lands.

workingjubilee · 2024-05-15T01:52:29Z

pgrx-tests/src/tests/srf_tests.rs

+    ) -> Result<TableIterator<'static, (name!(id, i32), name!(relname, Option<String>))>, spi::Error>
    {
        let oids = vec![1213, 1214, 1232, 1233, 1247, 1249, 1255];
-
-        TableIterator::new(oids.into_iter().map(|oid| {
-            (oid, Spi::get_one(&format!("SELECT CAUSE_AN_ERROR FROM pg_class WHERE oid = {oid}")))
-        }))
+        let result = oids
+            .into_iter()
+            .map(|oid| {
+                Ok((
+                    oid,
+                    Spi::get_one(&format!(
+                        "SELECT CAUSE_AN_ERROR FROM pg_class WHERE oid = {oid}"
+                    ))?,
+                ))
+            })
+            .collect::<Result<Vec<_>, _>>();
+        result.map(|v| TableIterator::new(v))


So, this should be TableIterator<'_, Result<Tuple, Error>> so that no eager evaluation has to happen. I think I can make that work with a bit more effort. But the schema generator chokes on that! And that is another reason why I want it out of our way.

I do not think people should be required to return TableIterator<'a, (Result, Result, Result, Result, Result, Result)> if they want to fallibly return a row, but they currently are. And I do not think this should be blocked on that.

Asking people to propagate errors to the top of each row return should be fine.

Asking people to propagate errors to the top of each row return should be fine.

I'm not sure this is a nice thing to ask. When SPI, for example, is involved, and what you're trying to do is return a TableIterator of a bunch of individual values you somehow got from SPI, it's definitely a lot easier to just return the Result<T> (or Result<Option<T>>) in whatever tuple position. Especially if there's quite a few values in the tuple.

This isn't a hill I'm willing to die on, but it'd be nice to support, for example, TableIterator<(Result<S>, T, Result<U>, Result<V>, W, Result<X>, Y, Result<Z>)>. Then we'll panic whenever we get to unwrapping that result and the user doesn't have to worry about it.

@eeeebbbbrrrr Honestly, we'll never have good UX until we expand to supporting actual struct arguments, really.

What does "actual struct arguments" mean?

TableIterator should be able to return a struct that describes the row in question, but this requires being able to use traits like these to return fairly arbitrary generic types. It doesn't work as long as we rely on parsing.

pgrx-examples/spi_srf/src/lib.rs

workingjubilee · 2024-05-15T05:25:41Z

This PR opens a LOT of pure simplifications like 9dd8f5f, it's just going to take forever to find them all.

workingjubilee · 2024-05-15T16:53:07Z

pgrx-examples/spi_srf/src/lib.rs

-            .collect::<Vec<_>>();
-        Ok(TableIterator::new(filtered))
+            .map(|row| {
+                Ok((row["dog_name"].value()?, row["dog_age"].value()?, row["dog_breed"].value()?))


@eeeebbbbrrrr it's certainly not less code but in most cases it just looks like going "hello? I? have? a? question? if that's okay?"

It's not terrible, but I'm sure you agree that returning a Result from a closure is awkward at best. In this case we .collect::<Result<Vec<_>>>() so it's not too terrible.

I prefer the left side of this diff, but like I said elsewhere, I'm not willing to die on this hill.

yeah, the left side is a bit nicer, but that type signature... that's what I really have it in for. so yeah, if I didn't believe it would open up better possibilities in short order, I wouldn't do it.

that's what I really have it in for

Yeah, that's why I'm not really gonna fight on this one. I hate typing out all that mess too. But once the return signature has been typed out, it makes it a lot easier to just write some code and return some values.

Many question marks is much-preferable to having excessively-verbose nested type signatures, yeah.

tests::heap_tuple::tests::pg_test_tuple_desc_clone tests::result_tests::tests::pg_test_return_result_set_of tests::result_tests::tests::pg_test_return_result_set_of_error tests::srf_tests::tests::pg_test_generate_series tests::srf_tests::tests::pg_test_return_none_setof_iterator tests::srf_tests::tests::pg_test_return_some_setof_iterator tests::srf_tests::tests::pg_test_srf_setof_datum_detoasting_with_borrow tests::struct_type_tests::tests::pg_test_complex_storage_and_retrieval

They now know how to create their own tupdesc in the context.

workingjubilee · 2024-05-17T22:49:58Z

pgrx-tests/src/tests/srf_tests.rs

+    pub fn spi_in_setof() -> Result<SetOfIterator<'static, Option<String>>, spi::Error> {
        let oids = vec![1213, 1214, 1232, 1233, 1247, 1249, 1255];
-
-        SetOfIterator::new(oids.into_iter().map(|oid| {
-            Spi::get_one(&format!("SELECT CAUSE_AN_ERROR FROM pg_class WHERE oid = {oid}"))
-        }))
+        let result = oids
+            .into_iter()
+            .map(|oid| {
+                Spi::get_one(&format!("SELECT CAUSE_AN_ERROR FROM pg_class WHERE oid = {oid}"))
+            })
+            .collect::<Result<Vec<Option<_>>, _>>();
+        result.map(SetOfIterator::new)


Breaking this one annoys me much more than the TableIterator nesting case but I want to make sure that SetOfIterator has exactly as stringent requirements, currently, as TableIterator<'a, (T,)>, because it will make things much worse down the line if they meaningfully differ.

NotGyro

Looks good to me. This really does make code working with the returns from Rust to Postgres much cleaner and more elegant.

I had some confusion about the SetOfIterator type, but it was cleared up in a company meeting. The single-item case behaves differently than the iterator case for legacy Postgres reasons, which is why SetOfIterator can be one item and/or an iterator.

NotGyro

Thanks for the documentation! This explains it neatly.

This allows hiding the details of the type so it cannot be constructed nor meddled with externally, and simplifies many actual cases.

workingjubilee · 2024-05-20T22:15:51Z

This last commit makes the "Ret" type into an associated type and thus makes the iterators eat all the complexity of their return ABI as now only they have to know about how to drive themselves forward. Result does not have to think about them, and the implementation details are once more private to iterators and fully hidden behind the trait. This prevents anyone from trying to make any of the many bug-inducing nonsensical calls that were technically allowed at first, and probably made the code have to carry around way more panic branches.

idk it just bugged me.

workingjubilee · 2024-05-20T23:40:26Z

The last few commits were just cleanup.

workingjubilee force-pushed the impl-return-shipping branch from 1c7e5bc to 6bf40f1 Compare May 14, 2024 21:21

workingjubilee commented May 14, 2024

View reviewed changes

pgrx/src/callconv.rs Outdated Show resolved Hide resolved

pgrx/src/callconv.rs Outdated Show resolved Hide resolved

pgrx/src/iter.rs Outdated Show resolved Hide resolved

pgrx/src/iter.rs Outdated Show resolved Hide resolved

pgrx/src/iter.rs Outdated Show resolved Hide resolved

workingjubilee commented May 14, 2024

View reviewed changes

pgrx/src/iter.rs Outdated Show resolved Hide resolved

workingjubilee mentioned this pull request May 15, 2024

Add one-or-none iterator conveniences #1703

Merged

workingjubilee force-pushed the impl-return-shipping branch from eaa3633 to a55501b Compare May 15, 2024 01:36

workingjubilee commented May 15, 2024

View reviewed changes

workingjubilee force-pushed the impl-return-shipping branch from bd44aaa to 9dd8f5f Compare May 15, 2024 02:55

workingjubilee marked this pull request as ready for review May 15, 2024 05:23

workingjubilee commented May 15, 2024

View reviewed changes

workingjubilee added 20 commits May 15, 2024 13:17

partial

0798d6a

another partial

7b5ac49

Refactor a bunch of stuff

3652e1f

extract a wrapper for srfs

450bb2b

start work on the wrapper fn

d9a3ae4

check compiles proc macros but does not finish building pg_externs

ed4fb5c

check passes

ab79662

almost builds all tests

7f1a334

explicitly impl BoxRet for &str

d049e7b

type inference defeated

12daefb

Everything except composite types

8182fae

silence deprecations

9c86b83

remove nonsense

f900ec9

ignore unused

c2cfa94

fix 7 tests

07caff4

fix last tests

0ba6f91

Convert scalar values to single return strat

65f0a48

and unit AKA void returns

a2259ff

its wrapper_code now

cbc239e

Remove psychic powers from tuple returns

d067421

They now know how to create their own tupdesc in the context.

workingjubilee force-pushed the impl-return-shipping branch from 99245aa to d067421 Compare May 17, 2024 21:17

workingjubilee added 4 commits May 17, 2024 14:42

Save the argument ABI change for a later PR

7ea378e

Add some docs

1f924e4

impl SetOfIterator<'_, T> with TableIterator<'_, (T,)>

ea996dd

Break one last test

5492bf8

workingjubilee commented May 17, 2024

View reviewed changes

workingjubilee mentioned this pull request May 17, 2024

Draw the rest of the ̶o̶w̶l̶ new ABI-handling framework #1661

Open

13 tasks

remove the fixed fixme

670c521

workingjubilee requested a review from NotGyro May 17, 2024 23:29

NotGyro approved these changes May 20, 2024

View reviewed changes

workingjubilee added 2 commits May 20, 2024 13:44

Explain history and ABI of 1-column RETURNS TABLE

dbd68ca

Rename TableIterator generic to Row?

93f5808

NotGyro approved these changes May 20, 2024

View reviewed changes

Make Ret into an associated type

fef9f4e

This allows hiding the details of the type so it cannot be constructed nor meddled with externally, and simplifies many actual cases.

workingjubilee added 4 commits May 20, 2024 15:20

Consistently order the Step variants

ac20e7d

idk it just bugged me.

Add some more docs

7417758

more docs more

97cb10f

lint the new code with deny(unsafe_op_in_unsafe_fn)

5941406

workingjubilee merged commit 6bd4f0e into pgcentralfoundation:develop May 20, 2024
12 checks passed

workingjubilee deleted the impl-return-shipping branch May 21, 2024 17:32

my-vegetable-has-exploded mentioned this pull request Jun 1, 2024

Implement aggregate functions for sparse vector. tensorchord/pgvecto.rs#476

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `RetAbi` and `BoxRet` #1701

Implement `RetAbi` and `BoxRet` #1701

workingjubilee commented May 14, 2024 •

edited

workingjubilee left a comment

workingjubilee May 15, 2024

eeeebbbbrrrr May 15, 2024

workingjubilee May 15, 2024

eeeebbbbrrrr May 15, 2024

workingjubilee May 15, 2024 •

edited

workingjubilee commented May 15, 2024

workingjubilee May 15, 2024

eeeebbbbrrrr May 15, 2024

workingjubilee May 15, 2024

eeeebbbbrrrr May 15, 2024 •

edited

NotGyro May 20, 2024

workingjubilee May 17, 2024

NotGyro left a comment •

edited

NotGyro left a comment

workingjubilee commented May 20, 2024

workingjubilee commented May 20, 2024

Implement RetAbi and BoxRet #1701

Implement RetAbi and BoxRet #1701

Conversation

workingjubilee commented May 14, 2024 • edited

workingjubilee left a 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 May 15, 2024 • edited

Choose a reason for hiding this comment

workingjubilee commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eeeebbbbrrrr May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NotGyro left a comment • edited

Choose a reason for hiding this comment

NotGyro left a comment

Choose a reason for hiding this comment

workingjubilee commented May 20, 2024

workingjubilee commented May 20, 2024

Implement `RetAbi` and `BoxRet` #1701

Implement `RetAbi` and `BoxRet` #1701

workingjubilee commented May 14, 2024 •

edited

workingjubilee May 15, 2024 •

edited

eeeebbbbrrrr May 15, 2024 •

edited

NotGyro left a comment •

edited