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

On Fri, Nov 20, 2020 at 12:23 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Nov 19, 2020 at 2:52 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> > On Thu, Nov 19, 2020 at 5:06 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.

>
> 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.

regards,
Ajin Cherian
Fujitsu Australia