On Fri, Oct 28, 2022 at 6:05 PM Michael Paquier <mich...@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
