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(). At least that would be consistent with what
we do for autovacuum as the apply worker is just a bgworker.
--
Michael