MIME-Version: 1.0
References: 
 <CA+hUKGLqUAbpvO-aRznvF2LXnVBuRkYa7NcEG65C7Z0nziWKqw@mail.gmail.com>
 <0974d29d-d047-36fb-d466-93b933411442@iki.fi>
In-Reply-To: <0974d29d-d047-36fb-d466-93b933411442@iki.fi>
From: vignesh C <vignesh21@gmail.com>
Date: Sun, 21 Jan 2024 07:35:53 +0530
Message-ID: 
 <CALDaNm3gpHBkriWU0yHfNHP3_Lv5mOc54FAqXJFNND-qD_c-JQ@mail.gmail.com>
Subject: Re: Cross-database SERIALIZABLE safe snapshots
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Thomas Munro <thomas.munro@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CALDaNm3gpHBkriWU0yHfNHP3_Lv5mOc54FAqXJFNND-qD_c-JQ%40mail.gmail.com>
Precedence: bulk

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