Thread: GUC values - recommended way to declare the C variables?
Hi hackers.
I have a question about the recommended way to declare the C variables
used for the GUC values.
Here are some examples from the code:
~
The GUC boot values are defined in src/backend.utils/misc/guc_tables.c
e.g. See the 4, and 2 below
{
{"max_logical_replication_workers",
PGC_POSTMASTER,
REPLICATION_SUBSCRIBERS,
gettext_noop("Maximum number of logical replication worker processes."),
NULL,
},
&max_logical_replication_workers,
4, 0, MAX_BACKENDS,
NULL, NULL, NULL
},
{
{"max_sync_workers_per_subscription",
PGC_SIGHUP,
REPLICATION_SUBSCRIBERS,
gettext_noop("Maximum number of table synchronization workers per
subscription."),
NULL,
},
&max_sync_workers_per_subscription,
2, 0, MAX_BACKENDS,
NULL, NULL, NULL
},
~~
Meanwhile, the associated C variables are declared in their respective modules.
e.g. src/backend/replication/launcher.c
int max_logical_replication_workers = 4;
int max_sync_workers_per_subscription = 2;
~~
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.
Furthermore, there seems no consistency with how these C variables are
auto-initialized:
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;
b) Sometimes the static value is assigned the same hardwired value as
the GUC boot value
- See src/backend/utils/misc/guc_tables.c,
max_logical_replication_workers boot value is 4
- See src/backend/replication/launcher.c, int
max_logical_replication_workers = 4;
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.
- See src/backend/replication/launcher.c, int
max_logical_replication_workers = 4;
~
I would like to know what is the recommended way/convention to write
the C variable declarations for the GUC values.
IMO I felt the launch.c code as shown would be greatly improved simply
by starting with 0 values, and including an explanatory comment.
e.g.
CURRENT
int max_logical_replication_workers = 4;
int max_sync_workers_per_subscription = 2;
SUGGESTION
/*
* GUC variables. Initial values are assigned at startup via
InitializeGUCOptions.
*/
int max_logical_replication_workers = 0;
int max_sync_workers_per_subscription = 0;
Thoughts?
------
Kind Regards,
Peter Smith.
Fujitsu Australia
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.
regards, tom lane
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.
On Tue, Sep 27, 2022 at 11:07 AM Peter Smith <smithpb2250@gmail.com> wrote: > ... > 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. > PSA a small patch to tidy a few of the GUC C variables - adding comments and removing unnecessary declaration assignments. make check-world passed OK. ------ Kind Regards, Peter Smith. Fujitsu Australia.
Attachment
On Wed, Sep 28, 2022 at 10:13:22AM +1000, Peter Smith wrote: > PSA a small patch to tidy a few of the GUC C variables - adding > comments and removing unnecessary declaration assignments. > > make check-world passed OK. Looks reasonable to me. I've marked this as ready-for-committer. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Wed, Oct 12, 2022 at 12:12:15PM -0700, Nathan Bossart wrote: > Looks reasonable to me. I've marked this as ready-for-committer. So, the initial values of max_wal_senders and max_replication_slots became out of sync with their defaults in guc_tables.c. FWIW, I would argue the opposite way: rather than removing the initializations, I would fix and keep them as these references can be useful when browsing the area of the code related to such GUCs, without having to look at guc_tables.c for this information. -- Michael
Attachment
On Thu, Oct 13, 2022 at 09:47:25AM +0900, Michael Paquier wrote: > So, the initial values of max_wal_senders and max_replication_slots > became out of sync with their defaults in guc_tables.c. FWIW, I would > argue the opposite way: rather than removing the initializations, I > would fix and keep them as these references can be useful when > browsing the area of the code related to such GUCs, without having to > look at guc_tables.c for this information. Well, those initializations are only useful when they are kept in sync, which, as demonstrated by this patch, isn't always the case. I don't have a terribly strong opinion about this, but I'd lean towards reducing the number of places that track the default value of GUCs. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Fri, Oct 14, 2022 at 8:26 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > > On Thu, Oct 13, 2022 at 09:47:25AM +0900, Michael Paquier wrote: > > So, the initial values of max_wal_senders and max_replication_slots > > became out of sync with their defaults in guc_tables.c. FWIW, I would > > argue the opposite way: rather than removing the initializations, I > > would fix and keep them as these references can be useful when > > browsing the area of the code related to such GUCs, without having to > > look at guc_tables.c for this information. > > Well, those initializations are only useful when they are kept in sync, > which, as demonstrated by this patch, isn't always the case. I don't have > a terribly strong opinion about this, but I'd lean towards reducing the > number of places that track the default value of GUCs. > I agree if constants are used in both places then there will always be some risk they can get out of sync again. But probably it is no problem to just add #defines (e.g. in logicallauncher.h?) to be commonly used for the C variable declaration and also in the guc_tables. I chose not to do that way only because it didn't seem to be the typical convention for all the other numeric GUCs I looked at, but it's fine by me if that way is preferred ------ Kind Regards, Peter Smith. Fujitsu Australia
Peter Smith <smithpb2250@gmail.com> writes:
> I agree if constants are used in both places then there will always be
> some risk they can get out of sync again.
Yeah.
> But probably it is no problem to just add #defines (e.g. in
> logicallauncher.h?) to be commonly used for the C variable declaration
> and also in the guc_tables.
The problem is exactly that there's no great place to put those #define's,
at least not without incurring a lot of fresh #include bloat.
Also, if you did it like that, then it doesn't really address Michael's
desire to see the default value in the variable declaration.
I do lean towards having the data available, mainly because of the
fear I mentioned upthread that some GUCs may be accessed before
InitializeGUCOptions runs.
Could we fix the out-of-sync risk by having InitializeGUCOptions insist
that the pre-existing value of the variable match what is in guc_tables.c?
That may not work for string values but I think we could insist on it
for other GUC data types. For strings, maybe the rule could be "the
old value must be NULL or strcmp-equal to the boot_val".
regards, tom lane
On Thu, Oct 13, 2022 at 11:14:57PM -0400, Tom Lane wrote: > Could we fix the out-of-sync risk by having InitializeGUCOptions insist > that the pre-existing value of the variable match what is in guc_tables.c? > That may not work for string values but I think we could insist on it > for other GUC data types. For strings, maybe the rule could be "the > old value must be NULL or strcmp-equal to the boot_val". pg_strcasecmp()'d would be more flexible here? Sometimes the character casing on the values is not entirely consistent, but no objections to use something stricter, either. -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Oct 13, 2022 at 11:14:57PM -0400, Tom Lane wrote:
>> For strings, maybe the rule could be "the
>> old value must be NULL or strcmp-equal to the boot_val".
> pg_strcasecmp()'d would be more flexible here?
Don't see the point for that. The case we're talking about is
where the variable is declared like
char *my_guc_variable = "foo_bar";
where the initialization value is going to be a compile-time
constant. I don't see why we'd need to allow any difference
between that constant and the one used in guc_tables.c.
On the other hand, we could insist that string values be strcmp-equal with
no allowance for NULL. But that probably results in duplicate copies of
the string constant, and I'm not sure it buys anything in most places.
Allowing NULL doesn't seem like it creates any extra hazard for early
references, because they'd just crash if they try to use the value
while it's still NULL.
regards, tom lane
PSA v2* patches. Patch 0001 is just a minor tidy of the GUC C variables of logical replication. The C variable initial values are present again, how Michael preferred them [1]. Patch 0002 adds a sanity-check function called by InitializeGUCOptions, as suggested by Tom [2]. This is to ensure that the GUC C variable initial values are sensible and/or have not gone stale compared with the compiled-in defaults of guc_tables.c. This patch also changes some GUC C variable initial values which were already found (by this sanity-checker) to be different. ~~~ FYI, here are examples of errors when (contrived) mismatched values are detected: [postgres@CentOS7-x64 ~]$ pg_ctl -D ./MYDATAOSS/ start waiting for server to start....FATAL: GUC (PGC_INT) max_replication_slots, boot_val=10, C-var=999 stopped waiting pg_ctl: could not start server Examine the log output. [postgres@CentOS7-x64 ~]$ pg_ctl -D ./MYDATAOSS/ start waiting for server to start....FATAL: GUC (PGC_BOOL) enable_partitionwise_aggregate, boot_val=0, C-var=1 stopped waiting pg_ctl: could not start server Examine the log output. [postgres@CentOS7-x64 ~]$ pg_ctl -D ./MYDATAOSS/ start waiting for server to start....FATAL: GUC (PGC_REAL) cpu_operator_cost, boot_val=0.0025, C-var=99.99 stopped waiting pg_ctl: could not start server Examine the log output. [postgres@CentOS7-x64 ~]$ pg_ctl -D ./MYDATAOSS/ start waiting for server to start....FATAL: GUC (PGC_STRING) archive_command, boot_val=, C-var=banana stopped waiting pg_ctl: could not start server Examine the log output. [postgres@CentOS7-x64 ~]$ pg_ctl -D ./MYDATAOSS/ start waiting for server to start....FATAL: GUC (PGC_ENUM) wal_level, boot_val=1, C-var=99 stopped waiting pg_ctl: could not start server Examine the log output. ------ [1] prefer to have C initial values - https://www.postgresql.org/message-id/Y0dgHfEGvvay5nle%40paquier.xyz [2] sanity-check idea - https://www.postgresql.org/message-id/1113448.1665717297%40sss.pgh.pa.us Kind Regards, Peter Smith. Fujitsu Australia
Attachment
On Thu, Oct 20, 2022 at 11:56:58AM +1100, Peter Smith wrote: > Patch 0002 adds a sanity-check function called by > InitializeGUCOptions, as suggested by Tom [2]. This is to ensure that > the GUC C variable initial values are sensible and/or have not gone > stale compared with the compiled-in defaults of guc_tables.c. This > patch also changes some GUC C variable initial values which were > already found (by this sanity-checker) to be different. I like it. However it's fails on windows: https://cirrus-ci.com/task/5545965036765184 running bootstrap script ... FATAL: GUC (PGC_BOOL) update_process_title, boot_val=0, C-var=1 Maybe you need to exclude dynamically set gucs ? See also this other thread, where I added a flag identifying exactly that. https://commitfest.postgresql.org/40/3736/ I need to polish that patch some, but maybe it'll be useful for you, too. -- Justin
On Thu, Oct 20, 2022 at 3:16 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Thu, Oct 20, 2022 at 11:56:58AM +1100, Peter Smith wrote: > > Patch 0002 adds a sanity-check function called by > > InitializeGUCOptions, as suggested by Tom [2]. This is to ensure that > > the GUC C variable initial values are sensible and/or have not gone > > stale compared with the compiled-in defaults of guc_tables.c. This > > patch also changes some GUC C variable initial values which were > > already found (by this sanity-checker) to be different. > > I like it. > > However it's fails on windows: > > https://cirrus-ci.com/task/5545965036765184 > > running bootstrap script ... FATAL: GUC (PGC_BOOL) update_process_title, boot_val=0, C-var=1 > > Maybe you need to exclude dynamically set gucs ? > See also this other thread, where I added a flag identifying exactly > that. https://commitfest.postgresql.org/40/3736/ > I need to polish that patch some, but maybe it'll be useful for you, too. > Great, this looks very helpful. I will try again tomorrow by skipping over such GUCs. And I noticed a couple of other C initial values I had changed coincide with what you've marked as GUC_DYNAMIC_DEFAULT so I'll restore those to how they were before too. ------ Kind Regards, Peter Smith. Fujitsu Australia
On Thu, Oct 20, 2022 at 6:52 PM Peter Smith <smithpb2250@gmail.com> wrote: > > On Thu, Oct 20, 2022 at 3:16 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > > > On Thu, Oct 20, 2022 at 11:56:58AM +1100, Peter Smith wrote: > > > Patch 0002 adds a sanity-check function called by > > > InitializeGUCOptions, as suggested by Tom [2]. This is to ensure that > > > the GUC C variable initial values are sensible and/or have not gone > > > stale compared with the compiled-in defaults of guc_tables.c. This > > > patch also changes some GUC C variable initial values which were > > > already found (by this sanity-checker) to be different. > > > > I like it. > > > > However it's fails on windows: > > > > https://cirrus-ci.com/task/5545965036765184 > > > > running bootstrap script ... FATAL: GUC (PGC_BOOL) update_process_title, boot_val=0, C-var=1 > > > > Maybe you need to exclude dynamically set gucs ? > > See also this other thread, where I added a flag identifying exactly > > that. https://commitfest.postgresql.org/40/3736/ > > I need to polish that patch some, but maybe it'll be useful for you, too. > > PSA patch set v3. This is essentially the same as before except now, utilizing the GUC_DEFAULT_COMPILE flag added by Justin's patch [1], the sanity-check skips over any dynamic compiler-dependent GUCs. Patch 0001 - GUC trivial mods to logical replication GUC C var declarations Patch 0002 - (TMP) Justin's patch adds the GUC_DEFAULT_COMPILE flag support -- this is now a prerequisite for 0003 Patch 0003 - GUC sanity-check comparisons of GUC C var declarations with the GUC defaults from guc_tables.c ------ [1] Justin's patch of 24/Oct - https://www.postgresql.org/message-id/20221024220544.GJ16921%40telsasoft.com Kind Regards, Peter Smith. Fujitsu Australia
Attachment
On Tue, Oct 25, 2022 at 02:43:43PM +1100, Peter Smith wrote: > This is essentially the same as before except now, utilizing the > GUC_DEFAULT_COMPILE flag added by Justin's patch [1], the sanity-check > skips over any dynamic compiler-dependent GUCs. Yeah, this is a self-reminder that I should try to look at what's on the other thread. > Patch 0001 - GUC trivial mods to logical replication GUC C var declarations This one seems fine, so done. -- Michael
Attachment
On Tue, Oct 25, 2022 at 4:09 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Tue, Oct 25, 2022 at 02:43:43PM +1100, Peter Smith wrote: > > This is essentially the same as before except now, utilizing the > > GUC_DEFAULT_COMPILE flag added by Justin's patch [1], the sanity-check > > skips over any dynamic compiler-dependent GUCs. > > Yeah, this is a self-reminder that I should try to look at what's on > the other thread. > > > Patch 0001 - GUC trivial mods to logical replication GUC C var declarations > > This one seems fine, so done. > -- Thanks for pushing v3-0001. PSA v4. Rebased the remaining 2 patches so the cfbot can still work. ------ Kind Regards, Peter Smith. Fujitsu Australia
Attachment
+#ifdef USE_ASSERT_CHECKING
+ sanity_check_GUC_C_var(hentry->gucvar);
+#endif
=> You can conditionally define that as an empty function so #ifdefs
aren't needed in the caller:
void sanity_check_GUC_C_var()
{
#ifdef USE_ASSERT_CHECKING
...
#endif
}
+ /* Skip checking for dynamic (compiler-dependent) GUCs. */
=> This should say that the GUC's default is determined at compile-time.
But actually, I don't think you should use my patch. You needed to
exclude update_process_title:
src/backend/utils/misc/ps_status.c:bool update_process_title = true;
...
src/backend/utils/misc/guc_tables.c-#ifdef WIN32
src/backend/utils/misc/guc_tables.c- false,
src/backend/utils/misc/guc_tables.c-#else
src/backend/utils/misc/guc_tables.c- true,
src/backend/utils/misc/guc_tables.c-#endif
src/backend/utils/misc/guc_tables.c- NULL, NULL, NULL
My patch would also exclude the 16 other GUCs with compile-time defaults
from your check. It'd be better not to exclude them; I think the right
solution is to change the C variable initialization to a compile-time
constant:
#ifdef WIN32
bool update_process_title = false;
#else
bool update_process_title = true;
#endif
Or something more indirect like:
#ifdef WIN32
#define DEFAULT_PROCESS_TITLE false
#else
#define DEFAULT_PROCESS_TITLE true
#endif
bool update_process_title = DEFAULT_PROCESS_TITLE;
I suspect there's not many GUCs that would need to change - this might
be the only one. If this GUC were defined in the inverse (bool
skip_process_title), it wouldn't need special help, either.
--
Justin
Thanks for the feedback. PSA the v5 patch.
On Wed, Oct 26, 2022 at 7:04 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> +#ifdef USE_ASSERT_CHECKING
> + sanity_check_GUC_C_var(hentry->gucvar);
> +#endif
>
> => You can conditionally define that as an empty function so #ifdefs
> aren't needed in the caller:
>
> void sanity_check_GUC_C_var()
> {
> #ifdef USE_ASSERT_CHECKING
> ...
> #endif
> }
>
Fixed as suggested.
> But actually, I don't think you should use my patch. You needed to
> exclude update_process_title:
>
> src/backend/utils/misc/ps_status.c:bool update_process_title = true;
> ...
> src/backend/utils/misc/guc_tables.c-#ifdef WIN32
> src/backend/utils/misc/guc_tables.c- false,
> src/backend/utils/misc/guc_tables.c-#else
> src/backend/utils/misc/guc_tables.c- true,
> src/backend/utils/misc/guc_tables.c-#endif
> src/backend/utils/misc/guc_tables.c- NULL, NULL, NULL
>
> My patch would also exclude the 16 other GUCs with compile-time defaults
> from your check. It'd be better not to exclude them; I think the right
> solution is to change the C variable initialization to a compile-time
> constant:
>
> #ifdef WIN32
> bool update_process_title = false;
> #else
> bool update_process_title = true;
> #endif
>
> Or something more indirect like:
>
> #ifdef WIN32
> #define DEFAULT_PROCESS_TITLE false
> #else
> #define DEFAULT_PROCESS_TITLE true
> #endif
>
> bool update_process_title = DEFAULT_PROCESS_TITLE;
>
> I suspect there's not many GUCs that would need to change - this might
> be the only one. If this GUC were defined in the inverse (bool
> skip_process_title), it wouldn't need special help, either.
>
I re-checked all the GUC C vars which your patch flags as
GUC_DEFAULT_COMPILE. For some of them, where it was not any trouble, I
made the C var assignment use the same preprocessor rules as used by
guc_tables. For others (mostly the string ones) I left the GUC C var
untouched because the sanity checker function already has a rule not
to complain about int GUC C vars which are 0 or string GUC vars which
are NULL.
------
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachment
On Wed, Oct 26, 2022 at 06:31:56PM +1100, Peter Smith wrote: > I re-checked all the GUC C vars which your patch flags as > GUC_DEFAULT_COMPILE. For some of them, where it was not any trouble, I > made the C var assignment use the same preprocessor rules as used by > guc_tables. For others (mostly the string ones) I left the GUC C var > untouched because the sanity checker function already has a rule not > to complain about int GUC C vars which are 0 or string GUC vars which > are NULL. I see. So you have on this thread an independent patch to make the CF bot happy, still depend on the patch posted on [1] to bypass the changes with variables whose boot values are compilation-dependent. Is it right to believe that the only requirement here is GUC_DEFAULT_COMPILE but not GUC_DEFAULT_INITDB? The former is much more intuitive than the latter. Still, I see an inconsistency here in what you are doing here. sanity_check_GUC_C_var() would need to skip all the GUCs marked as GUC_DEFAULT_COMPILE, meaning that one could still be "fooled by a mismatched value" in these cases. We are talking about a limited set of them, but it seems to me that we have no need for this flag at all once the default GUC values are set with a #defined'd value, no? checkpoint_flush_after, bgwriter_flush_after, port and effective_io_concurrency do that, which is why v5-0001-GUC-C-variable-sanity-check.patch does its stuff only for maintenance_io_concurrency, update_process_title, assert_enabled and syslog_facility. I think that it would be simpler to have a default for these last four with a centralized definition, meaning that we would not need a GUC_DEFAULT_COMPILE at all, while the validation could be done for basically all the GUCs with default values assigned. In short, this patch has no need to depend on what's posted in [1]. [1]: https://www.postgresql.org/message-id/20221024220544.GJ16921@telsasoft.com -- Michael
Attachment
On Thu, Oct 27, 2022 at 11:06:56AM +0900, Michael Paquier wrote: > On Wed, Oct 26, 2022 at 06:31:56PM +1100, Peter Smith wrote: > > I re-checked all the GUC C vars which your patch flags as > > GUC_DEFAULT_COMPILE. For some of them, where it was not any trouble, I > > made the C var assignment use the same preprocessor rules as used by > > guc_tables. For others (mostly the string ones) I left the GUC C var > > untouched because the sanity checker function already has a rule not > > to complain about int GUC C vars which are 0 or string GUC vars which > > are NULL. > > I see. So you have on this thread an independent patch to make the CF > bot happy, still depend on the patch posted on [1] to bypass the > changes with variables whose boot values are compilation-dependent. It seems like you're reviewing the previous version of the patch, rather than the one attached to the message you responded to (which doesn't have anything to do with GUC_DEFAULT_COMPILE). I don't know what you meant by "make the CF bot happy" (?) -- Justin
On Wed, Oct 26, 2022 at 09:14:37PM -0500, Justin Pryzby wrote: > It seems like you're reviewing the previous version of the patch, rather > than the one attached to the message you responded to (which doesn't > have anything to do with GUC_DEFAULT_COMPILE). It does not seem so as things stand, I have been looking at v5-0001-GUC-C-variable-sanity-check.patch as posted by Peter here: https://www.postgresql.org/message-id/CAHut+PuCHjYXiTGdTOvHvDnjpbivLLr49gWVS+8VwnfoM4hJTw@mail.gmail.com In combination with a two-patch set as posted by you here: 0001-add-DYNAMIC_DEFAULT-for-settings-which-vary-by-.-con.patch 0002-WIP-test-guc-default-values.patch https://www.postgresql.org/message-id/20221024220544.GJ16921@telsasoft.com These are the latest patch versions posted on their respective thread I am aware of, and based on the latest updates of each thread it still looked like there was a dependency between both. So, is that the case or not? If not, sorry if I misunderstood things. > I don't know what you meant by "make the CF bot happy" (?) It is in my opinion confusing to see that the v5 posted on this thread, which was marked as ready for committer as of https://commitfest.postgresql.org/40/3934/, seem to rely on a facility that it makes no use of. Hence it looks to me that this patch has been posted as-is to allow the CF bot to pass (I would have posted that as an isolated two-patch set with the first patch introducing the flag if need be). Anyway, per my previous comments in my last message of this thread as of https://www.postgresql.org/message-id/Y1nnwFTrnL3ItleP@paquier.xyz, I don't see a need for DYNAMIC_DEFAULT from the other thread, nor do I see a need to a style like that: +/* GUC variable */ +bool update_process_title = +#ifdef WIN32 + false; +#else + true; +#endif I think that it would be cleaner to use the same approach as checking_after_flush and similar GUCs with a centralized definition, rather than spreading such style in two places for each GUC that this patch touches (aka its declaration and its default value in guc_tables.c). In any case, the patch of this thread still needs some adjustments IMO. -- Michael
Attachment
On Thu, Oct 27, 2022 at 11:33:48AM +0900, Michael Paquier wrote: > On Wed, Oct 26, 2022 at 09:14:37PM -0500, Justin Pryzby wrote: > > It seems like you're reviewing the previous version of the patch, rather > > than the one attached to the message you responded to (which doesn't > > have anything to do with GUC_DEFAULT_COMPILE). > > It does not seem so as things stand, I have been looking at > v5-0001-GUC-C-variable-sanity-check.patch as posted by Peter here: > https://www.postgresql.org/message-id/CAHut+PuCHjYXiTGdTOvHvDnjpbivLLr49gWVS+8VwnfoM4hJTw@mail.gmail.com This thread is about consistency of the global variables with what's set by the GUC infrastructure. In v4, Peter posted a 2-patch series with my patch as 001. But I pointed out that it's better to fix the initialization of the compile-time GUCs rather than exclude them from the check. Then Peter submitted v5 whcih does that, and isn't built on top of my patch. > In combination with a two-patch set as posted by you here: > 0001-add-DYNAMIC_DEFAULT-for-settings-which-vary-by-.-con.patch > 0002-WIP-test-guc-default-values.patch > https://www.postgresql.org/message-id/20221024220544.GJ16921@telsasoft.com That's a separate thread regarding consistency of the default values (annotations) shown in postgresql.conf. (I'm not sure whether or not my patch adding GUC flags is an agreed way forward, although they might turn out to be useful for other purposes). -- Justin
On Thu, Oct 27, 2022 at 1:33 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Oct 26, 2022 at 09:14:37PM -0500, Justin Pryzby wrote: > > It seems like you're reviewing the previous version of the patch, rather > > than the one attached to the message you responded to (which doesn't > > have anything to do with GUC_DEFAULT_COMPILE). > > It does not seem so as things stand, I have been looking at > v5-0001-GUC-C-variable-sanity-check.patch as posted by Peter here: > https://www.postgresql.org/message-id/CAHut+PuCHjYXiTGdTOvHvDnjpbivLLr49gWVS+8VwnfoM4hJTw@mail.gmail.com > > In combination with a two-patch set as posted by you here: > 0001-add-DYNAMIC_DEFAULT-for-settings-which-vary-by-.-con.patch > 0002-WIP-test-guc-default-values.patch > https://www.postgresql.org/message-id/20221024220544.GJ16921@telsasoft.com > > These are the latest patch versions posted on their respective thread > I am aware of, and based on the latest updates of each thread it still > looked like there was a dependency between both. So, is that the case > or not? If not, sorry if I misunderstood things. No. My v5 is no longer dependent on the other patch. > > > I don't know what you meant by "make the CF bot happy" (?) > > It is in my opinion confusing to see that the v5 posted on this > thread, which was marked as ready for committer as of > https://commitfest.postgresql.org/40/3934/, seem to rely on a facility > that it makes no use of. Hence it looks to me that this patch has > been posted as-is to allow the CF bot to pass (I would have posted > that as an isolated two-patch set with the first patch introducing the > flag if need be). Yeah, my v4 was posted along with the other GUC flag patch as a prerequisite to make the cfbot happy. This is no longer the case - v5 is a single independent patch. Sorry for the "ready for the committer" status being confusing. At that time I thought it was. > > Anyway, per my previous comments in my last message of this thread as > of https://www.postgresql.org/message-id/Y1nnwFTrnL3ItleP@paquier.xyz, > I don't see a need for DYNAMIC_DEFAULT from the other thread, nor do I > see a need to a style like that: > +/* GUC variable */ > +bool update_process_title = > +#ifdef WIN32 > + false; > +#else > + true; > +#endif > > I think that it would be cleaner to use the same approach as > checking_after_flush and similar GUCs with a centralized definition, > rather than spreading such style in two places for each GUC that this > patch touches (aka its declaration and its default value in > guc_tables.c). In any case, the patch of this thread still needs some > adjustments IMO. OK, I can make that adjustment if it is preferred. I think it is the same as what I already suggested a while ago [1] ("But probably it is no problem to just add #defines...") ------ [1] https://www.postgresql.org/message-id/1113448.1665717297%40sss.pgh.pa.us Kind Regards, Peter Smith. Fujitsu Australia.
On Wed, Oct 26, 2022 at 09:49:34PM -0500, Justin Pryzby wrote: > In v4, Peter posted a 2-patch series with my patch as 001. > But I pointed out that it's better to fix the initialization of the > compile-time GUCs rather than exclude them from the check. > Then Peter submitted v5 whcih does that, and isn't built on top of my > patch. Okidoki, thanks for the clarification. -- Michael
Attachment
On Thu, Oct 27, 2022 at 1:33 PM Michael Paquier <michael@paquier.xyz> wrote: > >... > > Anyway, per my previous comments in my last message of this thread as > of https://www.postgresql.org/message-id/Y1nnwFTrnL3ItleP@paquier.xyz, > I don't see a need for DYNAMIC_DEFAULT from the other thread, nor do I > see a need to a style like that: > +/* GUC variable */ > +bool update_process_title = > +#ifdef WIN32 > + false; > +#else > + true; > +#endif > > I think that it would be cleaner to use the same approach as > checking_after_flush and similar GUCs with a centralized definition, > rather than spreading such style in two places for each GUC that this > patch touches (aka its declaration and its default value in > guc_tables.c). In any case, the patch of this thread still needs some > adjustments IMO. PSA patch v6. The GUC defaults of guc_tables.c, and the modified GUC C var declarations now share the same common #define'd value (instead of cut/paste preprocessor code). Per Michael's suggestion [1] to use centralized definitions. ------ [1] https://www.postgresql.org/message-id/Y1nuDNZDncx7%2BA1j%40paquier.xyz Kind Regards, Peter Smith. Fujitsu Australia
Attachment
On Thu, Oct 27, 2022 at 07:00:26PM +1100, Peter Smith wrote: > The GUC defaults of guc_tables.c, and the modified GUC C var > declarations now share the same common #define'd value (instead of > cut/paste preprocessor code). Thanks. I have not looked at the checkup logic yet, but the central declarations seem rather sane, and I have a few comments about the latter. +#ifdef WIN32 +#define DEFAULT_UPDATE_PROCESS_TITLE false +#else +#define DEFAULT_UPDATE_PROCESS_TITLE true +#endif This is the kind of things I would document as a comment, say "Disabled on Windows as the performance overhead can be significant". Actually, pg_iovec.h uses WIN32 without any previous header declared, but win32.h tells a different story as of ed9b3606, where we would define WIN32 if it does not exist yet. That may impact the default depending on the environment used? I am wondering whether the top of win32.h could be removed, these days.. +#ifdef USE_PREFETCH +#define DEFAULT_EFFECTIVE_IO_CONCURRENCY 1 +#define DEFAULT_MAINTENANCE_IO_CONCURRENCY 10 +#else +#define DEFAULT_EFFECTIVE_IO_CONCURRENCY 0 +#define DEFAULT_MAINTENANCE_IO_CONCURRENCY 0 +#endif These don't make sense without prefetching available. Perhaps that's obvious enough when reading the code still I would add a small note. -- Michael
Attachment
On Fri, Oct 28, 2022 at 11:48:13AM +0900, Michael Paquier wrote: > Actually, pg_iovec.h uses WIN32 without any previous header declared, > but win32.h tells a different story as of ed9b3606, where we would > define WIN32 if it does not exist yet. Seeing all the places where pg_status.h is included, that should be fine, so please just ignore this part. -- Michael
Attachment
On Fri, Oct 28, 2022 at 11:48:13AM +0900, Michael Paquier wrote: > Thanks. I have not looked at the checkup logic yet, but the central > declarations seem rather sane, and I have a few comments about the > latter. So, I've had the energy to look at the check logic today, and noticed that, while the proposed patch is doing the job when loading the in-core GUCs, nothing is happening for the custom GUCs that could be loaded through shared_preload_libraries or just from a LOAD command. After adding an extra check in define_custom_variable() (reworking a bit the interface proposed while on it), I have found a few more issues than what's been already found on this thread: - 5 missing spots in pg_stat_statements. - 3 float rounding issues in pg_trgm. - 1 spot in pg_prewarm. - A few more that had no initialization, but these had a default of false/0/0.0 so it does not influence the end result but I have added some initializations anyway. With all that addressed, I am finishing with the attached. I have added some comments for the default definitions depending on the CFLAGS, explaining the reasons behind the choices made. The CI has produced a green run, which is not the same as the buildfarm, still gives some confidence. Thoughts? -- Michael
Attachment
On Fri, Oct 28, 2022 at 6:05 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Oct 28, 2022 at 11:48:13AM +0900, Michael Paquier wrote:
> > Thanks. I have not looked at the checkup logic yet, but the central
> > declarations seem rather sane, and I have a few comments about the
> > latter.
>
> So, I've had the energy to look at the check logic today, and noticed
> that, while the proposed patch is doing the job when loading the
> in-core GUCs, nothing is happening for the custom GUCs that could be
> loaded through shared_preload_libraries or just from a LOAD command.
>
> After adding an extra check in define_custom_variable() (reworking a
> bit the interface proposed while on it), I have found a few more
> issues than what's been already found on this thread:
> - 5 missing spots in pg_stat_statements.
> - 3 float rounding issues in pg_trgm.
> - 1 spot in pg_prewarm.
> - A few more that had no initialization, but these had a default of
> false/0/0.0 so it does not influence the end result but I have added
> some initializations anyway.
>
> With all that addressed, I am finishing with the attached. I have
> added some comments for the default definitions depending on the
> CFLAGS, explaining the reasons behind the choices made. The CI has
> produced a green run, which is not the same as the buildfarm, still
> gives some confidence.
>
> Thoughts?
LGTM.
The patch was intended to expose mismatches, and it seems to be doing
that job already...
I only had some nitpicks for a couple of the new comments, below:
======
1. src/include/storage/bufmgr.h
+
+/* effective when prefetching is available */
+#ifdef USE_PREFETCH
+#define DEFAULT_EFFECTIVE_IO_CONCURRENCY 1
+#define DEFAULT_MAINTENANCE_IO_CONCURRENCY 10
+#else
+#define DEFAULT_EFFECTIVE_IO_CONCURRENCY 0
+#define DEFAULT_MAINTENANCE_IO_CONCURRENCY 0
+#endif
Maybe avoid the word "effective" since that is also one of the GUC names.
Use uppercase.
SUGGESTION
/* Only applicable when prefetching is available */
======
2. src/include/utils/ps_status.h
+/* Disabled on Windows as the performance overhead can be significant */
+#ifdef WIN32
+#define DEFAULT_UPDATE_PROCESS_TITLE false
+#else
+#define DEFAULT_UPDATE_PROCESS_TITLE true
+#endif
extern PGDLLIMPORT bool update_process_title;
Perhaps put that comment inside the #ifdef WIN32
SUGGESTION
#ifdef WIN32
/* Disabled on Windows because the performance overhead can be significant */
#define DEFAULT_UPDATE_PROCESS_TITLE false
#else
...
======
src/backend/utils/misc/guc.c
3. InitializeGUCOptions
@@ -1413,6 +1496,9 @@ InitializeGUCOptions(void)
hash_seq_init(&status, guc_hashtab);
while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL)
{
+ /* check mapping between initial and default value */
+ Assert(check_GUC_init(hentry->gucvar));
+
Use uppercase.
Minor re-wording.
SUGGESTION
/* Check the GUC default and declared initial value for consistency */
~~~
4. define_custom_variable
Same as #3.
------
Kind Regards,
Peter Smith
Fujitsu Australia
On Mon, Oct 31, 2022 at 12:01:33PM +1100, Peter Smith wrote: > SUGGESTION > /* Only applicable when prefetching is available */ Thanks for the suggestion. Done this way, then. > +/* Disabled on Windows as the performance overhead can be significant */ > +#ifdef WIN32 > +#define DEFAULT_UPDATE_PROCESS_TITLE false > +#else > +#define DEFAULT_UPDATE_PROCESS_TITLE true > +#endif > extern PGDLLIMPORT bool update_process_title; > > Perhaps put that comment inside the #ifdef WIN32 I'd keep that externally, as ps_status.h does so. > [...] > SUGGESTION > /* Check the GUC default and declared initial value for consistency */ Okay, fine by me. I have split the change into two parts at the end: one to refactor and fix the C declarations, and a second to introduce the check routine with all the correct declarations in place. FWIW, I have been testing that with my own in-house modules and it has caught a few stupid inconsistencies. Let's see how it goes. -- Michael
Attachment
On Mon, Oct 31, 2022 at 4:02 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Oct 31, 2022 at 12:01:33PM +1100, Peter Smith wrote: > > SUGGESTION > > /* Only applicable when prefetching is available */ > > Thanks for the suggestion. Done this way, then. > > > +/* Disabled on Windows as the performance overhead can be significant */ > > +#ifdef WIN32 > > +#define DEFAULT_UPDATE_PROCESS_TITLE false > > +#else > > +#define DEFAULT_UPDATE_PROCESS_TITLE true > > +#endif > > extern PGDLLIMPORT bool update_process_title; > > > > Perhaps put that comment inside the #ifdef WIN32 > > I'd keep that externally, as ps_status.h does so. > > > [...] > > SUGGESTION > > /* Check the GUC default and declared initial value for consistency */ > > Okay, fine by me. > > I have split the change into two parts at the end: one to refactor and > fix the C declarations, and a second to introduce the check routine > with all the correct declarations in place. > > FWIW, I have been testing that with my own in-house modules and it has > caught a few stupid inconsistencies. Let's see how it goes. Thanks for pushing. ------ Kind Regards, Peter Smith. Fujitsu Australia.