On Tue, Sep 27, 2022 at 10:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Smith <smithpb2250@gmail.com> writes:
> > It seems confusing to me that for the above code the initial value is
> > "hardwired" in multiple places. Specifically, it looks tempting to
> > just change the variable declaration value, but IIUC that's going to
> > achieve nothing because it will just be overwritten by the
> > "boot-value" during the GUC mechanism start-up.
>
> Well, if you try that you'll soon discover it doesn't work ;-)
>
> IIRC, the primary argument for hand-initializing GUC variables is to
> ensure that they have a sane value even before InitializeGUCOptions runs.
> Obviously, that only matters for some subset of the GUCs that could be
> consulted very early in startup ... but it's not perfectly clear just
> which ones it matters for.
>
> > a) Sometimes the static variable is assigned some (dummy?) value that
> > is not the same as the boot value
> > - See src/backend/utils/misc/guc_tables.c, max_replication_slots boot
> > value is 10
> > - See src/backend/replication/slot.c, int max_replication_slots = 0;
>
> That seems pretty bogus. I think if we're not initializing a GUC to
> the "correct" value then we should just leave it as not explicitly
> initialized.
>
> > c) Sometimes the GUC C variables don't even have a comment saying that
> > they are GUC variables, so it is not at all obvious their initial
> > values are going to get overwritten by some external mechanism.
>
> That's flat out sloppy commenting. There are a lot of people around
> here who seem to think comments are optional :-(
>
> > SUGGESTION
> > /*
> > * GUC variables. Initial values are assigned at startup via
> > InitializeGUCOptions.
> > */
> > int max_logical_replication_workers = 0;
> > int max_sync_workers_per_subscription = 0;
>
> 1. Comment far wordier than necessary. In most places we just
> annotate these as "GUC variables", and I think that's sufficient.
> You're going to have a hard time getting people to write more
> than that anyway.
>
> 2. I don't agree with explicitly initializing to a wrong value.
> It'd be sufficient to do
>
> int max_logical_replication_workers;
> int max_sync_workers_per_subscription;
>
> which would also make it clearer that initialization happens
> through some other mechanism.
>
Thanks for your advice.
I will try to post a patch in the new few days to address (per your
suggestions) some of the variables that I am more familiar with.
------
Kind Regards,
Peter Smith.
Fujitsu Australia.
