Re: Rework LogicalOutputPluginWriterUpdateProgress

On Thu, Mar 9, 2023 at 10:56 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> 2. rollback_prepared_cb_wrapper
>
> /*
> * If the plugin support two-phase commits then rollback prepared callback
> * is mandatory
> + *
> + * FIXME: This should have been caught much earlier.
> */
> if (ctx->callbacks.rollback_prepared_cb == NULL)
> ereport(ERROR,
>
> ~
>
> Why is this seemingly unrelated FIXME still in the patch?
>

After reading this Fixme comment and the error message ("logical
replication at prepare time requires a %s callback
rollback_prepared_cb"), I think we can move this and a similar check
in function commit_prepared_cb_wrapper() to prepare_cb_wrapper()
function. This is because there is no use of letting prepare pass when
we can't do a rollback or commit prepared. What do you think?

>
> 4.
>
> @@ -1370,6 +1377,8 @@ stream_abort_cb_wrapper(ReorderBuffer *cache,
> ReorderBufferTXN *txn,
>
> /* Pop the error context stack */
> error_context_stack = errcallback.previous;
> +
> + UpdateProgressAndKeepalive(ctx, (txn->toptxn == NULL));
> }
>
> ~
>
> Are the double parentheses necessary?
>

Personally, I find this style easier to follow.

--
With Regards,
Amit Kapila.