Fix issue #1209

[eeeebbbbrrrr]

Cleanup lifetimes such that a Spi::connect() block can't return things that should be borrowed from the lifetime of that SPI connection, which includes cursors (SpiCursor).

This causes the test from #1209 to not compile, which is the desired result, with:

error[E0515]: cannot return value referencing temporary value
   --> pgrx-tests/src/tests/spi_tests.rs:516:13
    |
516 |               c.open_cursor("select 'hello' from generate_series(1, 10000)", None)
    |               ^-------------------------------------------------------------------
    |               |
    |  _____________temporary value created here
    | |
517 | |                 .fetch(10000)
518 | |                 .unwrap()
    | |_________________________^ returns a value referencing data owned by the current function
    |
    = help: use `.collect()` to allocate the iterator

error[E0515]: cannot return value referencing function parameter `c`
   --> pgrx-tests/src/tests/spi_tests.rs:516:13
    |
516 |               c.open_cursor("select 'hello' from generate_series(1, 10000)", None)
    |               ^-------------------------------------------------------------------
    |               |
    |  _____________`c` is borrowed here
    | |
517 | |                 .fetch(10000)
518 | |                 .unwrap()
    | |_________________________^ returns a value referencing data owned by the current function
    |
    = help: use `.collect()` to allocate the iterator

error[E0515]: cannot return value referencing temporary value
   --> pgrx-tests/src/tests/spi_tests.rs:523:30
    |
523 |             Spi::connect(|c| c.open_cursor("select 1", None).fetch(1).unwrap());
    |                              -------------------------------^^^^^^^^^^^^^^^^^^
    |                              |
    |                              returns a value referencing data owned by the current function
    |                              temporary value created here
    |
    = help: use `.collect()` to allocate the iterator

error[E0515]: cannot return value referencing function parameter `c`
   --> pgrx-tests/src/tests/spi_tests.rs:523:30
    |
523 |             Spi::connect(|c| c.open_cursor("select 1", None).fetch(1).unwrap());
    |                              -------------------------------^^^^^^^^^^^^^^^^^^
    |                              |
    |                              returns a value referencing data owned by the current function
    |                              `c` is borrowed here
    |
    = help: use `.collect()` to allocate the iterator

I am not sure how to write a test with pgrx' test suite to ensure something doesn't compile. That'll be easy to do over in PL/Rust, but I feel like we should have some kind of regression test around this here. Thoughts @thomcc or @workingjubilee?

[workingjubilee]

TIME FOR COMPILETEST

[workingjubilee]

@eeeebbbbrrrr The library of choice to test what you want to test is https://docs.rs/ui_test/latest/ui_test/ and it enables what are called "UI tests" by rustc's testing infrastructure. I said "compiletest" because it is a revised version of https://github.com/rust-lang/rust/tree/1b3e68692592d71938df8e7fd8e53fbe5e7ef58c/src/tools/compiletest

[david-monroe]

Unfortunately I don't think it is that simple.

1. SpiHeapTupleData is also just a pointer to allocated data that is freed by SPI_finish.

2. There is impl<'a> FromDatum for &'a str which I think allows you to get a &'static str pointing into temporary data via

       pub fn get<T: IntoDatum + FromDatum>(&self, ordinal: usize) -> Result<Option<T>> {

   FromDatum should likely have a lifetime parameter similar to Deserialize:

   impl<'de: 'a, 'a> Deserialize<'de> for &'a str {

You should do a thorough analysis of lifetimes in this crate.

[workingjubilee]

We have indeed been doing that. I have been working on various possible solutions for actually guaranteeing type safety over FFI. Unfortunately it's very hard to get someone who both:

• is very familiar with the "objective lifetimes" of Postgres
• is very familiar with how Rust's lifetime system works

And it's hard to simply fuse two people together, so while Eric knows the former and I know the latter, we can't necessarily do a Vulcan mind meld and even if we did, there's a lot of code to review and fix.

As you might have noticed, approximately everything implements FromDatum and it is a bound for everything thus it is used everywhere so "just adding a lifetime parameter" probably is close to "rewrite the entire 34000 lines of pgrx, pgrx-macros, and pgrx-sql-entity-graph".

[eeeebbbbrrrr]

Unfortunately I don't think it is that simple.

1. SpiHeapTupleData is also just a pointer to allocated data that is freed by SPI_finish.

There's a follow-up commit here that addresses this.

[eeeebbbbrrrr]

I want to spend a little more time analyzing this, but I think this PR is now catching the things.

It looks like there were two problems here:

• not enough of the Spi types and functions have the proper (any?) lifetime annotations to ensure their instances or return values all associated with the surrounding Spi::connect() lifetime.

• SpiHeapTupleDataEntry::value() is allocating the "FromDatum" version of the backing Datum in the wrong Postgres MemoryContext

[workingjubilee]

Non-blocking, I think.

[workingjubilee]

Do we later use this to access data allocated past the SPITubleTable, or does it alias with anything else? I notice it doesn't get a lot of action (directly) in our test infra, so I'm wondering if the new &'conn mut will break existing user code.

[eeeebbbbrrrr]

Being an implementation detail, I'm unclear how it would break existing user code?

If that thing were to leak out to the public API tho, at least now it has a proper lifetime and might affect compilation, which we'd want, yeah?

[workingjubilee]

There's a few details where internal changes can actually leak out due to Rust unfortunately inferring certain information that is otherwise fairly laborious to annotate on every single type ever, and I can never off-handedly remember exactly which things get affected by this. I think *mut T to &'a mut T is usually nonbreaking, however, it's &'a T to &'a mut T that reliably gotchas people.

This is sometimes referred to as the "perfect derive problem" or "semver hazards": https://github.com/rust-lang/lang-team/blob/master/design-meeting-minutes/2022-04-12-implied-bounds-and-perfect-derive.md

[eeeebbbbrrrr]

Jubilee, is there an action item here? I’m not really sure.

[workingjubilee]

No, I don't think so, I'm just noting it as something to keep an eye on and to try to write more tests that exercise the more niche points of pgrx::spi.

[workingjubilee]

Well, there was the question about aliasing as usual, and also about range of access, given that I'm not immediately sure if pg_sys::SPITubleTable is like ArrayType or not.

[thomcc]

Range of access doesn't really matter. Stacked borrowed forbids it, but treed-borrows allows &mut Header with trailing data, and I've been told that even though TB is likely to be replaced, that problem will certainly remain fixed in the replacement.

IOW, this only matters for running in miri (under default settings), which isn't a concern.

[workingjubilee]

As you might have noticed, approximately everything implements FromDatum and it is a bound for everything thus it is used everywhere so "just adding a lifetime parameter" probably is close to "rewrite the entire 34000 lines of pgrx, pgrx-macros, and pgrx-sql-entity-graph".

Which, to be clear, is what is going to happen, effectively.

not enough of the Spi types and functions have the proper (any?) lifetime annotations to ensure their instances or return values all associated with the surrounding Spi::connect() lifetime.

I have some questions about those.

[eeeebbbbrrrr]

I want to spend a little more time analyzing this, but I think this PR is now catching the things.

It looks like there were two problems here:

• not enough of the Spi types and functions have the proper (any?) lifetime annotations to ensure their instances or return values all associated with the surrounding Spi::connect() lifetime.
• SpiHeapTupleDataEntry::value() is allocating the "FromDatum" version of the backing Datum in the wrong Postgres MemoryContext

To amend the first bullet point there... The SpiClient that Spi::connect() gives the closure needs to be an anonymous lifetime, not one dictated by the caller, which is what I first did in 67aad55.

This stuff is incredibly complex.

Our intention here was always that any Spi-related things created within the scope of Spi::connect() cannot escape that scope. The only thing that can are returned Datums, which we have special support for to allocate in the parent memory context. That's essentially point two above in that we were missing that in one place.

I want to think on this a bit more before we merge, but I'm feeling pretty good about it now.

[eeeebbbbrrrr]

Jubilee, is there an action item here? I’m not really sure.

[workingjubilee]

I believe this is an improvement and should resolve the immediate problem.

[workingjubilee]

This is making me realize that I don't understand why Query is its own trait instead of being some kind of extension trait on SpiClient, if not to specifically help with this kind of unsound pattern, because it breaks the core lifetime relationship and is implemented the same way in all(?) cases.

However, I am not going to think about that too hard right now as... well, it's a preexisting oddity.