On Sun, Nov 5, 2023 at 5:33 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Nov 03, 2023 at 01:33:26PM +1100, Peter Smith wrote:
> > On Fri, Nov 3, 2023 at 1:11 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >> Now, that Michael also committed another similar change in commit
> >> 7021d3b176, it is better to be consistent in both cases. So, either we
> >
> > I agree. Both patches are setting a special GUC value at the command
> > line, and both of them don't want the user to somehow override that.
> > Since the requirements are the same, I felt the implementations
> > (regardless if they use a guc hook or something else) should also be
> > done the same way. Yesterday I posted a review comment on the other
> > thread [1] (#2c) trying to express the same point about consistency.
>
> Yeah, I certainly agree about consistency in the implementation for
> both sides of the coin.
>
> Nevertheless, I'm still +-0 on the GUC hook addition as I am wondering
> if there could be a case where one would be interested in enforcing
> the state of the GUCs anyway, and we'd prevent entirely that. Another
> thing that we can do for max_logical_replication_workers, rather than
> a GUC hook, is to add a check on IsBinaryUpgrade in
> ApplyLauncherRegister().
>
Do you mean to say that if 'IsBinaryUpgrade' is true then let's not
allow to launch launcher or apply worker? If so, I guess this won't be
any better than prohibiting at an early stage or explicitly overriding
those with internal values and documenting it, at least that way we
can be consistent for both variables (max_logical_replication_workers
and max_slot_wal_keep_size).
--
With Regards,
Amit Kapila.