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

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.

Let me know your thoughts on these two approaches or any other
suggestions on this.

regards,
Ajin Cherian
Fujitsu Australia