Re: Cross-database SERIALIZABLE safe snapshots

On 09/03/2023 07:34, Thomas Munro wrote:
> Here is a feature idea that emerged from a pgsql-bugs thread[1] that I
> am kicking into the next commitfest. Example:
>
> s1: \c db1
> s1: CREATE TABLE t (i int);
> s1: BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
> s1: INSERT INTO t VALUES (42);
>
> s2: \c db2
> s2: BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE;
> s2: SELECT * FROM x;
>
> I don't know of any reason why s2 should have to wait, or
> alternatively, without DEFERRABLE, why it shouldn't immediately drop
> from SSI to SI (that is, opt out of predicate locking and go faster).
> This change relies on the fact that PostgreSQL doesn't allow any kind
> of cross-database access, except for shared catalogs, and all catalogs
> are already exempt from SSI. I have updated this new version of the
> patch to explain that very clearly at the place where that exemption
> happens, so that future hackers would see that we rely on that fact
> elsewhere if reconsidering that.

Makes sense.

> @@ -1814,7 +1823,17 @@ GetSerializableTransactionSnapshotInt(Snapshot snapshot,
> {
> othersxact = dlist_container(SERIALIZABLEXACT, xactLink, iter.cur);
>
> - if (!SxactIsCommitted(othersxact)
> + /*
> + * We can't possibly have an unsafe conflict with a transaction in
> + * another database. The only possible overlap is on shared
> + * catalogs, but we don't support SSI for shared catalogs. The
> + * invalid database case covers 2PC, because we don't yet record
> + * database OIDs in the 2PC information. We also filter out doomed
> + * transactions as they can't possibly commit.
> + */
> + if ((othersxact->database == InvalidOid ||
> + othersxact->database == MyDatabaseId)
> + && !SxactIsCommitted(othersxact)
> && !SxactIsDoomed(othersxact)
> && !SxactIsReadOnly(othersxact))
> {

Why don't we set the database OID in 2PC transactions? We actually do
set it correctly - or rather we never clear it - when a transaction is
prepared. But you set it to invalid when recovering a prepared
transaction on system startup. So the comment is a bit misleading: the
optimization doesn't apply to 2PC transactions recovered after restart,
other 2PC transactions are fine.

I'm sure it's not a big deal in practice, but it's also not hard to fix.
We do store the database OID in the twophase state. The caller of
predicatelock_twophase_recover() has it, we just need a little plumbing
to pass it down.

Attached patches:

1. Rebased version of your patch, just trivial pgindent conflict fixes
2. Some comment typo fixes and improvements
3. Set the database ID on recovered 2PC transactions

A test for this would be nice. isolationtester doesn't support
connecting to different databases, restarting the server to test the 2PC
recovery, but a TAP test could do it.

--
Heikki Linnakangas
Neon (https://neon.tech)