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

> 21 дек. 2020 г., в 10:40, Michael Paquier <michael@paquier.xyz> написал(а):
>
>> In general, I wonder whether WaitForLockersMultiple and GetLockConflicts
>> need to gain an additional parameter indicating whether to consider
>> prepared xacts.  It's not clear to me that their current behavior is wrong
>> for all possible uses.
>
> WaitForLockers is used only by REINDEX and CREATE/DROP CONCURRENTLY,
> where it seems to me we need to care about all the cases related to
> concurrent build, validation and index drop.  The other caller of
> GetLockConflicts() is for conflict resolution in standbys where it is
> fine to ignore 2PC transactions as these cannot be cancelled.

I don't think that fact that we cannot cancel transaction is sufficient here to ignore prepared transaction. I think
thereshould not exist any prepared transaction that we need to cancel in standby conflict resolution. And if it exists
-it's a sign of corruption, we could emit warning or something like that. 
But I'm really not an expert here, just a common sense that prepared transaction is just like regular transaction that
survivescrash. If we wait for any transaction - probably we should wait for prepared too. I'm not insisting on anything
though.

>  So I
> agree that we are going to need more control with a new option
> argument to be able to control if 2PC transactions are ignored or
> not.
>
> Hmm.  The approach taken by the patch looks to be back-patchable.
> Based on the lack of complaints on the matter, we could consider
> instead putting an error in WaitForLockersMultiple() if there is at
> least one numPrepXact which would at least avoid inconsistent data.
> But I don't think what's proposed here is bad either.
>
> VirtualTransactionIdIsValidOrPreparedXact() is confusing IMO, knowing
> that VirtualTransactionIdIsPreparedXact() combined with
> LocalTransactionIdIsValid() would be enough to do the job.
>
> -       Assert(VirtualTransactionIdIsValid(vxid));
> +       Assert(VirtualTransactionIdIsValidOrPreparedXact(vxid));
> +
> +       if (VirtualTransactionIdIsPreparedXact(vxid))
> [...]
> #define VirtualTransactionIdIsPreparedXact(vxid) \
>    ((vxid).backendId == InvalidBackendId)
> This would allow the case where backendId and localTransactionId are
> both invalid.  So it would be better to also check in
> VirtualTransactionIdIsPreparedXact() that the XID is not invalid, no?
Seems valid. Removed VirtualTransactionIdIsValidOrPreparedXact() from patch.

Thanks!

Best regards, Andrey Borodin.