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 doint 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.
Attachments:
v1-0001-Tidied-some-GUC-C-variable-declarations.patchapplication/octet-stream; name=v1-0001-Tidied-some-GUC-C-variable-declarations.patchDownload+11-11
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
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
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]prefer to have C initial values - /messages/by-id/Y0dgHfEGvvay5nle@paquier.xyz.
Patch 0002 adds a sanity-check function called by
InitializeGUCOptions, as suggested by Tom [2]sanity-check idea - /messages/by-id/1113448.1665717297@sss.pgh.pa.us. 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 - /messages/by-id/Y0dgHfEGvvay5nle@paquier.xyz
/messages/by-id/Y0dgHfEGvvay5nle@paquier.xyz
[2]: sanity-check idea - /messages/by-id/1113448.1665717297@sss.pgh.pa.us
/messages/by-id/1113448.1665717297@sss.pgh.pa.us
Kind Regards,
Peter Smith.
Fujitsu Australia
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]Justin's patch of 24/Oct - /messages/by-id/20221024220544.GJ16921@telsasoft.com, 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 - /messages/by-id/20221024220544.GJ16921@telsasoft.com
/messages/by-id/20221024220544.GJ16921@telsasoft.com
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v3-0003-GUC-C-variable-sanity-check.patchapplication/octet-stream; name=v3-0003-GUC-C-variable-sanity-check.patchDownload+89-12
v3-0001-GUC-tidy-some-C-variable-declarations.patchapplication/octet-stream; name=v3-0001-GUC-tidy-some-C-variable-declarations.patchDownload+6-6
v3-0002-TMP-Justins-DYNAMIC_DEFAULT-patch.patchapplication/octet-stream; name=v3-0002-TMP-Justins-DYNAMIC_DEFAULT-patch.patchDownload+70-32
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
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
+#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, NULLMy 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;
#endifOr something more indirect like:
#ifdef WIN32
#define DEFAULT_PROCESS_TITLE false
#else
#define DEFAULT_PROCESS_TITLE true
#endifbool 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
Attachments:
v5-0001-GUC-C-variable-sanity-check.patchapplication/octet-stream; name=v5-0001-GUC-C-variable-sanity-check.patchDownload+122-22
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]/messages/by-id/20221024220544.GJ16921@telsasoft.com -- Michael 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]/messages/by-id/20221024220544.GJ16921@telsasoft.com -- Michael.
[1]: /messages/by-id/20221024220544.GJ16921@telsasoft.com -- Michael
--
Michael