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

Thanks for looking into this!

> 17 янв. 2021 г., в 12:24, Noah Misch <noah@leadboat.com> написал(а):
> 
>> --- a/src/backend/storage/lmgr/lock.c
>> +++ b/src/backend/storage/lmgr/lock.c
>> @@ -2931,15 +2929,17 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
>> 
>> 	/*
>> 	 * Allocate memory to store results, and fill with InvalidVXID.  We only
>> -	 * need enough space for MaxBackends + a terminator, since prepared xacts
>> -	 * don't count. InHotStandby allocate once in TopMemoryContext.
>> +	 * need enough space for MaxBackends + max_prepared_xacts + a
>> +	 * terminator. InHotStandby allocate once in TopMemoryContext.
>> 	 */
>> 	if (InHotStandby)
>> 	{
>> 		if (vxids == NULL)
>> 			vxids = (VirtualTransactionId *)
>> 				MemoryContextAlloc(TopMemoryContext,
>> -								   sizeof(VirtualTransactionId) * (MaxBackends + 1));
>> +								   sizeof(VirtualTransactionId) * (MaxBackends 
>> +								   + max_prepared_xacts
>> +								   + 1));
> 
> PostgreSQL typically puts the operator before the newline.  Also, please note
> the whitespace error that "git diff --check origin/master" reports.
Fixed.
> 
>> 	}
>> 	else
>> 		vxids = (VirtualTransactionId *)
> 
> This is updating only the InHotStandby branch.  Both branches need the change.
Fixed.
> 
>> @@ -4461,9 +4462,21 @@ bool
>> VirtualXactLock(VirtualTransactionId vxid, bool wait)
>> {
>> 	LOCKTAG		tag;
>> -	PGPROC	   *proc;
>> +	PGPROC	   *proc = NULL;
>> 
>> -	Assert(VirtualTransactionIdIsValid(vxid));
>> +	Assert(VirtualTransactionIdIsValid(vxid) ||
>> +			VirtualTransactionIdIsPreparedXact(vxid));
>> +
>> +	if (VirtualTransactionIdIsPreparedXact(vxid))
>> +	{
>> +		LockAcquireResult lar;
>> +		/* If it's prepared xact - just wait for it */
>> +		SET_LOCKTAG_TRANSACTION(tag, vxid.localTransactionId);
>> +		lar = LockAcquire(&tag, ShareLock, false, !wait);
>> +		if (lar == LOCKACQUIRE_OK)
> 
> This should instead test "!= LOCKACQUIRE_NOT_AVAIL", lest
> LOCKACQUIRE_ALREADY_HELD happen.  (It perhaps can't happen, but skipping the
> LockRelease() would be wrong if it did.)
I think that code that successfully acquired lock should release it. Other way we risk to release someone's else lock held for a reason. We only acquire lock to release it instantly anyway.

> 
>> +			LockRelease(&tag, ShareLock, false);
>> +		return lar != LOCKACQUIRE_NOT_AVAIL;
>> +	}
>> 
>> 	SET_LOCKTAG_VIRTUALTRANSACTION(tag, vxid);
>> 
>> diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
>> index 1c3e9c1999..cedb9d6d2a 100644
>> --- a/src/include/storage/lock.h
>> +++ b/src/include/storage/lock.h
>> @@ -70,6 +70,8 @@ typedef struct
>> #define VirtualTransactionIdIsValid(vxid) \
>> 	(((vxid).backendId != InvalidBackendId) && \
>> 	 LocalTransactionIdIsValid((vxid).localTransactionId))
>> +#define VirtualTransactionIdIsPreparedXact(vxid) \
>> +	((vxid).backendId == InvalidBackendId)
> 
> Your patch is introducing VirtualTransactionId values that represent prepared
> xacts, and it has VirtualTransactionIdIsValid() return false for those values.
> Let's make VirtualTransactionIdIsValid() return true for them, since they're
> as meaningful as any other value.  The GetLockConflicts() header comment says
> "The result array is palloc'd and is terminated with an invalid VXID."  Patch
> v4 falsifies that comment.  The array can contain many of these new "invalid"
> VXIDs, and an all-zeroes VXID terminates the array.  Rather than change the
> comment, let's change VirtualTransactionIdIsValid() to render the comment true
> again.  Most (perhaps all) VirtualTransactionIdIsValid() callers won't need to
> distinguish the prepared-transaction case.
Makes sense, fixed. I was afraid that there's something I'm not aware about. I've iterated over VirtualTransactionIdIsValid() calls and did not find suspicious cases.

> An alternative to redefining VXID this way would be to have some new type,
> each instance of which holds either a valid VXID or a valid
> prepared-transaction XID.  That alternative feels inferior to me, though.
> What do you think?
I think we should not call valid vxids "invalid".

By the way maybe rename "check-prepared-txns" to "check-prepared-xacts" for consistency?

Thanks!

Best regards, Andrey Borodin.