On Wed, Nov 10, 2021 at 12:26 PM osumi.takami...@fujitsu.com
<osumi.takami...@fujitsu.com> wrote:
>
> On Monday, November 8, 2021 10:15 PM vignesh C <vignes...@gmail.com> wrote:
> > Thanks for the updated patch. Please create a Commitfest entry for this. It
> > will
> > help to have a look at CFBot results for the patch, also if required rebase
> > and
> > post a patch on top of Head.
> As requested, created a new entry for this - [1]
>
> FYI: the skip xid patch has been updated to v20 in [2]
> but the v3 for disable_on_error is not affected by this update
> and still applicable with no regression.
>
> [1] - https://commitfest.postgresql.org/36/3407/
> [2] -
> https://www.postgresql.org/message-id/CAD21AoAT42mhcqeB1jPfRL1%2BEUHbZk8MMY_fBgsyZvJeKNpG%2Bw%40mail.gmail.com
>
I had a look at this patch and have a couple of initial review
comments for some issues I spotted:
src/backend/commands/subscriptioncmds.c
(1) bad array entry assignment
The following code block added by the patch assigns
"values[Anum_pg_subscription_subdisableonerr - 1]" twice, resulting in
it being always set to true, rather than the specified option value:
+ if (IsSet(opts.specified_opts, SUBOPT_DISABLE_ON_ERR))
+ {
+ values[Anum_pg_subscription_subdisableonerr - 1]
+ = BoolGetDatum(opts.disableonerr);
+ values[Anum_pg_subscription_subdisableonerr - 1]
+ = true;
+ }
The 2nd line is meant to instead be
"replaces[Anum_pg_subscription_subdisableonerr - 1] = true".
(compare to handling for other similar options)
src/backend/replication/logical/worker.c
(2) unreachable code?
In the patch code there seems to be some instances of unreachable code
after re-throwing errors:
e.g.
+ /* If we caught an error above, disable the subscription */
+ if (disable_subscription)
+ {
+ ReThrowError(DisableSubscriptionOnError(cctx));
+ MemoryContextSwitchTo(ecxt);
+ }
+ else
+ {
+ PG_RE_THROW();
+ MemoryContextSwitchTo(ecxt);
+ }
+ if (disable_subscription)
+ {
+ ReThrowError(DisableSubscriptionOnError(cctx));
+ MemoryContextSwitchTo(ecxt);
+ }
I'm guessing it was intended to do the "MemoryContextSwitch(ecxt);"
before re-throwing (?), but it's not really clear, as in the 1st and
3rd cases, the DisableSubscriptionOnError() calls anyway immediately
switch the memory context to cctx.
Regards,
Greg Nancarrow
Fujitsu Australia
