On Fri, Oct 30, 2020 at 9:51 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Fri, Oct 30, 2020 at 2:46 PM Ajin Cherian <itsa...@gmail.com> wrote: > > > > On Thu, Oct 29, 2020 at 11:19 PM Amit Kapila <amit.kapil...@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