On Thu, Sep 17, 2020 at 2:02 PM Ajin Cherian <itsa...@gmail.com> wrote:
>
> On Tue, Sep 15, 2020 at 10:43 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>>
>>
>> Few other comments:
>> ===================
>> 1.
>> ReorderBufferProcessTXN()
>> {
>> ..
>> if (streaming)
>> {
>> ReorderBufferTruncateTXN(rb, txn);
>>
>> /* Reset the CheckXidAlive */
>> CheckXidAlive = InvalidTransactionId;
>> }
>> else
>> ReorderBufferCleanupTXN(rb, txn);
>> ..
>> }
>>
>> I don't think we can perform ReorderBufferCleanupTXN for the prepared
>> transactions because if we have removed the ReorderBufferTxn before
>> commit, the later code might not consider such a transaction in the
>> system and compute the wrong value of restart_lsn for a slot.
>> Basically, in SnapBuildProcessRunningXacts() when we call
>> ReorderBufferGetOldestTXN(), it should show the ReorderBufferTxn of
>> the prepared transaction which is not yet committed but because we
>> have removed it after prepare, it won't get that TXN and then that
>> leads to wrong computation of restart_lsn. Once we start from a wrong
>> point in WAL, the snapshot built was incorrect which will lead to the
>> wrong result. This is the same reason why the patch is not doing
>> ReorderBufferForget in DecodePrepare when we decide to skip the
>> transaction. Also, here, we need to set CheckXidAlive =
>> InvalidTransactionId; for prepared xact as well.
>>
>>
>
> Just to confirm what you are expecting here. so after we send out the prepare
> transaction to the plugin, you are suggesting to NOT do a
> ReorderBufferCleanupTXN, but what to do instead?. Are you suggesting to do
> what you suggested
> as part of concurrent abort handling?
>
Yes.
> Something equivalent to ReorderBufferTruncateTXN()? remove all changes of the
> transaction but keep the invalidations and tuplecids etc?
>
I don't think you don't need tuplecids. I have checked
ReorderBufferFinishPrepared() and that seems to require only
invalidations, check if anything else is required.
> Do you think we should have a new flag in txn to indicate that this
> transaction has already been decoded? (prepare_decoded?)
>
Yeah, I think that would be better. How about if name the new variable
as cleanup_prepared?
--
With Regards,
Amit Kapila.
