|From:||Andrey Borodin <x4mmm(at)yandex-team(dot)ru>|
|To:||Michael Paquier <michael(at)paquier(dot)xyz>|
|Cc:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-bugs(at)lists(dot)postgresql(dot)org|
|Subject:||Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
> 21 дек. 2020 г., в 10:40, Michael Paquier <michael(at)paquier(dot)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 there should 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 survives crash. 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
> 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.
Best regards, Andrey Borodin.
|Next Message||Andrey Borodin||2020-12-21 08:19:59||Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data|
|Previous Message||Michael Paquier||2020-12-21 07:13:17||Re: BUG #16780: Inconsistent recovery_target_xid handling across platforms|