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

On Fri, Oct 30, 2020 at 9:51 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Oct 30, 2020 at 2:46 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > On Thu, Oct 29, 2020 at 11:19 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > >
> > > 6.
> > > +pg_decode_stream_prepare(LogicalDecodingContext *ctx,
> > > + ReorderBufferTXN *txn,
> > > + XLogRecPtr prepare_lsn)
> > > +{
> > > + TestDecodingData *data = ctx->output_plugin_private;
> > > +
> > > + if (data->skip_empty_xacts && !data->xact_wrote_changes)
> > > + return;
> > > +
> > > + OutputPluginPrepareWrite(ctx, true);
> > > +
> > > + if (data->include_xids)
> > > + appendStringInfo(ctx->out, "preparing streamed transaction TXN %u", txn->xid);
> > > + else
> > > + appendStringInfo(ctx->out, "preparing streamed transaction");
> > >
> > > I think we should include 'gid' as well in the above messages.
> >
> > Updated.
> >
>
> gid needs to be included in the case of 'include_xids' as well.
>

Updated.

> > >
> > > 7.
> > > @@ -221,12 +235,26 @@ StartupDecodingContext(List *output_plugin_options,
> > >   ctx->streaming = (ctx->callbacks.stream_start_cb != NULL) ||
> > >   (ctx->callbacks.stream_stop_cb != NULL) ||
> > >   (ctx->callbacks.stream_abort_cb != NULL) ||
> > > + (ctx->callbacks.stream_prepare_cb != NULL) ||
> > >   (ctx->callbacks.stream_commit_cb != NULL) ||
> > >   (ctx->callbacks.stream_change_cb != NULL) ||
> > >   (ctx->callbacks.stream_message_cb != NULL) ||
> > >   (ctx->callbacks.stream_truncate_cb != NULL);
> > >
> > >   /*
> > > + * To support two-phase logical decoding, we require
> > > prepare/commit-prepare/abort-prepare
> > > + * callbacks. The filter-prepare callback is optional. We however
> > > enable two-phase logical
> > > + * decoding when at least one of the methods is enabled so that we
> > > can easily identify
> > > + * missing methods.
> > > + *
> > > + * We decide it here, but only check it later in the wrappers.
> > > + */
> > > + ctx->twophase = (ctx->callbacks.prepare_cb != NULL) ||
> > > + (ctx->callbacks.commit_prepared_cb != NULL) ||
> > > + (ctx->callbacks.rollback_prepared_cb != NULL) ||
> > > + (ctx->callbacks.filter_prepare_cb != NULL);
> > > +
> > >
> > > I think stream_prepare_cb should be checked for the 'twophase' flag
> > > because we won't use this unless two-phase is enabled. Am I missing
> > > something?
> >
> > Was fixed in v14.
> >
>
> But you still have it in the streaming check. I don't think we need
> that for the streaming case.
>

Updated.

> Few other comments on v15-0002-Support-2PC-txn-backend-and-tests:
> ======================================================================
> 1. The functions DecodeCommitPrepared and DecodeAbortPrepared have a
> lot of code similar to DecodeCommit/Abort. Can we merge these
> functions?

Merged the two functions into DecodeCommit and DecodeAbort..

>
> 2.
> DecodeCommitPrepared()
> {
> ..
> + * If filter check present and this needs to be skipped, do a regular commit.
> + */
> + if (ctx->callbacks.filter_prepare_cb &&
> + ReorderBufferPrepareNeedSkip(ctx->reorder, xid, parsed->twophase_gid))
> + {
> + ReorderBufferCommit(ctx->reorder, xid, buf->origptr, buf->endptr,
> + commit_time, origin_id, origin_lsn);
> + }
> + else
> + {
> + ReorderBufferFinishPrepared(ctx->reorder, xid, buf->origptr, buf->endptr,
> + commit_time, origin_id, origin_lsn,
> + parsed->twophase_gid, true);
> + }
> +
> +}
>
> Can we expand the comment here to say why we need to do ReorderBufferCommit?

Updated.

>
> 3. There are a lot of test cases in this patch which is a good thing
> but can we split them into a separate patch for the time being as I
> would like to focus on the core logic of the patch first. We can later
> see if we need to retain all or part of those tests.

Split the patch and created a new patch for test_decoding tests.

>
> 4. Please run pgindent on your patches.

Have not done this. Will do this, after unifiying the patchset.

regards,
Ajin Cherian
Fujitsu Australia