Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Hi!

> 15 авг. 2021 г., в 18:45, Andres Freund <andres@anarazel.de> написал(а):
> 
> Hi,
> 
> On 2021-08-15 16:09:37 +0500, Andrey Borodin wrote:
>> From 929736512ebf8eb9ac6ddaaf49b9e6148d72cfbb Mon Sep 17 00:00:00 2001
>> From: Andrey Borodin <amborodin@acm.org>
>> Date: Fri, 30 Jul 2021 14:40:16 +0500
>> Subject: [PATCH v12 2/6] PoC fix for race in RelationBuildDesc() and relcache
>> invalidation
>> 
>> ---
>> src/backend/utils/cache/relcache.c | 28 ++++++++++++++++++++++++++++
>> 1 file changed, 28 insertions(+)
>> 
>> diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
>> index 13d9994af3..7eec7b1f41 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,12 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
>> 	 */
>> 	heap_freetuple(pg_class_tuple);
>> 
>> +	if (inProgress[in_progress_offset].invalidated)
>> +		goto retry;				/* TODO free old one */
>> +	/* inProgress is in fact the stack, we can safely remove it's top */
>> +	inProgress[in_progress_offset].relid = InvalidOid;
>> +	Assert(inProgress[in_progress_offset + 1].relid == InvalidOid);
>> +
>> 	/*
>> 	 * Insert newly created relation into relcache hash table, if requested.
>> 	 *
>> @@ -2805,6 +2826,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;
>> +	}
>> }
> 
> Desperately needs comments. Without a commit message and without
> comments it's hard to review this without re-reading the entire thread -
> which approximately nobody will do.
I've added some comments. But it seems we should use dynamic allocations instead of 100-based array.

> 
> 
>> From 7e47dae2828d88ddb2161fda0c3b08a158c6cf37 Mon Sep 17 00:00:00 2001
>> From: Andrey Borodin <amborodin@acm.org>
>> Date: Sat, 7 Aug 2021 20:27:14 +0500
>> Subject: [PATCH v12 5/6] PoC fix clear xid
>> 
>> ---
>> src/backend/access/transam/xact.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
>> index 387f80419a..9b19d939eb 100644
>> --- a/src/backend/access/transam/xact.c
>> +++ b/src/backend/access/transam/xact.c
>> @@ -2500,7 +2500,6 @@ PrepareTransaction(void)
>> 	 * done *after* the prepared transaction has been marked valid, else
>> 	 * someone may think it is unlocked and recyclable.
>> 	 */
>> -	ProcArrayClearTransaction(MyProc);
>> 
>> 	/*
>> 	 * In normal commit-processing, this is all non-critical post-transaction
>> @@ -2535,6 +2534,8 @@ PrepareTransaction(void)
>> 	PostPrepare_MultiXact(xid);
>> 
>> 	PostPrepare_Locks(xid);
>> +
>> +	ProcArrayClearTransaction(MyProc);
>> 	PostPrepare_PredicateLocks(xid);
> 
> The comment above ProcArrayClearTransaction would need to be moved and
> updated...
Fixed.

> 
>> From 6db9cafd146db1a645bb6885157b0e1f032765e0 Mon Sep 17 00:00:00 2001
>> From: Andrey Borodin <amborodin@acm.org>
>> Date: Mon, 19 Jul 2021 11:50:02 +0500
>> Subject: [PATCH v12 4/6] Fix CREATE INDEX CONCURRENTLY in precence of vxids
>> converted to 2pc
> ...
> 
>> +/*
>> + * TwoPhaseGetXidByVXid
>> + *		Try to lookup for vxid among prepared xacts
>> + */
>> +XidListEntry
>> +TwoPhaseGetXidByVXid(VirtualTransactionId vxid)
>> +{
>> +	int				i;
>> +	XidListEntry	result;
>> +	result.next = NULL;
>> +	result.xid = InvalidTransactionId;
>> +
>> +	LWLockAcquire(TwoPhaseStateLock, LW_SHARED);
>> +
>> +	for (i = 0; i < TwoPhaseState->numPrepXacts; i++)
>> +	{
>> +		GlobalTransaction	gxact = TwoPhaseState->prepXacts[i];
>> +		PGPROC			   *proc = &ProcGlobal->allProcs[gxact->pgprocno];
>> +		VirtualTransactionId proc_vxid;
>> +		GET_VXID_FROM_PGPROC(proc_vxid, *proc);
>> +
>> +		if (VirtualTransactionIdEquals(vxid, proc_vxid))
>> +		{
>> +			if (result.xid != InvalidTransactionId)
>> +			{
>> +				/* Already has candidate - need to alloc some space */
>> +				XidListEntry *copy = palloc(sizeof(XidListEntry));
>> +				copy->next = result.next;
>> +				copy->xid = result.xid;
>> +				result.next = copy;
>> +			}
>> +			result.xid = gxact->xid;
>> +		}
>> +	}
>> +
>> +	LWLockRelease(TwoPhaseStateLock);
>> +
>> +	return result;
>> +}
> 
> Dynamic memory allocations while holding a fairly central lock - one
> which is now going to be more contended - doesn't seem great.
> 
> Is the memory context this is called in guaranteed to be of a proper
> duration?  Including being reset in case there's an error at some point
> before the memory is freed?
I've removed custom list and all memory allocations. If there are multiple 2PCs with same vxid - we just wait for one, then retry.

>> +/*
>> + *		WaitXact
>> + *
>> + *		Wait for xid completition if have xid. Otherwise try to find xid among
>> + *		two-phase procarray entries.
>> + */
>> +static bool WaitXact(VirtualTransactionId vxid, TransactionId xid, bool wait)
>> +{
>> +	LockAcquireResult	lar;
>> +	LOCKTAG				tag;
>> +	XidListEntry		xidlist;
>> +	XidListEntry	   *xidlist_ptr = NULL; /* pointer to TwoPhaseGetXidByVXid()s pallocs */
>> +	bool				result;
>> +
>> +	if (TransactionIdIsValid(xid))
>> +	{
>> +		/* We know exact xid - no need to search in 2PC state */
>> +		xidlist.xid = xid;
>> +		xidlist.next = NULL;
>> +	}
>> +	else
>> +	{
>> +		/* You cant have vxid among 2PCs if you have no 2PCs */
>> +		if (max_prepared_xacts == 0)
>> +			return true;
>> +
>> +		/*
>> +		 * We must search for vxids in 2pc state
>> +		 * XXX: O(N*N) complexity where N is number of prepared xacts
>> +		 */
>> +		xidlist = TwoPhaseGetXidByVXid(vxid);
>> +		/* Return if transaction is gone entirely */
>> +		if (!TransactionIdIsValid(xidlist.xid))
>> +			return true;
>> +		xidlist_ptr = xidlist.next;
>> +	}
> 
> Perhaps I missed this - but won't we constantly enter this path for
> non-2pc transactions? E.g.
I've restored heuristics that if it's not 2PC - we just exit from WaitPreparedXact().


> 
>> @@ -4573,7 +4649,7 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait)
>> 	 */
>> 	proc = BackendIdGetProc(vxid.backendId);
>> 	if (proc == NULL)
>> -		return true;
>> +		return WaitXact(vxid, InvalidTransactionId, wait);
>> 
>> 	/*
>> 	 * We must acquire this lock before checking the backendId and lxid
>> @@ -4587,9 +4663,12 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait)
>> 		|| proc->fpLocalTransactionId != vxid.localTransactionId)
>> 	{
>> 		LWLockRelease(&proc->fpInfoLock);
>> -		return true;
>> +		return WaitXact(vxid, InvalidTransactionId, wait);
>> 	}
> 
> 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.

>> diff --git a/src/include/access/twophase.h b/src/include/access/twophase.h
>> index e27e1a8fe8..a5f28d3a80 100644
>> --- a/src/include/access/twophase.h
>> +++ b/src/include/access/twophase.h
>> @@ -25,6 +25,17 @@
>>  */
>> typedef struct GlobalTransactionData *GlobalTransaction;
>> 
>> +/* 
>> + * XidListEntry is expected to be used as list very rarely. Under normal
>> + * circumstances TwoPhaseGetXidByVXid() returns only one xid.
>> + * But under certain conditions can return many xids or nothing.
>> + */
>> +typedef struct XidListEntry
>> +{
>> +	TransactionId xid;
>> +	struct XidListEntry* next;
>> +} XidListEntry;
> 
> I don't think we should invent additional ad-hoc list types.
Fixed, removed list here entirely.

I'm attaching new version. It works fine on my machines.

Thanks!

Best regards, Andrey Borodin.