Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data - Mailing list pgsql-bugs

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
heldfor 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.