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

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
Date: 2020-12-21 07:24:55
Message-ID: FD04942E-BC4E-42FE-82DF-75D28E3093AC@yandex-team.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs

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

Attachment Content-Type Size
v3-0001-Wait-for-prepared-xacts-in-CREATE-INDEX-CONCURREN.patch application/octet-stream 5.2 KB
v3-0002-Add-test-for-CIC-with-prepared-xacts.patch application/octet-stream 3.1 KB

In response to

Responses

Browse pgsql-bugs by date

  From Date Subject
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