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

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?

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.

--
With Regards,
Amit Kapila.