On Thu, Mar 9, 2023 at 10:56 AM Peter Smith <smithpb2250@gmail.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.
