Horton

Thread

Commits

  1. Add check on initial and boot values when loading GUCs

  2. Clean up some inconsistencies with GUC declarations

  3. Clean up some GUC declarations and comments

  1. GUC values - recommended way to declare the C variables?

    Peter Smith <smithpb2250@gmail.com> — 2022-09-26T23:51:12Z

    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

  2. Re: GUC values - recommended way to declare the C variables?

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-27T00:08:51Z

    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

  3. Re: GUC values - recommended way to declare the C variables?

    Peter Smith <smithpb2250@gmail.com> — 2022-09-27T01:07:53Z

    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.

  4. Re: GUC values - recommended way to declare the C variables?

    Peter Smith <smithpb2250@gmail.com> — 2022-09-28T00:13:22Z

    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.

  5. Re: GUC values - recommended way to declare the C variables?

    Nathan Bossart <nathandbossart@gmail.com> — 2022-10-12T19:12:15Z

    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

  6. Re: GUC values - recommended way to declare the C variables?

    Michael Paquier <michael@paquier.xyz> — 2022-10-13T00:47:25Z

    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

  7. Re: GUC values - recommended way to declare the C variables?

    Nathan Bossart <nathandbossart@gmail.com> — 2022-10-13T21:26:35Z

    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

  8. Re: GUC values - recommended way to declare the C variables?

    Peter Smith <smithpb2250@gmail.com> — 2022-10-14T02:15:58Z

    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

  9. Re: GUC values - recommended way to declare the C variables?

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-10-14T03:14:57Z

    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

  10. Re: GUC values - recommended way to declare the C variables?

    Michael Paquier <michael@paquier.xyz> — 2022-10-14T03:56:13Z

    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

  11. Re: GUC values - recommended way to declare the C variables?

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-10-14T04:07:02Z

    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

  12. Re: GUC values - recommended way to declare the C variables?

    Peter Smith <smithpb2250@gmail.com> — 2022-10-20T00:56:58Z

    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

  13. Re: GUC values - recommended way to declare the C variables?

    Justin Pryzby <pryzby@telsasoft.com> — 2022-10-20T04:15:59Z

    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

  14. Re: GUC values - recommended way to declare the C variables?

    Peter Smith <smithpb2250@gmail.com> — 2022-10-20T07:52:00Z

    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

  15. Re: GUC values - recommended way to declare the C variables?

    Peter Smith <smithpb2250@gmail.com> — 2022-10-25T03:43:43Z

    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

  16. Re: GUC values - recommended way to declare the C variables?

    Michael Paquier <michael@paquier.xyz> — 2022-10-25T05:09:32Z

    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

  17. Re: GUC values - recommended way to declare the C variables?

    Peter Smith <smithpb2250@gmail.com> — 2022-10-25T06:11:12Z

    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

  18. Re: GUC values - recommended way to declare the C variables?

    Justin Pryzby <pryzby@telsasoft.com> — 2022-10-25T20:04:01Z

    +#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

  19. Re: GUC values - recommended way to declare the C variables?

    Peter Smith <smithpb2250@gmail.com> — 2022-10-26T07:31:56Z

    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

  20. Re: GUC values - recommended way to declare the C variables?

    Michael Paquier <michael@paquier.xyz> — 2022-10-27T02:06:56Z

    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

  21. Re: GUC values - recommended way to declare the C variables?

    Justin Pryzby <pryzby@telsasoft.com> — 2022-10-27T02:14:37Z

    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

  22. Re: GUC values - recommended way to declare the C variables?

    Michael Paquier <michael@paquier.xyz> — 2022-10-27T02:33:48Z

    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

  23. Re: GUC values - recommended way to declare the C variables?

    Justin Pryzby <pryzby@telsasoft.com> — 2022-10-27T02:49:34Z

    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

  24. Re: GUC values - recommended way to declare the C variables?

    Peter Smith <smithpb2250@gmail.com> — 2022-10-27T02:49:53Z

    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.

  25. Re: GUC values - recommended way to declare the C variables?

    Michael Paquier <michael@paquier.xyz> — 2022-10-27T02:53:37Z

    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

  26. Re: GUC values - recommended way to declare the C variables?

    Peter Smith <smithpb2250@gmail.com> — 2022-10-27T08:00:26Z

    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

  27. Re: GUC values - recommended way to declare the C variables?

    Michael Paquier <michael@paquier.xyz> — 2022-10-28T02:48:13Z

    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

  28. Re: GUC values - recommended way to declare the C variables?

    Michael Paquier <michael@paquier.xyz> — 2022-10-28T03:05:43Z

    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

  29. Re: GUC values - recommended way to declare the C variables?

    Michael Paquier <michael@paquier.xyz> — 2022-10-28T07:05:12Z

    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

  30. Re: GUC values - recommended way to declare the C variables?

    Peter Smith <smithpb2250@gmail.com> — 2022-10-31T01:01:33Z

    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

  31. Re: GUC values - recommended way to declare the C variables?

    Michael Paquier <michael@paquier.xyz> — 2022-10-31T05:02:49Z

    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

  32. Re: GUC values - recommended way to declare the C variables?

    Peter Smith <smithpb2250@gmail.com> — 2022-10-31T06:21:48Z

    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.