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

On Fri, Nov 20, 2020 at 9:12 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Fri, Nov 20, 2020 at 12:23 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Nov 19, 2020 at 2:52 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > On Thu, Nov 19, 2020 at 5:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > > I think the same check should be there in truncate as well to make the
> > > > APIs consistent and also one can use it for writing another test that
> > > > has a truncate operation.
> > >
> > > Updated the checks in both truncate callbacks (stream and non-stream).
> > > Also added a test case for testing concurrent aborts while decoding
> > > streaming TRUNCATE.
> > >
> >
> > While reviewing/editing the code in 0002-Support-2PC-txn-backend, I
> > came across the following code which seems dubious to me.
> >
> > 1.
> > + /*
> > + * If streaming, reset the TXN so that it is allowed to stream
> > + * remaining data. Streaming can also be on a prepared txn, handle
> > + * it the same way.
> > + */
> > + if (streaming)
> > + {
> > + elog(LOG, "stopping decoding of %u",txn->xid);
> > + ReorderBufferResetTXN(rb, txn, snapshot_now,
> > +   command_id, prev_lsn,
> > +   specinsert);
> > + }
> > + else
> > + {
> > + elog(LOG, "stopping decoding of %s (%u)",
> > + txn->gid != NULL ? txn->gid : "", txn->xid);
> > + ReorderBufferTruncateTXN(rb, txn, true);
> > + }
> >
> > Why do we need to handle the prepared txn case differently here? I
> > think for both cases we can call ReorderBufferResetTXN as it frees the
> > memory we should free in exceptions. Sure, there is some code (like
> > stream_stop and saving the snapshot for next run) in
> > ReorderBufferResetTXN which needs to be only called when we are
> > streaming the txn but otherwise, it seems it can be used here. We can
> > easily identify if the transaction is streamed to differentiate that
> > code path. Can you think of any other reason for not doing so?
>
> Yes, I agree with this that ReorderBufferResetTXN  needs to be called
> to free up memory after an exception.
> Will change ReorderBufferResetTXN so that it now has an extra
> parameter that indicates streaming; so that the stream_stop and saving
> of the snapshot is only done if streaming.
>

I've already made the changes for this in the patch, you can verify
the same when I'll share the new version. We don't need to pass an
extra parameter rbtx_prepared()/rbtxn_is_streamed should serve the
need.

> >
> > 2.
> > +void
> > +ReorderBufferFinishPrepared(ReorderBuffer *rb, TransactionId xid,
> > + XLogRecPtr commit_lsn, XLogRecPtr end_lsn,
> > + TimestampTz commit_time,
> > + RepOriginId origin_id, XLogRecPtr origin_lsn,
> > + char *gid, bool is_commit)
> > +{
> > + ReorderBufferTXN *txn;
> > +
> > + /*
> > + * The transaction may or may not exist (during restarts for example).
> > + * Anyway, two-phase transactions do not contain any reorderbuffers. So
> > + * allow it to be created below.
> > + */
> > + txn = ReorderBufferTXNByXid(rb, xid, true, NULL, commit_lsn,
> > + true);
> >
> > Why should we allow to create a new transaction here or in other words
> > in which cases txn won't be present? I guess this should be the case
> > with the earlier version of the patch where at prepare time we were
> > cleaning the ReorderBufferTxn.
>
> Just confirmed this, yes, you are right. Even after a restart, the
> transaction does get created again prior to this, We need not be
> creating
> it here. I will change this as well.
>

I'll take care of it along with other changes.

Thanks for the confirmation.


-- 
With Regards,
Amit Kapila.