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

Thanks for looking into this!

> 17 янв. 2021 г., в 12:24, Noah Misch <noah(at)leadboat(dot)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.