Date: Sun, 19 Sep 2021 21:41:43 -0700
From: Noah Misch <noah@leadboat.com>
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>,
	Michael Paquier <michael@paquier.xyz>, Tom Lane <tgl@sss.pgh.pa.us>,
	Peter Geoghegan <pg@bowt.ie>, Andres Freund <andres@anarazel.de>
Subject: Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data
Message-ID: <20210920044143.GA3414844@rfd.leadboat.com>
References: <5E041A70-4946-489C-9B6D-764DF627A92D@yandex-team.ru>
 <20210808163752.GB23176@rfd.leadboat.com>
 <20210814221328.GA146778@rfd.leadboat.com>
 <20210815084535.GA1227376@rfd.leadboat.com>
 <E86CBC3F-1FCF-44BF-BAD8-1301F66D8767@yandex-team.ru>
 <20210815134547.3okuz6ktvu4qf7ig@alap3.anarazel.de>
 <15936F8A-0436-40C8-9D1A-4A4623681554@yandex-team.ru>
 <20210829180913.ghojzzsgkqgm7vki@alap3.anarazel.de>
 <364BAA98-1A3F-4C39-B0D9-ED8C40EA902D@yandex-team.ru>
 <20210908064515.GA2496820@rfd.leadboat.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="HcAYCG3uE/tztfnV"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20210908064515.GA2496820@rfd.leadboat.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Archived-At: 
 <https://www.postgresql.org/message-id/20210920044143.GA3414844%40rfd.leadboat.com>
Precedence: bulk


--HcAYCG3uE/tztfnV
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On Mon, Aug 23, 2021 at 10:38:00PM +0500, Andrey Borodin wrote:
> --- a/src/backend/access/transam/twophase.c
> +++ b/src/backend/access/transam/twophase.c
> @@ -459,14 +459,15 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
>  	proc->pgprocno = gxact->pgprocno;
>  	SHMQueueElemInit(&(proc->links));
>  	proc->waitStatus = PROC_WAIT_STATUS_OK;
> -	/* We set up the gxact's VXID as InvalidBackendId/XID */
> -	proc->lxid = (LocalTransactionId) xid;
> +	/* We set up the gxact's VXID as real for CIC purposes */
> +	proc->lxid = MyProc->lxid;

This breaks the case where the server restarted after PREPARE TRANSACTION.
MyProc->lxid is 0 in the startup process, and LocalTransactionIdIsValid(0) is
false.  I'm attaching a test case addition.  Can you repair this?

>  	proc->xid = xid;
>  	Assert(proc->xmin == InvalidTransactionId);
>  	proc->delayChkpt = false;
>  	proc->statusFlags = 0;
>  	proc->pid = 0;
> -	proc->backendId = InvalidBackendId;
> +	/* May be backendId of startup process */
> +	proc->backendId = MyBackendId;

Incidentally, MyBackendId of startup process depends on other facts.  When
using hot standby, InitRecoveryTransactionEnvironment() sets MyBackendId=1.
Otherwise, including clean startup of a non-standby node, MyBackendId is
InvalidBackendId.  This may be harmless.  I didn't know about it.

On Tue, Sep 07, 2021 at 11:45:15PM -0700, Noah Misch wrote:
> On Sun, Aug 29, 2021 at 11:38:03PM +0500, Andrey Borodin wrote:
> > > 29 авг. 2021 г., в 23:09, Andres Freund <andres@anarazel.de> написал(а):
> > >>> It seems like it's going to add a substantial amount of work even when
> > >>> no 2PC xacts are involved?
> > >> Only if 2PCs are enabled.
> > > 
> > > I don't think that's good enough. Plenty of systems have 2PC enabled but very
> > > few if any transactions end up as 2PC ones.
> 
> > Best optimisation I can imagine here is to gather all vxids with unknown xids and search for them in one call to TwoPhaseGetXidByVXid() with one LWLockAcquire(TwoPhaseStateLock, LW_SHARED).
> > 
> > Does it worth the complexity?
> 
> https://www.postgresql.org/search/?m=1&q=TwoPhaseStateLock&l=&d=-1&s=r
> suggests this is the first postgresql.org discussion of TwoPhaseStateLock as a
> bottleneck.  Nonetheless, if Andres Freund finds it's worth the complexity,
> then I'm content with it.  I'd certainly expect some performance benefit.
> Andres, what do you think?

A few more benefits (beyond lock contention) come to mind:

- Looking at the three VirtualXactLock() callers, waiting for final
  disposition of prepared transactions is necessary for
  WaitForLockersMultiple(), disadvantageous for WaitForOlderSnapshots(), and
  dead code for ResolveRecoveryConflictWithVirtualXIDs().  In
  WaitForOlderSnapshots(), PREPARE is as good as COMMIT/ABORT, because a
  prepared transaction won't do further database reads.  Waiting on the
  prepared transaction there could give CIC an arbitrarily-long, needless
  delay.  ResolveRecoveryConflictWithVirtualXIDs() will never wait on a
  prepared transaction, because prepared transactions hold no locks during
  recovery.  (If a prepared transaction originally acquired
  AccessExclusiveLock, the startup process holds that lock on its behalf.)
  Coordinating the XID search at a higher layer would let us change
  WaitForLockersMultiple() without changing the others.

- v13 WaitPreparedXact() experiences starvation when a steady stream of
  prepared transactions have the same VXID.  Since VXID reuse entails
  reconnecting, starvation will be unnoticeable in systems that follow best
  practices around connection lifespan.  The 2021-08-23 patch version didn't
  have that hazard.

None of those benefits clearly justify the complexity, but they're relevant to
the decision.

--HcAYCG3uE/tztfnV
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
 filename="test-2pc-wait-after-recovery-v0.patch"

diff --git a/contrib/amcheck/t/003_cic_2pc.pl b/contrib/amcheck/t/003_cic_2pc.pl
index fa1a499..9190dca 100644
--- a/contrib/amcheck/t/003_cic_2pc.pl
+++ b/contrib/amcheck/t/003_cic_2pc.pl
@@ -9,7 +9,7 @@ use Config;
 use PostgresNode;
 use TestLib;
 
-use Test::More tests => 5;
+use Test::More tests => 6;
 
 my ($node, $result);
 
@@ -95,6 +95,37 @@ is($result, '0', 'bt_index_check checks index');
 
 
 #
+# Server restart shall not change whether prepared xact blocks CIC
+#
+
+$node->safe_psql('postgres', q(
+BEGIN;
+INSERT INTO tbl VALUES(0);
+PREPARE TRANSACTION 'spans_restart';
+BEGIN;
+CREATE TABLE unused ();
+PREPARE TRANSACTION 'persists_forever';
+));
+$node->restart;
+
+$reindex_in  = '';
+$reindex_out = '';
+$reindex_timer = IPC::Run::timeout(180);
+$reindex_h = $node->background_psql('postgres', \$reindex_in, \$reindex_out, $reindex_timer,
+	on_error_stop => 1);
+$reindex_in .= q(
+\echo start
+REINDEX TABLE CONCURRENTLY tbl;
+);
+pump $reindex_h until $reindex_out =~ /start/ || $reindex_timer->is_expired;
+
+$node->safe_psql('postgres', "COMMIT PREPARED 'spans_restart'");
+$reindex_h->finish;
+$result = $node->psql('postgres', q(SELECT bt_index_check('idx',true)));
+is($result, '0', 'bt_index_check checks index');
+
+
+#
 # Test 2. Stress CIC+2PC with pgbench
 #
 

--HcAYCG3uE/tztfnV--