Re: repeated decoding of prepared transactions

On Sat, Feb 20, 2021 at 9:46 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Feb 19, 2021 at 8:21 AM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > Based on this suggestion, I have created a patch on HEAD which now
> > does not allow repeated decoding
> > of prepared transactions. For this, the code now enforces
> > full_snapshot if two-phase decoding is enabled.
> > Do have a look at the patch and see if you have any comments.
> >
>
> Few minor comments:
> ===================
> 1.
> .git/rebase-apply/patch:135: trailing whitespace.
>          * We need to mark the transaction as prepared, so that we
> don't resend it on
> warning: 1 line adds whitespace errors.
>
> Whitespace issue.
>
> 2.
> /*
> + * Set snapshot type
> + */
> +void
> +SetSnapBuildType(SnapBuild *builder, bool need_full_snapshot)
>
> There is no caller which passes the second parameter as false, so why
> have it? Can't we have a function with SetSnapBuildFullSnapshot or
> something like that?
>
> 3.
> @@ -431,6 +431,10 @@ CreateInitDecodingContext(const char *plugin,
>   startup_cb_wrapper(ctx, &ctx->options, true);
>   MemoryContextSwitchTo(old_context);
>
> + /* If two-phase is on, then only full snapshot can be used */
> + if (ctx->twophase)
> + SetSnapBuildType(ctx->snapshot_builder, true);
> +
>   ctx->reorder->output_rewrites = ctx->options.receive_rewrites;
>
>   return ctx;
> @@ -534,6 +538,10 @@ CreateDecodingContext(XLogRecPtr start_lsn,
>
>   ctx->reorder->output_rewrites = ctx->options.receive_rewrites;
>
> + /* If two-phase is on, then only full snapshot can be used */
> + if (ctx->twophase)
> + SetSnapBuildType(ctx->snapshot_builder, true);
>
> I think it is better to add a detailed comment on why we are doing
> this? You can write the comment in one of the places.
>

Few more comments:
==================
1. I think you need to update the examples in the docs as well [1].
2. Also the text in the description of begin_prepare_cb [2] needs some
adjustment. We can say something on lines that if users want they can
check if the same GID exists and then they can either error out or
take appropriate action based on their need.

[1] - https://www.postgresql.org/docs/devel/logicaldecoding-example.html
[2] - https://www.postgresql.org/docs/devel/logicaldecoding-output-plugin.html

-- 
With Regards,
Amit Kapila.