On Tue, Sep 27, 2022 at 10:08 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > > Peter Smith <smithpb2...@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.
