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

On Mon, Sep 28, 2020 at 1:13 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Wed, Sep 23, 2020 at 2:39 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> > 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.
>
> I have added a new patch for supporting 2 phase commit semantics in
> the streaming APIs for the logical decoding plugins. I have added 3
> APIs
> 1. stream_prepare
> 2. stream_commit_prepared
> 3. stream_abort_prepared
>
> I have also added the support for the new APIs in test_decoding
> plugin. I have not yet added it to pgoutpout.
>
> I have also added a fix for the error I saw while calling
> ReorderBufferCleanupTXN as part of FinishPrepared handling. As a
> result I have removed the function I added earlier,
> ReorderBufferCleanupPreparedTXN.
> Please have a look at the new changes and let me know what you think.
>
> I will continue to look at:
>
> 1. Remove snapshots on prepare truncate.
> 2. Bug seen while abort of prepared transaction, the prepared flag is
> lost, and not able to make out that it was a previously prepared
> transaction.

I have started looking into you latest patches, as of now I have a
few comments.

v6-0001

@@ -1987,7 +2072,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb,
ReorderBufferTXN *txn,
prev_lsn = change->lsn;

/* Set the current xid to detect concurrent aborts. */
- if (streaming)
+ if (streaming || rbtxn_prepared(change->txn))
{
curtxn = change->txn;
SetupCheckXidLive(curtxn->xid);
@@ -2249,7 +2334,6 @@ ReorderBufferProcessTXN(ReorderBuffer *rb,
ReorderBufferTXN *txn,
break;
}
}
-

For streaming transaction we need to check the xid everytime because
there could concurrent a subtransaction abort, but
for two-phase we don't need to call SetupCheckXidLive everytime,
because we are sure that transaction is going to be
the same throughout the processing.

Apart from this I have also noticed a couple of cosmetic changes

+ {
+ xl_xact_parsed_prepare parsed;
+ xl_xact_prepare *xlrec;
+ /* check that output plugin is capable of twophase decoding */
+ if (!ctx->enable_twophase)
+ {
+ ReorderBufferProcessXid(reorder, XLogRecGetXid(r), buf->origptr);
+ break;
+ }

One blank line after variable declations

- /* remove potential on-disk data, and deallocate */
+ /*
+ * remove potential on-disk data, and deallocate.
+ *
+ * We remove it even for prepared transactions (GID is enough to
+ * commit/abort those later).
+ */
+
ReorderBufferCleanupTXN(rb, txn);

Comment not aligned properly

v6-0003

+LookupGXact(const char *gid)
+{
+ int i;
+
+ LWLockAcquire(TwoPhaseStateLock, LW_EXCLUSIVE);
+
+ for (i = 0; i < TwoPhaseState->numPrepXacts; i++)
+ {
+ GlobalTransaction gxact = TwoPhaseState->prepXacts[i];

I think we should take LW_SHARED lowck here no?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com