Обсуждение: Cross-database SERIALIZABLE safe snapshots

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.

[1] https://www.postgresql.org/message-id/flat/17368-98a4f99e8e4b4402%40postgresql.org

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)

On Mon, 10 Jul 2023 at 14:48, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> 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.

@Thomas Munro As this patch is already marked as "Ready for
Committer", do you want to take this patch forward based on Heikki's
suggestions and get it committed?

Regards,
Vignesh