pg.ddx.io  pgsql-bugs@postgresql.org mailing list archive  

help / color / mirror / Atom feed

From: Noah Misch <noah@leadboat.com>
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Peter Geoghegan <pg@bowt.ie>
Cc: Andres Freund <andres@anarazel.de>
Subject: Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data
Date: Sun, 17 Oct 2021 08:12:05 -0700
Message-ID: <20211017151205.GA2384251@rfd.leadboat.com> (raw)
In-Reply-To: <20210925201012.GC4134968@rfd.leadboat.com>
References: <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>
	<20210920044143.GA3414844@rfd.leadboat.com>
	<593C6C66-F19E-412A-9896-F1F5B272424A@yandex-team.ru>
	<20210925201012.GC4134968@rfd.leadboat.com>

On Sat, Sep 25, 2021 at 01:10:12PM -0700, Noah Misch wrote:
> On Sat, Sep 25, 2021 at 10:25:05PM +0500, Andrey Borodin wrote:
> > > 20 сент. 2021 г., в 09:41, Noah Misch <noah@leadboat.com> написал(а):

> I'll queue a task to review the rest of the patch.

I think the attached version is ready for commit.  Notable differences
vs. v14:

- Made TwoPhaseGetXidByVXid() stop leaking TwoPhaseStateLock when it found a
  second match.

- Instead of moving the ProcArrayClearTransaction() call within
  PrepareTransaction(), move the PostPrepare_Locks() call.  I didn't find any
  bug in the other way, but it's a good principle to maximize similarity with
  CommitTransaction().  PostPrepare_Locks() has no counterpart in
  CommitTransaction(), so that principle is indifferent to moving it.

- inval-build-race-v0.1.patch was incompatible with debug_discard_caches.  The
  being-built relation would always get invalidated, making
  RelationBuildDesc() an infinite loop.  I've fixed this by making
  RelationCacheInvalidate() invalidate in-progress rels only when called for
  sinval overflow, not when called for debug_discard_caches.  This adds some
  function arguments that only matter in assert builds.  That's not great, but
  InvalidateSystemCaches() is expensive anyway.  I considered instead adding
  functions HoldDebugInval() and ResumeDebugInval(), which RelationBuildDesc()
  would use to suppress debug_discard_caches during any iteration after the
  first.  That didn't seem better.

- Discard the in-progress array after an ERROR during RelationBuildDesc().

- Made the relcache.c changes repalloc the list of in-progress rels as needed.

- Changed the background_pgbench args from ($dbname, $stdin, \$stdout, $timer,
  $files, $opts) to ($opts, $files, \$stdout, $timer).  $dbname was unused.
  pgbench doesn't make interesting use of its stdin, so I dropped that arg
  until we have a use case.  $opts and $files seem akin to the $dbname arg of
  background_psql, so I moved them to the front.  I'm not sure that last
  change was an improvement.

- Made 001_pgbench_with_server.pl use PostgresNode::pgbench(), rather than
  duplicate code.  Factored out a subroutine of PostgresNode::pgbench() and
  PostgresNode::background_pgbench().

- Lots of comment and naming changes.

One thing not done here is to change the tests to use CREATE INDEX
CONCURRENTLY instead of REINDEX CONCURRENTLY, so they're back-patchable to v11
and earlier.  I may do that before pushing, or I may just omit the tests from
older branches.

> > > - 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.
> > I think the probability of such a stream is abysmal. You not only need a stream of 2PC with the same vxid, but a stream of overlapping 2PC with the same vxid. And the most critical thing that can happen - CIC waiting for the stream to became one-2PC-at-a-time for a moment.
> 
> You're probably right about that.

I didn't know of the "stateP->nextLXID = nextLocalTransactionId;" in
CleanupInvalidationState(), which indeed makes this all but impossible.

On Sat, Aug 07, 2021 at 03:19:57PM -0700, Noah Misch wrote:
> On Sun, Aug 08, 2021 at 12:00:55AM +0500, Andrey Borodin wrote:
> > Changes:
> > 1. Added assert in step 2 (fix for missed invalidation message). I wonder how deep possibly could be RelationBuildDesc() inside RelationBuildDesc() inside RelationBuildDesc() ... ? If the depth is unlimited we, probably, need a better data structure.
> 
> I don't know either, hence that quick data structure to delay the question.
> debug_discard_caches=3 may help answer the question.  RelationBuildDesc()
> reads pg_constraint, which is !rd_isnailed.  Hence, I expect one can at least
> get RelationBuildDesc("pg_constraint") inside RelationBuildDesc("user_table").

debug_discard_caches=5 yields a depth of eight when opening a relation having
a CHECK constraint:

my_rel_having_check_constraint
pg_constraint_conrelid_contypid_conname_index
pg_index
pg_constraint
pg_constraint
pg_constraint
pg_constraint
pg_constraint

While debug_discard_caches doesn't permit higher values, I think one could
reach depths greater than eight by, for example, having a number of sessions
invalidate pg_constraint as often as possible.  Hence, I'm glad the code no
longer relies on a depth limit.

Attachments: download all (2 files, .tar.gz)

inval-build-race-v1.patch (text/plain, 25.4K) download
Author:     Noah Misch <noah@leadboat.com>
Commit:     Noah Misch <noah@leadboat.com>

    Avoid race in RelationBuildDesc() affecting CREATE INDEX CONCURRENTLY.
    
    CIC and REINDEX CONCURRENTLY assume backends see their catalog changes
    no later than each backend's next transaction start.  That failed to
    hold when a backend absorbed a relevant invalidation in the middle of
    running RelationBuildDesc() on the CIC index.  Queries that use the
    resulting index can silently fail to find rows.  Fix this for future
    index builds by making RelationBuildDesc() loop until it finishes
    without accepting a relevant invalidation.  It may be necessary to
    reindex to recover from past occurrences; REINDEX CONCURRENTLY suffices.
    Back-patch to 9.6 (all supported versions).
    
    Noah Misch and Andrey Borodin, reviewed (in earlier versions) by Andres
    Freund.
    
    Discussion: https://postgr.es/m/20210730022548.GA1940096@gust.leadboat.com

diff --git a/contrib/amcheck/t/002_cic.pl b/contrib/amcheck/t/002_cic.pl
new file mode 100644
index 0000000..dba301e
--- /dev/null
+++ b/contrib/amcheck/t/002_cic.pl
@@ -0,0 +1,80 @@
+
+# Copyright (c) 2021, PostgreSQL Global Development Group
+
+# Test CREATE INDEX CONCURRENTLY with concurrent modifications
+use strict;
+use warnings;
+
+use Config;
+use PostgresNode;
+use TestLib;
+
+use Test::More tests => 4;
+
+my ($node, $result);
+
+#
+# Test set-up
+#
+$node = PostgresNode->new('CIC_test');
+$node->init;
+$node->append_conf('postgresql.conf', 'lock_timeout = 180000');
+$node->start;
+$node->safe_psql('postgres', q(CREATE EXTENSION amcheck));
+$node->safe_psql('postgres', q(CREATE TABLE tbl(i int)));
+$node->safe_psql('postgres', q(CREATE INDEX idx on tbl(i)));
+
+#
+# Stress CIC with pgbench
+#
+
+# Run background pgbench with CIC. We cannot mix-in this script into single
+# pgbench: CIC will deadlock with itself occasionally.
+my $pgbench_out   = '';
+my $pgbench_timer = IPC::Run::timeout(180);
+my $pgbench_h     = $node->background_pgbench(
+	'--no-vacuum --client=1 --transactions=200',
+	{
+		'002_pgbench_concurrent_cic' => q(
+			REINDEX INDEX CONCURRENTLY idx;
+			SELECT bt_index_check('idx',true);
+		   )
+	},
+	\$pgbench_out,
+	$pgbench_timer);
+
+# Run pgbench.
+$node->pgbench(
+	'--no-vacuum --client=5 --transactions=200',
+	0,
+	[qr{actually processed}],
+	[qr{^$}],
+	'concurrent INSERTs',
+	{
+		'002_pgbench_concurrent_transaction' => q(
+			BEGIN;
+			SELECT pg_sleep(0.001);
+			INSERT INTO tbl VALUES(0);
+			COMMIT;
+		  ),
+		'002_pgbench_concurrent_transaction_savepoints' => q(
+			BEGIN;
+			SELECT pg_sleep(0.001);
+			SAVEPOINT s1;
+			SELECT pg_sleep(0.001);
+			INSERT INTO tbl VALUES(0);
+			COMMIT;
+		  )
+	});
+
+$pgbench_h->pump_nb;
+$pgbench_h->finish();
+$result =
+    ($Config{osname} eq "MSWin32")
+  ? ($pgbench_h->full_results)[0]
+  : $pgbench_h->result(0);
+is($result, 0, "pgbench with CIC works");
+
+# done
+$node->stop;
+done_testing();
diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c
index 9352c68..dd8586a 100644
--- a/src/backend/utils/cache/inval.c
+++ b/src/backend/utils/cache/inval.c
@@ -628,7 +628,7 @@ LocalExecuteInvalidationMessage(SharedInvalidationMessage *msg)
 			int			i;
 
 			if (msg->rc.relId == InvalidOid)
-				RelationCacheInvalidate();
+				RelationCacheInvalidate(false);
 			else
 				RelationCacheInvalidateEntry(msg->rc.relId);
 
@@ -686,11 +686,17 @@ LocalExecuteInvalidationMessage(SharedInvalidationMessage *msg)
 void
 InvalidateSystemCaches(void)
 {
+	InvalidateSystemCachesExtended(false);
+}
+
+void
+InvalidateSystemCachesExtended(bool debug_discard)
+{
 	int			i;
 
 	InvalidateCatalogSnapshot();
 	ResetCatalogCaches();
-	RelationCacheInvalidate();	/* gets smgr and relmap too */
+	RelationCacheInvalidate(debug_discard); /* gets smgr and relmap too */
 
 	for (i = 0; i < syscache_callback_count; i++)
 	{
@@ -759,7 +765,7 @@ AcceptInvalidationMessages(void)
 		if (recursion_depth < debug_discard_caches)
 		{
 			recursion_depth++;
-			InvalidateSystemCaches();
+			InvalidateSystemCachesExtended(true);
 			recursion_depth--;
 		}
 	}
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 13d9994..b54c911 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -151,6 +151,24 @@ bool		criticalSharedRelcachesBuilt = false;
 static long relcacheInvalsReceived = 0L;
 
 /*
+ * in_progress_list is a stack of ongoing RelationBuildDesc() calls.  CREATE
+ * INDEX CONCURRENTLY makes catalog changes under ShareUpdateExclusiveLock.
+ * It critically relies on each backend absorbing those changes no later than
+ * next transaction start.  Hence, RelationBuildDesc() loops until it finishes
+ * without accepting a relevant invalidation.  (Most invalidation consumers
+ * don't do this.)
+ */
+typedef struct inprogressent
+{
+	Oid			reloid;			/* OID of relation being built */
+	bool		invalidated;	/* whether an invalidation arrived for it */
+} InProgressEnt;
+
+static InProgressEnt *in_progress_list;
+static int	in_progress_list_len;
+static int	in_progress_list_maxlen;
+
+/*
  * eoxact_list[] stores the OIDs of relations that (might) need AtEOXact
  * cleanup work.  This list intentionally has limited size; if it overflows,
  * we fall back to scanning the whole hashtable.  There is no value in a very
@@ -1000,6 +1018,7 @@ equalRSDesc(RowSecurityDesc *rsdesc1, RowSecurityDesc *rsdesc2)
 static Relation
 RelationBuildDesc(Oid targetRelId, bool insertIt)
 {
+	int			in_progress_offset;
 	Relation	relation;
 	Oid			relid;
 	HeapTuple	pg_class_tuple;
@@ -1033,6 +1052,21 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
 	}
 #endif
 
+	/* Register to catch invalidation messages */
+	if (in_progress_list_len >= in_progress_list_maxlen)
+	{
+		int			allocsize;
+
+		allocsize = in_progress_list_maxlen * 2;
+		in_progress_list = repalloc(in_progress_list,
+									allocsize * sizeof(*in_progress_list));
+		in_progress_list_maxlen = allocsize;
+	}
+	in_progress_offset = in_progress_list_len++;
+	in_progress_list[in_progress_offset].reloid = targetRelId;
+retry:
+	in_progress_list[in_progress_offset].invalidated = false;
+
 	/*
 	 * find the tuple in pg_class corresponding to the given relation id
 	 */
@@ -1051,6 +1085,8 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
 			MemoryContextDelete(tmpcxt);
 		}
 #endif
+		Assert(in_progress_offset + 1 == in_progress_list_len);
+		in_progress_list_len--;
 		return NULL;
 	}
 
@@ -1214,6 +1250,21 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
 	heap_freetuple(pg_class_tuple);
 
 	/*
+	 * If an invalidation arrived mid-build, start over.  Between here and the
+	 * end of this function, don't add code that does or reasonably could read
+	 * system catalogs.  That range must be free from invalidation processing
+	 * for the !insertIt case.  For the insertIt case, RelationCacheInsert()
+	 * will enroll this relation in ordinary relcache invalidation processing,
+	 */
+	if (in_progress_list[in_progress_offset].invalidated)
+	{
+		RelationDestroyRelation(relation, false);
+		goto retry;
+	}
+	Assert(in_progress_offset + 1 == in_progress_list_len);
+	in_progress_list_len--;
+
+	/*
 	 * Insert newly created relation into relcache hash table, if requested.
 	 *
 	 * There is one scenario in which we might find a hashtable entry already
@@ -2566,6 +2617,14 @@ RelationClearRelation(Relation relation, bool rebuild)
 
 		/* Build temporary entry, but don't link it into hashtable */
 		newrel = RelationBuildDesc(save_relid, false);
+
+		/*
+		 * Between here and the end of the swap, don't add code that does or
+		 * reasonably could read system catalogs.  That range must be free
+		 * from invalidation processing.  See RelationBuildDesc() manipulation
+		 * of in_progress_list.
+		 */
+
 		if (newrel == NULL)
 		{
 			/*
@@ -2805,6 +2864,14 @@ RelationCacheInvalidateEntry(Oid relationId)
 		relcacheInvalsReceived++;
 		RelationFlushRelation(relation);
 	}
+	else
+	{
+		int			i;
+
+		for (i = 0; i < in_progress_list_len; i++)
+			if (in_progress_list[i].reloid == relationId)
+				in_progress_list[i].invalidated = true;
+	}
 }
 
 /*
@@ -2813,11 +2880,11 @@ RelationCacheInvalidateEntry(Oid relationId)
  *	 and rebuild those with positive reference counts.  Also reset the smgr
  *	 relation cache and re-read relation mapping data.
  *
- *	 This is currently used only to recover from SI message buffer overflow,
- *	 so we do not touch relations having new-in-transaction relfilenodes; they
- *	 cannot be targets of cross-backend SI updates (and our own updates now go
- *	 through a separate linked list that isn't limited by the SI message
- *	 buffer size).
+ *	 Apart from debug_discard_caches, this is currently used only to recover
+ *	 from SI message buffer overflow, so we do not touch relations having
+ *	 new-in-transaction relfilenodes; they cannot be targets of cross-backend
+ *	 SI updates (and our own updates now go through a separate linked list
+ *	 that isn't limited by the SI message buffer size).
  *
  *	 We do this in two phases: the first pass deletes deletable items, and
  *	 the second one rebuilds the rebuildable items.  This is essential for
@@ -2835,9 +2902,14 @@ RelationCacheInvalidateEntry(Oid relationId)
  *	 second pass processes nailed-in-cache items before other nondeletable
  *	 items.  This should ensure that system catalogs are up to date before
  *	 we attempt to use them to reload information about other open relations.
+ *
+ *	 After those two phases of work having immediate effects, we normally
+ *	 signal any RelationBuildDesc() on the stack to start over.  However, we
+ *	 don't do this if called as part of debug_discard_caches.  Otherwise,
+ *	 RelationBuildDesc() would become an infinite loop.
  */
 void
-RelationCacheInvalidate(void)
+RelationCacheInvalidate(bool debug_discard)
 {
 	HASH_SEQ_STATUS status;
 	RelIdCacheEnt *idhentry;
@@ -2845,6 +2917,7 @@ RelationCacheInvalidate(void)
 	List	   *rebuildFirstList = NIL;
 	List	   *rebuildList = NIL;
 	ListCell   *l;
+	int			i;
 
 	/*
 	 * Reload relation mapping data before starting to reconstruct cache.
@@ -2931,6 +3004,11 @@ RelationCacheInvalidate(void)
 		RelationClearRelation(relation, true);
 	}
 	list_free(rebuildList);
+
+	if (!debug_discard)
+		/* Any RelationBuildDesc() on the stack must start over. */
+		for (i = 0; i < in_progress_list_len; i++)
+			in_progress_list[i].invalidated = true;
 }
 
 /*
@@ -3082,6 +3160,13 @@ AtEOXact_RelationCache(bool isCommit)
 	int			i;
 
 	/*
+	 * Forget in_progress_list.  This is relevant when we're aborting due to
+	 * an error during RelationBuildDesc().
+	 */
+	Assert(in_progress_list_len == 0 || !isCommit);
+	in_progress_list_len = 0;
+
+	/*
 	 * Unless the eoxact_list[] overflowed, we only need to examine the rels
 	 * listed in it.  Otherwise fall back on a hash_seq_search scan.
 	 *
@@ -3228,6 +3313,14 @@ AtEOSubXact_RelationCache(bool isCommit, SubTransactionId mySubid,
 	int			i;
 
 	/*
+	 * Forget in_progress_list.  This is relevant when we're aborting due to
+	 * an error during RelationBuildDesc().  We don't commit subtransactions
+	 * during RelationBuildDesc().
+	 */
+	Assert(in_progress_list_len == 0 || !isCommit);
+	in_progress_list_len = 0;
+
+	/*
 	 * Unless the eoxact_list[] overflowed, we only need to examine the rels
 	 * listed in it.  Otherwise fall back on a hash_seq_search scan.  Same
 	 * logic as in AtEOXact_RelationCache.
@@ -3775,6 +3868,7 @@ void
 RelationCacheInitialize(void)
 {
 	HASHCTL		ctl;
+	int			allocsize;
 
 	/*
 	 * make sure cache memory context exists
@@ -3791,6 +3885,15 @@ RelationCacheInitialize(void)
 								  &ctl, HASH_ELEM | HASH_BLOBS);
 
 	/*
+	 * reserve enough in_progress_list slots for many cases
+	 */
+	allocsize = 4;
+	in_progress_list =
+		MemoryContextAlloc(CacheMemoryContext,
+						   allocsize * sizeof(*in_progress_list));
+	in_progress_list_maxlen = allocsize;
+
+	/*
 	 * relation mapper needs to be initialized too
 	 */
 	RelationMapInitialize();
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index ef53f6b..7ca96e5 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -14,54 +14,6 @@ my $node = PostgresNode->new('main');
 $node->init;
 $node->start;
 
-# invoke pgbench, with parameters:
-#   $opts: options as a string to be split on spaces
-#   $stat: expected exit status
-#   $out: reference to a regexp list that must match stdout
-#   $err: reference to a regexp list that must match stderr
-#   $name: name of test for error messages
-#   $files: reference to filename/contents dictionary
-#   @args: further raw options or arguments
-sub pgbench
-{
-	local $Test::Builder::Level = $Test::Builder::Level + 1;
-
-	my ($opts, $stat, $out, $err, $name, $files, @args) = @_;
-	my @cmd = ('pgbench', split /\s+/, $opts);
-	my @filenames = ();
-	if (defined $files)
-	{
-
-		# note: files are ordered for determinism
-		for my $fn (sort keys %$files)
-		{
-			my $filename = $node->basedir . '/' . $fn;
-			push @cmd, '-f', $filename;
-
-			# cleanup file weight
-			$filename =~ s/\@\d+$//;
-
-			#push @filenames, $filename;
-			# filenames are expected to be unique on a test
-			if (-e $filename)
-			{
-				ok(0, "$filename must not already exist");
-				unlink $filename or die "cannot unlink $filename: $!";
-			}
-			append_to_file($filename, $$files{$fn});
-		}
-	}
-
-	push @cmd, @args;
-
-	$node->command_checks_all(\@cmd, $stat, $out, $err, $name);
-
-	# cleanup?
-	#unlink @filenames or die "cannot unlink files (@filenames): $!";
-
-	return;
-}
-
 # tablespace for testing, because partitioned tables cannot use pg_default
 # explicitly and we want to test that table creation with tablespace works
 # for partitioned tables.
@@ -77,7 +29,7 @@ $node->safe_psql('postgres',
 # Test concurrent OID generation via pg_enum_oid_index.  This indirectly
 # exercises LWLock and spinlock concurrency.
 my $labels = join ',', map { "'l$_'" } 1 .. 1000;
-pgbench(
+$node->pgbench(
 	'--no-vacuum --client=5 --protocol=prepared --transactions=25',
 	0,
 	[qr{processed: 125/125}],
@@ -89,7 +41,7 @@ pgbench(
 	});
 
 # Trigger various connection errors
-pgbench(
+$node->pgbench(
 	'no-such-database',
 	1,
 	[qr{^$}],
@@ -99,13 +51,13 @@ pgbench(
 	],
 	'no such database');
 
-pgbench(
+$node->pgbench(
 	'-S -t 1', 1, [],
 	[qr{Perhaps you need to do initialization}],
 	'run without init');
 
 # Initialize pgbench tables scale 1
-pgbench(
+$node->pgbench(
 	'-i', 0,
 	[qr{^$}],
 	[
@@ -117,7 +69,7 @@ pgbench(
 	'pgbench scale 1 initialization',);
 
 # Again, with all possible options
-pgbench(
+$node->pgbench(
 	'--initialize --init-steps=dtpvg --scale=1 --unlogged-tables --fillfactor=98 --foreign-keys --quiet --tablespace=regress_pgbench_tap_1_ts --index-tablespace=regress_pgbench_tap_1_ts --partitions=2 --partition-method=hash',
 	0,
 	[qr{^$}i],
@@ -134,7 +86,7 @@ pgbench(
 	'pgbench scale 1 initialization');
 
 # Test interaction of --init-steps with legacy step-selection options
-pgbench(
+$node->pgbench(
 	'--initialize --init-steps=dtpvGvv --no-vacuum --foreign-keys --unlogged-tables --partitions=3',
 	0,
 	[qr{^$}],
@@ -151,7 +103,7 @@ pgbench(
 	'pgbench --init-steps');
 
 # Run all builtin scripts, for a few transactions each
-pgbench(
+$node->pgbench(
 	'--transactions=5 -Dfoo=bla --client=2 --protocol=simple --builtin=t'
 	  . ' --connect -n -v -n',
 	0,
@@ -164,7 +116,7 @@ pgbench(
 	[qr{^$}],
 	'pgbench tpcb-like');
 
-pgbench(
+$node->pgbench(
 	'--transactions=20 --client=5 -M extended --builtin=si -C --no-vacuum -s 1',
 	0,
 	[
@@ -177,7 +129,7 @@ pgbench(
 	[qr{scale option ignored}],
 	'pgbench simple update');
 
-pgbench(
+$node->pgbench(
 	'-t 100 -c 7 -M prepared -b se --debug',
 	0,
 	[
@@ -203,7 +155,7 @@ my $nthreads = 2;
 }
 
 # run custom scripts
-pgbench(
+$node->pgbench(
 	"-t 100 -c 1 -j $nthreads -M prepared -n",
 	0,
 	[
@@ -233,7 +185,7 @@ COMMIT;
 }
 	});
 
-pgbench(
+$node->pgbench(
 	'-n -t 10 -c 1 -M simple',
 	0,
 	[
@@ -254,7 +206,7 @@ COMMIT;
 }
 	});
 
-pgbench(
+$node->pgbench(
 	'-n -t 10 -c 2 -M extended',
 	0,
 	[
@@ -285,7 +237,7 @@ $node->append_conf('postgresql.conf',
 	  . "log_parameter_max_length = 0\n"
 	  . "log_parameter_max_length_on_error = 0");
 $node->reload;
-pgbench(
+$node->pgbench(
 	'-n -t1 -c1 -M prepared',
 	2,
 	[],
@@ -312,7 +264,7 @@ $node->append_conf('postgresql.conf',
 	    "log_parameter_max_length = -1\n"
 	  . "log_parameter_max_length_on_error = 64");
 $node->reload;
-pgbench(
+$node->pgbench(
 	'-n -t1 -c1 -M prepared',
 	2,
 	[],
@@ -326,7 +278,7 @@ pgbench(
 SELECT 1 / (random() / 2)::int, :one::int, :two::int;
 }
 	});
-pgbench(
+$node->pgbench(
 	'-n -t1 -c1 -M prepared',
 	2,
 	[],
@@ -354,7 +306,7 @@ $node->append_conf('postgresql.conf',
 	  . "log_parameter_max_length = 7\n"
 	  . "log_parameter_max_length_on_error = -1");
 $node->reload;
-pgbench(
+$node->pgbench(
 	'-n -t1 -c1 -M prepared',
 	2,
 	[],
@@ -371,7 +323,7 @@ SELECT 1 / (random() / 2)::int, :one::int, :two::int;
 
 $node->append_conf('postgresql.conf', "log_min_duration_statement = 0");
 $node->reload;
-pgbench(
+$node->pgbench(
 	'-n -t1 -c1 -M prepared',
 	2,
 	[],
@@ -394,7 +346,7 @@ like(
 $log = undef;
 
 # Check that bad parameters are reported during typinput phase of BIND
-pgbench(
+$node->pgbench(
 	'-n -t1 -c1 -M prepared',
 	2,
 	[],
@@ -418,7 +370,7 @@ $node->reload;
 
 # test expressions
 # command 1..3 and 23 depend on random seed which is used to call srandom.
-pgbench(
+$node->pgbench(
 	'--random-seed=5432 -t 1 -Dfoo=-10.1 -Dbla=false -Di=+3 -Dn=null -Dt=t -Df=of -Dd=1.0',
 	0,
 	[ qr{type: .*/001_pgbench_expressions}, qr{processed: 1/1} ],
@@ -653,7 +605,7 @@ $node->safe_psql('postgres',
 my $seed = int(rand(1000000000));
 for my $i (1, 2)
 {
-	pgbench(
+	$node->pgbench(
 		"--random-seed=$seed -t 1",
 		0,
 		[qr{processed: 1/1}],
@@ -693,7 +645,7 @@ ok($out =~ /\b$seed\|zipfian\|4\d\d\d\|2/,
 $node->safe_psql('postgres', 'DROP TABLE seeded_random;');
 
 # backslash commands
-pgbench(
+$node->pgbench(
 	'-t 1', 0,
 	[
 		qr{type: .*/001_pgbench_backslash_commands},
@@ -722,7 +674,7 @@ pgbench(
 	});
 
 # working \gset
-pgbench(
+$node->pgbench(
 	'-t 1', 0,
 	[ qr{type: .*/001_pgbench_gset}, qr{processed: 1/1} ],
 	[
@@ -757,7 +709,7 @@ SELECT 0 AS i4, 4 AS i4 \gset
 }
 	});
 # \gset cannot accept more than one row, causing command to fail.
-pgbench(
+$node->pgbench(
 	'-t 1', 2,
 	[ qr{type: .*/001_pgbench_gset_two_rows}, qr{processed: 0/1} ],
 	[qr{expected one row, got 2\b}],
@@ -770,7 +722,7 @@ SELECT 5432 AS fail UNION SELECT 5433 ORDER BY 1 \gset
 
 # working \aset
 # Valid cases.
-pgbench(
+$node->pgbench(
 	'-t 1', 0,
 	[ qr{type: .*/001_pgbench_aset}, qr{processed: 1/1} ],
 	[ qr{command=3.: int 8\b},       qr{command=4.: int 7\b} ],
@@ -786,7 +738,7 @@ SELECT 8 AS i6 UNION SELECT 9 ORDER BY 1 DESC \aset
 }
 	});
 # Empty result set with \aset, causing command to fail.
-pgbench(
+$node->pgbench(
 	'-t 1', 2,
 	[ qr{type: .*/001_pgbench_aset_empty}, qr{processed: 0/1} ],
 	[
@@ -803,7 +755,7 @@ pgbench(
 	});
 
 # Working \startpipeline
-pgbench(
+$node->pgbench(
 	'-t 1 -n -M extended',
 	0,
 	[ qr{type: .*/001_pgbench_pipeline}, qr{actually processed: 1/1} ],
@@ -819,7 +771,7 @@ pgbench(
 	});
 
 # Working \startpipeline in prepared query mode
-pgbench(
+$node->pgbench(
 	'-t 1 -n -M prepared',
 	0,
 	[ qr{type: .*/001_pgbench_pipeline_prep}, qr{actually processed: 1/1} ],
@@ -835,7 +787,7 @@ pgbench(
 	});
 
 # Try \startpipeline twice
-pgbench(
+$node->pgbench(
 	'-t 1 -n -M extended',
 	2,
 	[],
@@ -850,7 +802,7 @@ pgbench(
 	});
 
 # Try to end a pipeline that hasn't started
-pgbench(
+$node->pgbench(
 	'-t 1 -n -M extended',
 	2,
 	[],
@@ -864,7 +816,7 @@ pgbench(
 	});
 
 # Try \gset in pipeline mode
-pgbench(
+$node->pgbench(
 	'-t 1 -n -M extended',
 	2,
 	[],
@@ -1129,7 +1081,7 @@ for my $e (@errors)
 	$status != 0 or die "invalid expected status for test \"$name\"";
 	my $n = '001_pgbench_error_' . $name;
 	$n =~ s/ /_/g;
-	pgbench(
+	$node->pgbench(
 		'-n -t 1 -Dfoo=bla -Dnull=null -Dtrue=true -Done=1 -Dzero=0.0 -Dbadtrue=trueXXX'
 		  . ' -Dmaxint=9223372036854775807 -Dminint=-9223372036854775808'
 		  . ($no_prepare ? '' : ' -M prepared'),
@@ -1141,14 +1093,14 @@ for my $e (@errors)
 }
 
 # throttling
-pgbench(
+$node->pgbench(
 	'-t 100 -S --rate=100000 --latency-limit=1000000 -c 2 -n -r',
 	0,
 	[ qr{processed: 200/200}, qr{builtin: select only} ],
 	[qr{^$}],
 	'pgbench throttling');
 
-pgbench(
+$node->pgbench(
 
 	# given the expected rate and the 2 ms tx duration, at most one is executed
 	'-t 10 --rate=100000 --latency-limit=1 -n -r',
@@ -1220,7 +1172,7 @@ sub check_pgbench_logs
 my $bdir = $node->basedir;
 
 # Run with sampling rate, 2 clients with 50 transactions each.
-pgbench(
+$node->pgbench(
 	"-n -S -t 50 -c 2 --log --sampling-rate=0.5", 0,
 	[ qr{select only}, qr{processed: 100/100} ], [qr{^$}],
 	'pgbench logs', undef,
@@ -1230,7 +1182,7 @@ check_pgbench_logs($bdir, '001_pgbench_log_2', 1, 8, 92,
 	qr{^[01] \d{1,2} \d+ \d \d+ \d+$});
 
 # Run with different read-only option pattern, 1 client with 10 transactions.
-pgbench(
+$node->pgbench(
 	"-n -b select-only -t 10 -l", 0,
 	[ qr{select only}, qr{processed: 10/10} ], [qr{^$}],
 	'pgbench logs contents', undef,
diff --git a/src/include/utils/inval.h b/src/include/utils/inval.h
index 7706728..877e66c 100644
--- a/src/include/utils/inval.h
+++ b/src/include/utils/inval.h
@@ -62,6 +62,7 @@ extern void CacheRegisterRelcacheCallback(RelcacheCallbackFunction func,
 extern void CallSyscacheCallbacks(int cacheid, uint32 hashvalue);
 
 extern void InvalidateSystemCaches(void);
+extern void InvalidateSystemCachesExtended(bool debug_discard);
 
 extern void LogLogicalInvalidations(void);
 #endif							/* INVAL_H */
diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h
index d2c1757..aa060ef 100644
--- a/src/include/utils/relcache.h
+++ b/src/include/utils/relcache.h
@@ -122,7 +122,7 @@ extern void RelationForgetRelation(Oid rid);
 
 extern void RelationCacheInvalidateEntry(Oid relationId);
 
-extern void RelationCacheInvalidate(void);
+extern void RelationCacheInvalidate(bool debug_discard);
 
 extern void RelationCloseSmgrByOid(Oid relationId);
 
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index c59da75..bfb338c 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -1981,6 +1981,139 @@ sub interactive_psql
 	return $harness;
 }
 
+# Common sub of pgbench-invoking interfaces.  Makes any requested script files
+# and returns pgbench command-line options causing use of those files.
+sub _pgbench_make_files
+{
+	my ($self, $files) = @_;
+	my @file_opts;
+
+	if (defined $files)
+	{
+
+		# note: files are ordered for determinism
+		for my $fn (sort keys %$files)
+		{
+			my $filename = $self->basedir . '/' . $fn;
+			push @file_opts, '-f', $filename;
+
+			# cleanup file weight
+			$filename =~ s/\@\d+$//;
+
+			#push @filenames, $filename;
+			# filenames are expected to be unique on a test
+			if (-e $filename)
+			{
+				ok(0, "$filename must not already exist");
+				unlink $filename or die "cannot unlink $filename: $!";
+			}
+			TestLib::append_to_file($filename, $$files{$fn});
+		}
+	}
+
+	return @file_opts;
+}
+
+=pod
+
+=item $node->pgbench($opts, $stat, $out, $err, $name, $files, @args)
+
+Invoke B<pgbench>, with parameters and files.
+
+=over
+
+=item $opts
+
+Options as a string to be split on spaces.
+
+=item $stat
+
+Expected exit status.
+
+=item $out
+
+Reference to a regexp list that must match stdout.
+
+=item $err
+
+Reference to a regexp list that must match stderr.
+
+=item $name
+
+Name of test for error messages.
+
+=item $files
+
+Reference to filename/contents dictionary.
+
+=item @args
+
+Further raw options or arguments.
+
+=back
+
+=cut
+
+sub pgbench
+{
+	local $Test::Builder::Level = $Test::Builder::Level + 1;
+
+	my ($self, $opts, $stat, $out, $err, $name, $files, @args) = @_;
+	my @cmd = (
+		'pgbench',
+		split(/\s+/, $opts),
+		$self->_pgbench_make_files($files), @args);
+
+	$self->command_checks_all(\@cmd, $stat, $out, $err, $name);
+}
+
+=pod
+
+=item $node->background_pgbench($opts, $files, \$stdout, $timer) => harness
+
+Invoke B<pgbench> and return an IPC::Run harness object.  The process's stdin
+is empty, and its stdout and stderr go to the $stdout scalar reference.  This
+allows the caller to act on other parts of the system while B<pgbench> is
+running.  Errors from B<pgbench> are the caller's problem.
+
+The specified timer object is attached to the harness, as well.  It's caller's
+responsibility to select the timeout length, and to restart the timer after
+each command if the timeout is per-command.
+
+Be sure to "finish" the harness when done with it.
+
+=over
+
+=item $opts
+
+Options as a string to be split on spaces.
+
+=item $files
+
+Reference to filename/contents dictionary.
+
+=back
+
+=cut
+
+sub background_pgbench
+{
+	my ($self, $opts, $files, $stdout, $timer) = @_;
+
+	my @cmd =
+	  ('pgbench', split(/\s+/, $opts), $self->_pgbench_make_files($files));
+
+	local %ENV = $self->_get_env();
+
+	my $stdin = "";
+	# IPC::Run would otherwise append to existing contents:
+	$$stdout = "" if ref($stdout);
+
+	my $harness = IPC::Run::start \@cmd, '<', \$stdin, '>', $stdout, $timer;
+
+	return $harness;
+}
+
 =pod
 
 =item $node->connect_ok($connstr, $test_name, %params)
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index cb5b5ec..1c7bac0 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1093,6 +1093,7 @@ ImportForeignSchemaStmt
 ImportForeignSchemaType
 ImportForeignSchema_function
 ImportQual
+InProgressEnt
 IncludeWal
 InclusionOpaque
 IncrementVarSublevelsUp_context

prepared-transactions-cic-series202107-v15nm.patch (text/plain, 19.1K) download
Author:     Noah Misch <noah@leadboat.com>
Commit:     Noah Misch <noah@leadboat.com>

    Fix CREATE INDEX CONCURRENTLY for the newest prepared transactions.
    
    The purpose of commit 8a54e12a38d1545d249f1402f66c8cde2837d97c was to
    fix this, and it sufficed when the PREPARE TRANSACTION completed before
    the CIC looked for lock conflicts.  Otherwise, things still broke.  As
    before, in a cluster having used CIC while having enabled prepared
    transactions, queries that use the resulting index can silently fail to
    find rows.  It may be necessary to reindex to recover from past
    occurrences; REINDEX CONCURRENTLY suffices.  Fix this for future index
    builds by making CIC wait for arbitrarily-recent prepared transactions
    and for ordinary transactions that may yet PREPARE TRANSACTION.  As part
    of that, have PREPARE TRANSACTION transfer locks to its dummy PGPROC
    before it calls ProcArrayClearTransaction().  Back-patch to 9.6 (all
    supported versions).
    
    Andrey Borodin, reviewed (in earlier versions) by Andres Freund.
    
    Discussion: https://postgr.es/m/01824242-AA92-4FE9-9BA7-AEBAFFEA3D0C@yandex-team.ru

diff --git a/contrib/amcheck/t/003_cic_2pc.pl b/contrib/amcheck/t/003_cic_2pc.pl
new file mode 100644
index 0000000..7c5c052
--- /dev/null
+++ b/contrib/amcheck/t/003_cic_2pc.pl
@@ -0,0 +1,187 @@
+
+# Copyright (c) 2021, PostgreSQL Global Development Group
+
+# Test CREATE INDEX CONCURRENTLY with concurrent prepared-xact modifications
+use strict;
+use warnings;
+
+use Config;
+use PostgresNode;
+use TestLib;
+
+use Test::More tests => 6;
+
+my ($node, $result);
+
+#
+# Test set-up
+#
+$node = PostgresNode->new('CIC_2PC_test');
+$node->init;
+$node->append_conf('postgresql.conf', 'max_prepared_transactions = 10');
+$node->append_conf('postgresql.conf', 'lock_timeout = 180000');
+$node->start;
+$node->safe_psql('postgres', q(CREATE EXTENSION amcheck));
+$node->safe_psql('postgres', q(CREATE TABLE tbl(i int)));
+$node->safe_psql('postgres', q(CREATE INDEX idx on tbl(i)));
+
+
+#
+# Run 3 overlapping 2PC transactions with concurrent reindex rebuild
+#
+# We have two concurrent background psql processes: $main_h for INSERTs and
+# $reindex_h for cuncurrent reindex.  Also, we use non-background psql for
+# some COMMIT PREPARED statements.
+#
+
+my $main_in    = '';
+my $main_out   = '';
+my $main_timer = IPC::Run::timeout(180);
+
+my $main_h =
+  $node->background_psql('postgres', \$main_in, \$main_out, $main_timer,
+	on_error_stop => 1);
+$main_in .= q(
+BEGIN;
+INSERT INTO tbl VALUES(0);
+\echo syncpoint1
+);
+pump $main_h until $main_out =~ /syncpoint1/ || $main_timer->is_expired;
+
+my $reindex_in    = '';
+my $reindex_out   = '';
+my $reindex_timer = IPC::Run::timeout(180);
+my $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;
+
+$main_in .= q(
+PREPARE TRANSACTION 'a';
+);
+
+$main_in .= q(
+BEGIN;
+INSERT INTO tbl VALUES(0);
+\echo syncpoint2
+);
+pump $main_h until $main_out =~ /syncpoint2/ || $main_timer->is_expired;
+
+$node->safe_psql('postgres', q(COMMIT PREPARED 'a';));
+
+$main_in .= q(
+PREPARE TRANSACTION 'b';
+BEGIN;
+INSERT INTO tbl VALUES(0);
+\echo syncpoint3
+);
+pump $main_h until $main_out =~ /syncpoint3/ || $main_timer->is_expired;
+
+$node->safe_psql('postgres', q(COMMIT PREPARED 'b';));
+
+$main_in .= q(
+PREPARE TRANSACTION 'c';
+COMMIT PREPARED 'c';
+);
+$main_h->pump_nb;
+
+$main_h->finish;
+$reindex_h->finish;
+
+$result = $node->psql('postgres', q(SELECT bt_index_check('idx',true)));
+is($result, '0', 'bt_index_check after overlapping 2PC');
+
+
+#
+# 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 after 2PC and restart');
+
+
+#
+# Stress CIC+2PC with pgbench
+#
+
+# Fix broken index first
+$node->safe_psql('postgres', q(REINDEX TABLE CONCURRENTLY tbl;));
+
+# Run background pgbench with CIC. We cannot mix-in this script into single
+# pgbench: CIC will deadlock with itself occasionally.
+my $pgbench_out   = '';
+my $pgbench_timer = IPC::Run::timeout(180);
+my $pgbench_h     = $node->background_pgbench(
+	'--no-vacuum --client=1 --transactions=100',
+	{
+		'002_pgbench_concurrent_cic' => q(
+			REINDEX INDEX CONCURRENTLY idx;
+			SELECT bt_index_check('idx',true);
+		   )
+	},
+	\$pgbench_out,
+	$pgbench_timer);
+
+# Run pgbench.
+$node->pgbench(
+	'--no-vacuum --client=5 --transactions=100',
+	0,
+	[qr{actually processed}],
+	[qr{^$}],
+	'concurrent INSERTs w/ 2PC',
+	{
+		'002_pgbench_concurrent_2pc' => q(
+			BEGIN;
+			INSERT INTO tbl VALUES(0);
+			PREPARE TRANSACTION 'c:client_id';
+			COMMIT PREPARED 'c:client_id';
+		  ),
+		'002_pgbench_concurrent_2pc_savepoint' => q(
+			BEGIN;
+			SAVEPOINT s1;
+			INSERT INTO tbl VALUES(0);
+			PREPARE TRANSACTION 'c:client_id';
+			COMMIT PREPARED 'c:client_id';
+		  )
+	});
+
+$pgbench_h->pump_nb;
+$pgbench_h->finish();
+$result =
+    ($Config{osname} eq "MSWin32")
+  ? ($pgbench_h->full_results)[0]
+  : $pgbench_h->result(0);
+is($result, 0, "pgbench with CIC works");
+
+# done
+$node->stop;
+done_testing();
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 2156de1..f6e7fa7 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -459,14 +459,24 @@ 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;
+	if (LocalTransactionIdIsValid(MyProc->lxid))
+	{
+		/* clone VXID, for TwoPhaseGetXidByVirtualXID() to find */
+		proc->lxid = MyProc->lxid;
+		proc->backendId = MyBackendId;
+	}
+	else
+	{
+		Assert(AmStartupProcess() || !IsPostmasterEnvironment);
+		/* GetLockConflicts() uses this to specify a wait on the XID */
+		proc->lxid = xid;
+		proc->backendId = InvalidBackendId;
+	}
 	proc->xid = xid;
 	Assert(proc->xmin == InvalidTransactionId);
 	proc->delayChkpt = false;
 	proc->statusFlags = 0;
 	proc->pid = 0;
-	proc->backendId = InvalidBackendId;
 	proc->databaseId = databaseid;
 	proc->roleId = owner;
 	proc->tempNamespaceId = InvalidOid;
@@ -847,6 +857,53 @@ TwoPhaseGetGXact(TransactionId xid, bool lock_held)
 }
 
 /*
+ * TwoPhaseGetXidByVirtualXID
+ *		Lookup VXID among xacts prepared since last startup.
+ *
+ * (This won't find recovered xacts.)  If more than one matches, return any
+ * and set "have_more" to true.  To witness multiple matches, a single
+ * BackendId must consume 2^32 LXIDs, with no intervening database restart.
+ */
+TransactionId
+TwoPhaseGetXidByVirtualXID(VirtualTransactionId vxid,
+						   bool *have_more)
+{
+	int			i;
+	TransactionId result = InvalidTransactionId;
+
+	Assert(VirtualTransactionIdIsValid(vxid));
+	LWLockAcquire(TwoPhaseStateLock, LW_SHARED);
+
+	for (i = 0; i < TwoPhaseState->numPrepXacts; i++)
+	{
+		GlobalTransaction gxact = TwoPhaseState->prepXacts[i];
+		PGPROC	   *proc;
+		VirtualTransactionId proc_vxid;
+
+		if (!gxact->valid)
+			continue;
+		proc = &ProcGlobal->allProcs[gxact->pgprocno];
+		GET_VXID_FROM_PGPROC(proc_vxid, *proc);
+		if (VirtualTransactionIdEquals(vxid, proc_vxid))
+		{
+			/* Startup process sets proc->backendId to InvalidBackendId. */
+			Assert(!gxact->inredo);
+
+			if (result != InvalidTransactionId)
+			{
+				*have_more = true;
+				break;
+			}
+			result = gxact->xid;
+		}
+	}
+
+	LWLockRelease(TwoPhaseStateLock);
+
+	return result;
+}
+
+/*
  * TwoPhaseGetDummyBackendId
  *		Get the dummy backend ID for prepared transaction specified by XID
  *
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 4cc38f0..b7eed9f 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2506,6 +2506,13 @@ PrepareTransaction(void)
 	XactLastRecEnd = 0;
 
 	/*
+	 * Transfer our locks to a dummy PGPROC.  This has to be done before
+	 * ProcArrayClearTransaction().  Otherwise, a GetLockConflicts() would
+	 * conclude "xact already committed or aborted" for our locks.
+	 */
+	PostPrepare_Locks(xid);
+
+	/*
 	 * Let others know about no transaction in progress by me.  This has to be
 	 * done *after* the prepared transaction has been marked valid, else
 	 * someone may think it is unlocked and recyclable.
@@ -2544,7 +2551,6 @@ PrepareTransaction(void)
 
 	PostPrepare_MultiXact(xid);
 
-	PostPrepare_Locks(xid);
 	PostPrepare_PredicateLocks(xid);
 
 	ResourceOwnerRelease(TopTransactionResourceOwner,
diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.c
index cdf2266..2db0424 100644
--- a/src/backend/storage/lmgr/lmgr.c
+++ b/src/backend/storage/lmgr/lmgr.c
@@ -871,9 +871,10 @@ XactLockTableWaitErrorCb(void *arg)
  * To do this, obtain the current list of lockers, and wait on their VXIDs
  * until they are finished.
  *
- * Note we don't try to acquire the locks on the given locktags, only the VXIDs
- * of its lock holders; if somebody grabs a conflicting lock on the objects
- * after we obtained our initial list of lockers, we will not wait for them.
+ * Note we don't try to acquire the locks on the given locktags, only the
+ * VXIDs and XIDs of their lock holders; if somebody grabs a conflicting lock
+ * on the objects after we obtained our initial list of lockers, we will not
+ * wait for them.
  */
 void
 WaitForLockersMultiple(List *locktags, LOCKMODE lockmode, bool progress)
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 364654e..c25af7f 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -2900,8 +2900,12 @@ FastPathGetRelationLockEntry(LOCALLOCK *locallock)
  * The result array is palloc'd and is terminated with an invalid VXID.
  * *countp, if not null, is updated to the number of items set.
  *
- * Of course, the result could be out of date by the time it's returned,
- * so use of this function has to be thought about carefully.
+ * Of course, the result could be out of date by the time it's returned, so
+ * use of this function has to be thought about carefully.  Similarly, a
+ * PGPROC with no "lxid" will be considered non-conflicting regardless of any
+ * lock it holds.  Existing callers don't care about a locker after that
+ * locker's pg_xact updates complete.  CommitTransaction() clears "lxid" after
+ * pg_xact updates and before releasing locks.
  *
  * Note we never include the current xact's vxid in the result array,
  * since an xact never blocks itself.
@@ -4530,36 +4534,79 @@ VirtualXactLockTableCleanup(void)
 }
 
 /*
+ *		XactLockForVirtualXact
+ *
+ * If TransactionIdIsValid(xid), this is essentially XactLockTableWait(xid,
+ * NULL, NULL, XLTW_None) or ConditionalXactLockTableWait(xid).  Unlike those
+ * functions, it assumes "xid" is never a subtransaction and that "xid" is
+ * prepared, committed, or aborted.
+ *
+ * If !TransactionIdIsValid(xid), this locks every prepared XID having been
+ * known as "vxid" before its PREPARE TRANSACTION.
+ */
+static bool
+XactLockForVirtualXact(VirtualTransactionId vxid,
+					   TransactionId xid, bool wait)
+{
+	bool		more = false;
+
+	/* There is no point to wait for 2PCs if you have no 2PCs. */
+	if (max_prepared_xacts == 0)
+		return true;
+
+	do
+	{
+		LockAcquireResult lar;
+		LOCKTAG		tag;
+
+		/* Clear state from previous iterations. */
+		if (more)
+		{
+			xid = InvalidTransactionId;
+			more = false;
+		}
+
+		/* If we have no xid, try to find one. */
+		if (!TransactionIdIsValid(xid))
+			xid = TwoPhaseGetXidByVirtualXID(vxid, &more);
+		if (!TransactionIdIsValid(xid))
+		{
+			Assert(!more);
+			return true;
+		}
+
+		/* Check or wait for XID completion. */
+		SET_LOCKTAG_TRANSACTION(tag, xid);
+		lar = LockAcquire(&tag, ShareLock, false, !wait);
+		if (lar == LOCKACQUIRE_NOT_AVAIL)
+			return false;
+		LockRelease(&tag, ShareLock, false);
+	} while (more);
+
+	return true;
+}
+
+/*
  *		VirtualXactLock
  *
- * If wait = true, wait until the given VXID has been released, and then
- * return true.
+ * If wait = true, wait as long as the given VXID or any XID acquired by the
+ * same transaction is still running.  Then, return true.
  *
- * If wait = false, just check whether the VXID is still running, and return
- * true or false.
+ * If wait = false, just check whether that VXID or one of those XIDs is still
+ * running, and return true or false.
  */
 bool
 VirtualXactLock(VirtualTransactionId vxid, bool wait)
 {
 	LOCKTAG		tag;
 	PGPROC	   *proc;
+	TransactionId xid = InvalidTransactionId;
 
 	Assert(VirtualTransactionIdIsValid(vxid));
 
-	if (VirtualTransactionIdIsPreparedXact(vxid))
-	{
-		LockAcquireResult lar;
-
-		/*
-		 * Prepared transactions don't hold vxid locks.  The
-		 * LocalTransactionId is always a normal, locked XID.
-		 */
-		SET_LOCKTAG_TRANSACTION(tag, vxid.localTransactionId);
-		lar = LockAcquire(&tag, ShareLock, false, !wait);
-		if (lar != LOCKACQUIRE_NOT_AVAIL)
-			LockRelease(&tag, ShareLock, false);
-		return lar != LOCKACQUIRE_NOT_AVAIL;
-	}
+	if (VirtualTransactionIdIsRecoveredPreparedXact(vxid))
+		/* no vxid lock; localTransactionId is a normal, locked XID */
+		return XactLockForVirtualXact(vxid, vxid.localTransactionId, wait);
 
 	SET_LOCKTAG_VIRTUALTRANSACTION(tag, vxid);
 
@@ -4573,7 +4620,7 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait)
 	 */
 	proc = BackendIdGetProc(vxid.backendId);
 	if (proc == NULL)
-		return true;
+		return XactLockForVirtualXact(vxid, InvalidTransactionId, wait);
 
 	/*
 	 * We must acquire this lock before checking the backendId and lxid
@@ -4582,12 +4629,12 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait)
 	 */
 	LWLockAcquire(&proc->fpInfoLock, LW_EXCLUSIVE);
 
-	/* If the transaction has ended, our work here is done. */
 	if (proc->backendId != vxid.backendId
 		|| proc->fpLocalTransactionId != vxid.localTransactionId)
 	{
+		/* VXID ended */
 		LWLockRelease(&proc->fpInfoLock);
-		return true;
+		return XactLockForVirtualXact(vxid, InvalidTransactionId, wait);
 	}
 
 	/*
@@ -4634,6 +4681,16 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait)
 		proc->fpVXIDLock = false;
 	}
 
+	/*
+	 * If the proc has an XID now, we'll avoid a TwoPhaseGetXidByVirtualXID()
+	 * search.  The proc might have assigned this XID but not yet locked it,
+	 * in which case the proc will lock this XID before releasing the VXID.
+	 * The fpInfoLock critical section excludes VirtualXactLockTableCleanup(),
+	 * so we won't save an XID of a different VXID.  It doesn't matter whether
+	 * we save this before or after setting up the primary lock table entry.
+	 */
+	xid = proc->xid;
+
 	/* Done with proc->fpLockBits */
 	LWLockRelease(&proc->fpInfoLock);
 
@@ -4641,7 +4698,7 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait)
 	(void) LockAcquire(&tag, ShareLock, false, false);
 
 	LockRelease(&tag, ShareLock, false);
-	return true;
+	return XactLockForVirtualXact(vxid, xid, wait);
 }
 
 /*
diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c
index dd8586a..777fab4 100644
--- a/src/backend/utils/cache/inval.c
+++ b/src/backend/utils/cache/inval.c
@@ -65,6 +65,20 @@
  *	(XXX is it worth testing likewise for duplicate catcache flush entries?
  *	Probably not.)
  *
+ *	Many subsystems own higher-level caches that depend on relcache and/or
+ *	catcache, and they register callbacks here to invalidate their caches.
+ *	While building a higher-level cache entry, a backend may receive a
+ *	callback for the being-built entry or one of its dependencies.  This
+ *	implies the new higher-level entry would be born stale, and it might
+ *	remain stale for the life of the backend.  Many caches do not prevent
+ *	that.  They rely on DDL for can't-miss catalog changes taking
+ *	AccessExclusiveLock on suitable objects.  (For a change made with less
+ *	locking, backends might never read the change.)  The relation cache,
+ *	however, needs to reflect changes from CREATE INDEX CONCURRENTLY no later
+ *	than the beginning of the next transaction.  Hence, when a relevant
+ *	invalidation callback arrives during a build, relcache.c reattempts that
+ *	build.  Caches with similar needs could do likewise.
+ *
  *	If a relcache flush is issued for a system relation that we preload
  *	from the relcache init file, we must also delete the init file so that
  *	it will be rebuilt during the next backend restart.  The actual work of
diff --git a/src/include/access/twophase.h b/src/include/access/twophase.h
index e27e1a8..2d75842 100644
--- a/src/include/access/twophase.h
+++ b/src/include/access/twophase.h
@@ -34,6 +34,8 @@ extern void TwoPhaseShmemInit(void);
 extern void AtAbort_Twophase(void);
 extern void PostPrepare_Twophase(void);
 
+extern TransactionId TwoPhaseGetXidByVirtualXID(VirtualTransactionId vxid,
+												bool *have_more);
 extern PGPROC *TwoPhaseGetDummyProc(TransactionId xid, bool lock_held);
 extern BackendId TwoPhaseGetDummyBackendId(TransactionId xid, bool lock_held);
 
diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
index 9b2a421..a5286fa 100644
--- a/src/include/storage/lock.h
+++ b/src/include/storage/lock.h
@@ -47,10 +47,11 @@ extern bool Debug_deadlocks;
 
 /*
  * Top-level transactions are identified by VirtualTransactionIDs comprising
- * PGPROC fields backendId and lxid.  For prepared transactions, the
- * LocalTransactionId is an ordinary XID.  These are guaranteed unique over
- * the short term, but will be reused after a database restart or XID
- * wraparound; hence they should never be stored on disk.
+ * PGPROC fields backendId and lxid.  For recovered prepared transactions, the
+ * LocalTransactionId is an ordinary XID; LOCKTAG_VIRTUALTRANSACTION never
+ * refers to that kind.  These are guaranteed unique over the short term, but
+ * will be reused after a database restart or XID wraparound; hence they
+ * should never be stored on disk.
  *
  * Note that struct VirtualTransactionId can not be assumed to be atomically
  * assignable as a whole.  However, type LocalTransactionId is assumed to
@@ -70,7 +71,7 @@ typedef struct
 #define LocalTransactionIdIsValid(lxid) ((lxid) != InvalidLocalTransactionId)
 #define VirtualTransactionIdIsValid(vxid) \
 	(LocalTransactionIdIsValid((vxid).localTransactionId))
-#define VirtualTransactionIdIsPreparedXact(vxid) \
+#define VirtualTransactionIdIsRecoveredPreparedXact(vxid) \
 	((vxid).backendId == InvalidBackendId)
 #define VirtualTransactionIdEquals(vxid1, vxid2) \
 	((vxid1).backendId == (vxid2).backendId && \

view thread (107+ messages)  latest in thread

Message-ID: <20211017151205.GA2384251@rfd.leadboat.com>
Permalink:  ../20211017151205.GA2384251@rfd.leadboat.com/
Also on:    postgresql.org/message-id/20211017151205.GA2384251@rfd.leadboat.com

copy link · copy postgr.es · copy git am

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-bugs@postgresql.org
  Cc: noah@leadboat.com, x4mmm@yandex-team.ru, pgsql-bugs@lists.postgresql.org, michael@paquier.xyz, tgl@sss.pgh.pa.us, pg@bowt.ie, andres@anarazel.de
  Subject: Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data
  In-Reply-To: <20211017151205.GA2384251@rfd.leadboat.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox