Date: Thu, 29 Jul 2021 22:25:48 -0400
From: Noah Misch <noah@leadboat.com>
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: Michael Paquier <michael@paquier.xyz>, Tom Lane <tgl@sss.pgh.pa.us>,
	pgsql-bugs@lists.postgresql.org
Subject: Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data
Message-ID: <20210730022548.GA1940096@gust.leadboat.com>
References: <1727EC55-0709-4E06-91C9-212682A134C7@yandex-team.ru>
 <BFC46D37-D67B-42DA-8B2C-6CA761911DF7@yandex-team.ru>
 <20210720214936.GA1040253@gust.leadboat.com>
 <E50B139F-5B6B-41F2-8F6D-CC474290CCBA@yandex-team.ru>
 <20210721175530.GA1090577@gust.leadboat.com>
 <946242CB-D63A-4D7B-A5A5-485165F5B78A@yandex-team.ru>
 <20210723223021.GA1354601@gust.leadboat.com>
 <845C714B-DBCF-49E9-9F95-238DB042F926@yandex-team.ru>
 <B8B5720D-B325-42B1-A5EF-ED01725503FA@yandex-team.ru>
 <ABABD793-1438-445B-879E-457499C2FC11@yandex-team.ru>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="3V7upXqbjpZ4EhLz"
Content-Disposition: inline
In-Reply-To: <ABABD793-1438-445B-879E-457499C2FC11@yandex-team.ru>
Archived-At: 
 <https://www.postgresql.org/message-id/20210730022548.GA1940096%40gust.leadboat.com>
Precedence: bulk


--3V7upXqbjpZ4EhLz
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Jul 29, 2021 at 07:21:44PM +0500, Andrey Borodin wrote:
> Currently CIC test breaks indexes even without 2PC. How is it supposed to work if vxid stays vxid through GetLockConflicts()\WaitForLockersMultiple() barrier and then suddenly converts to xid and commits before index validated?

It's an inval race condition.  Here's the sequence of events leading to a
failure.  "i:" lines are events within the INSERT backend, and "r:" lines are
events within the REINDEX CONCURRENTLY backend:

r: Phase 2 begins.
i: Start INSERT.
i: Acquire RowExclusiveLock on "idx_ccnew".
i: RelationBuildDesc() begins for relation "idx_ccnew".
i: RelationBuildDesc() calls RelationInitIndexAccessInfo(), which loads
   indisready=f.
r: Phase 2 commits indisready=t for idx_ccnew.
r: Start waiting for the INSERT to finish.
i: LocalExecuteInvalidationMessage() calls RelationCacheInvalidateEntry() for
   the OID of relation "idx_ccnew".  Its RelationIdCacheLookup() call returns
   NULL, because nothing has passed the relation to RelationCacheInsert().
i: RelationBuildDesc(), the same call that began above, calls
   RelationCacheInsert().  This is a problem, because the entry needs
   invalidation, but we already consumed the invalidation messages.
i: Inserts heap tuple with xmin=xid1.  Does not insert to idx_ccnew.  Commits.
r: validate_index() runs, adding the index tuple for xid1.
i: Second INSERT runs, adding a heap tuple with xmin=xid2.  It uses the stale
   relcache entry having indisready=f, so this insert also does not modify
   idx_ccnew.  This time, it was responsible for making that index entry;
   nothing else will.

In summary, the relcache is deaf to invals of a given relation during
RelationBuildDesc() of that relation.  One fix would be to make
RelationBuildDesc() more like this pseudocode:

  retry:
    WatchInvalFor(targetRelId);
    ... complicated build steps that may call AcceptInvalidationMessages() ...
    if (ReceivedInvalFor(targetRelId))
      goto retry;

I'm attaching a proof-of-concept sufficient to let your v7 pgbench test pass.
Among other things, I haven't checked whether other inval message types have
the same hazard.  What alternative fix designs should we consider?

--3V7upXqbjpZ4EhLz
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="inval-build-race-v0.patch"

diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 13d9994af3..e5f8809445 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -997,9 +997,16 @@ equalRSDesc(RowSecurityDesc *rsdesc1, RowSecurityDesc *rsdesc2)
  *		(suggesting we are trying to access a just-deleted relation).
  *		Any other error is reported via elog.
  */
+typedef struct InProgressRels {
+	Oid relid;
+	bool invalidated;
+} InProgressRels;
+static InProgressRels inProgress[100];
+
 static Relation
 RelationBuildDesc(Oid targetRelId, bool insertIt)
 {
+	int in_progress_offset;
 	Relation	relation;
 	Oid			relid;
 	HeapTuple	pg_class_tuple;
@@ -1033,6 +1040,14 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
 	}
 #endif
 
+	for (in_progress_offset = 0;
+		 OidIsValid(inProgress[in_progress_offset].relid);
+		 in_progress_offset++)
+		;
+	inProgress[in_progress_offset].relid = targetRelId;
+retry:
+	inProgress[in_progress_offset].invalidated = false;
+
 	/*
 	 * find the tuple in pg_class corresponding to the given relation id
 	 */
@@ -1213,6 +1228,10 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
 	 */
 	heap_freetuple(pg_class_tuple);
 
+	if (inProgress[in_progress_offset].invalidated)
+		goto retry;				/* TODO free old one */
+	inProgress[in_progress_offset].relid = InvalidOid;
+
 	/*
 	 * Insert newly created relation into relcache hash table, if requested.
 	 *
@@ -2805,6 +2824,13 @@ RelationCacheInvalidateEntry(Oid relationId)
 		relcacheInvalsReceived++;
 		RelationFlushRelation(relation);
 	}
+	else
+	{
+		int i;
+		for (i = 0; OidIsValid(inProgress[i].relid); i++)
+			if (inProgress[i].relid == relationId)
+				inProgress[i].invalidated = true;
+	}
 }
 
 /*

--3V7upXqbjpZ4EhLz--