Re: [HACKERS] logical decoding of two-phase transactions

On Tue, Sep 29, 2020 at 5:08 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Wed, Sep 23, 2020 at 2:39 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> > If the transaction is prepared which you can ensure via
> > ReorderBufferTxnIsPrepared() (considering you have a proper check in
> > that function), it should not require skipping the transaction in
> > Abort. One way it could happen is if you clean up the ReorderBufferTxn
> > in Prepare which you were doing in earlier version of patch which I
> > pointed out was wrong, if you have changed that then I don't know why
> > it could fail, may be someplace else during prepare the patch is
> > freeing it. Just check that.
>
> I had a look at this problem. The problem happens when decoding is
> done after a prepare but before the corresponding rollback
> prepared/commit prepared.
> For eg:
>
> Begin;
> <change 1>
> <change 2>
> PREPARE TRANSACTION '<prepare#1>';
> SELECT data FROM pg_logical_slot_get_changes(...);
> :
> :
> ROLLBACK PREPARED '<prepare#1>';
> SELECT data FROM pg_logical_slot_get_changes(...);
>
> Since the prepare is consumed in the first call to
> pg_logical_slot_get_changes, subsequently when it is encountered in
> the second call, it is skipped (as already decoded) in DecodePrepare
> and the txn->flags are not set to
> reflect the fact that it was prepared. The same behaviour is seen when
> it is commit prepared after the original prepare was consumed.
> Initially I was thinking about the following approach to fix it in DecodePrepare
> Approach 1:
>        1. Break the big Skip check in DecodePrepare into 2 parts.
>           Return if the following conditions are true:
>          If (parsed->dbId != InvalidOid && parsed->dbId !=
> ctx->slot->data.database) ||
>            ctx->fast_forward || FilterByOrigin(ctx, origin_id))
>
>       2. Check If this condition is true:
>           SnapBuildXactNeedsSkip(ctx->snapshot_builder, buf->origptr)
>
>           Then this means we are skipping because this has already
> been decoded, then instead of returning, call a new function
> ReorderBufferMarkPrepare() which will only update the flags in the txn
> to indicate that the transaction is prepared
>           Then later in DecodeAbort or DecodeCommit, we can confirm
> that the transaction has been Prepared by checking if the flag is set
> and call ReorderBufferFinishPrepared appropriately.
>
> But then, thinking about this some more, I thought of a second approach.
> Approach 2:
>         If the only purpose of all this was to differentiate between
> Abort vs Rollback Prepared and Commit vs Commit Prepared, then we dont
> need this. We already know the exact operation
>         in DecodeXactOp and can differentiate there. We only
> overloaded DecodeAbort and DecodeCommit for convenience, we can always
> call these functions with an extra flag to denote that we are either
> commit or aborting a
>         previously prepared transaction and call
> ReorderBufferFinishPrepared accordingly.
>

The second approach sounds better but you can see if there is not much
you want to reuse from DecodeCommit/DecodeAbort then you can even
write new functions DecodeCommitPrepared/DecodeAbortPrepared. OTOH, if
there is a common code among them then passing the flag would be a
better way.

-- 
With Regards,
Amit Kapila.