`SpiClient::select` not seeing changes from `Spi::run` in same transaction

[EdMcBane]

This issue affects the current develop branch.

Queries executed with SpiClient::select do not see data changes applied within the same transaction by update queries executed using Spi::run.

E.g. this fails, as with_select is zero, instead of 1 as expected:

    #[pg_test]
    fn test_spi_select_sees_run() -> spi::Result<()> {
        Spi::run("CREATE TABLE asd(id int)")?;
        Spi::run("INSERT INTO asd(id) VALUES (1)")?;
        let with_select = Spi::connect(|client| {
            client.select("SELECT COUNT(*) FROM asd", None, None)?
                .first().get_one::<i64>()
        })?;
        let with_get_one = Spi::get_one::<i64>("SELECT COUNT(*) FROM asd")?;

        assert_eq!(with_select, with_get_one);
        Ok(())
    }

Using SpiClient::update in the same session works:

    #[pg_test]
    fn test_spi_select_sees_update() -> spi::Result<()> {
        let with_select = Spi::connect(|mut client| {
            client.update("CREATE TABLE asd(id int)", None, None)?;
            client.update("INSERT INTO asd(id) VALUES (1)", None, None)?;
            client.select("SELECT COUNT(*) FROM asd", None, None)?
                .first().get_one::<i64>()
        })?;
        let with_get_one = Spi::get_one::<i64>("SELECT COUNT(*) FROM asd")?;

        assert_eq!(with_select, with_get_one);
        Ok(())
    }

Using SpiClient::update in a separate session does not work:

    #[pg_test]
    fn test_spi_select_sees_update_in_other_session() -> spi::Result<()> {
        Spi::connect::<spi::Result<()>, _>(|mut client| {
            client.update("CREATE TABLE asd(id int)", None, None)?;
            client.update("INSERT INTO asd(id) VALUES (1)", None, None)?;
            Ok(())
        })?;
        let with_select = Spi::connect(|client| {
            client.select("SELECT COUNT(*) FROM asd", None, None)?
                .first().get_one::<i64>()
        })?;
        let with_get_one = Spi::get_one::<i64>("SELECT COUNT(*) FROM asd")?;

        assert_eq!(with_select, with_get_one);
        Ok(())
    }

[EdMcBane]

The issue appears to be the new way of handling the readonly flag in SpiClient.

The flag starts as true and gets set to false as soon as an update is performed using e.g. Spi::update.

Unfortunately this only works within the same Spi::connect session, showing very confusing behavior for the common case of using multiple Spi::connect sessions in the same transaction, and even more so if using the Spi facade methods such as Spi::run.

As the postgres docs say, and was reported in the docs of SpiClient::readonly:

It is generally unwise to mix read-only and read-write commands within a single function
using SPI; that could result in very confusing behavior, since the read-only queries
would not see the results of any database updates done by the read-write queries.

I believe the flag should either be managed at the transaction level (so ensuring consistency of visibility regardless of Spi session scope), but this could prove very difficult, or removed altogether, reverting to the previous behavior of always passing readonly=false.

[eeeebbbbrrrr]

After some experimentation and looking through the Postgres sources for inspiration, this seems to fix things:

impl<'conn> Drop for SpiClient<'conn> {
    fn drop(&mut self) {
        if self.readonly == false {
            // Assume the user actually modified the database during this SpiClient's lifetime
            // and push a new snapshot with an updated CommandId.  This ensures that subsequent 
            // `readonly == true` SpiClients will see the changes made by this one
            unsafe {
                pg_sys::PushCopiedSnapshot(pg_sys::GetActiveSnapshot());
                pg_sys::UpdateActiveSnapshotCommandId();
            }
        }
    }
}

Thoughts?

[yrashk]

@eeeebbbbrrrr I think it's an interesting workaround, indeed!

[eeeebbbbrrrr]

This was fixed in pr #984. Closing this issue.

[eeeebbbbrrrr]

@eeeebbbbrrrr I think it's an interesting workaround, indeed!

I don't think it's a "workaround" per se. After going through parts of the Postgres sources I think it's the right way to handle this. Postgres does the exact pattern in quite a few places with similar comments. We just missed it in #963 because we're not Tom Lane... and that's a really good excuse.