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

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