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

On Tue, Sep 22, 2020 at 5:18 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Sun, Sep 20, 2020 at 3:31 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> > 3.
> >
> > + /*
> > + * If it's ROLLBACK PREPARED then handle it via callbacks.
> > + */
> > + if (TransactionIdIsValid(xid) &&
> > + !SnapBuildXactNeedsSkip(ctx->snapshot_builder, buf->origptr) &&
> > + parsed->dbId == ctx->slot->data.database &&
> > + !FilterByOrigin(ctx, origin_id) &&
> > + ReorderBufferTxnIsPrepared(ctx->reorder, xid, parsed->twophase_gid))
> > + {
> > + ReorderBufferFinishPrepared(ctx->reorder, xid, buf->origptr, buf->endptr,
> > + commit_time, origin_id, origin_lsn,
> > + parsed->twophase_gid, false);
> > + return;
> > + }
> >
> >
> > I think we have already checked !SnapBuildXactNeedsSkip, parsed->dbId
> > == ctx->slot->data.database and !FilterByOrigin in DecodePrepare
> > so if those are not true then we wouldn't have prepared this
> > transaction i.e. ReorderBufferTxnIsPrepared will be false so why do we
> > need
> > to recheck this conditions.
>
> I didnt change this, as I am seeing cases where the Abort is getting
> called for transactions that needs to be skipped. I also see that the
> same check is there both in DecodePrepare and DecodeCommit.
> So, while the same transactions were not getting prepared or
> committed, it tries to get ROLLBACK PREPARED (as part of finish
> prepared handling). The check in if ReorderBufferTxnIsPrepared() is
> also not proper.
>

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 will need to relook
> this logic again in a future patch.
>

No problem. I think you can handle the other comments and then we can
come back to this and you might want to share the exact details of the
test (may be a narrow down version of the original test) and I or
someone else might be able to help you with that.

--
With Regards,
Amit Kapila.