Date: Sun, 5 Dec 2021 23:38:05 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: GUC flags
Message-ID: <20211206053805.GG17618@telsasoft.com>
References: <20211129030833.GJ17618@telsasoft.com>
 <YaSJcceKORqKS+cM@paquier.xyz>
 <YaXGfc+tRYFFN+bo@paquier.xyz>
 <20211201075904.GW17618@telsasoft.com>
 <YadjSDYGxYEO5k/R@paquier.xyz>
 <20211202033439.GL17618@telsasoft.com>
 <YahVin9880J9W45Z@paquier.xyz>
 <20211202051734.GN17618@telsasoft.com>
 <Yaltp7EjWny7Gl4e@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="Zr5irScySb2At5ws"
Content-Disposition: inline
In-Reply-To: <Yaltp7EjWny7Gl4e@paquier.xyz>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20211206053805.GG17618%40telsasoft.com>
Precedence: bulk


--Zr5irScySb2At5ws
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri, Dec 03, 2021 at 10:06:47AM +0900, Michael Paquier wrote:
> On Wed, Dec 01, 2021 at 11:17:34PM -0600, Justin Pryzby wrote:
> > I find it easier to read "wait before authentication ..." than "wait ... before
> > authentication".
> 
> I have a hard time seeing a strong difference here.  At the end, I
> have used what you suggested, adjusted the rest based on your set of
> comments, and applied the patch.

Thanks.  One more item.  The check_guc script currently outputs 68 false
positives - even though it includes a list of 20 exceptions.  This is not
useful.

$ (cd ./src/backend/utils/misc/; ./check_guc) |wc -l
68

With the attached:

$ (cd ./src/backend/utils/misc/; ./check_guc)   
config_file seems to be missing from postgresql.conf.sample

That has a defacto exception for the "include" directive, which seems
reasonable.

This requires GNU awk.  I'm not sure if that's a limitation of any
significance.

-- 
Justin

--Zr5irScySb2At5ws
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-check_guc-fix-absurd-number-of-false-positives.patch"

From 8d9130a25a59630063e644a884d354eb085faa4e Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 5 Dec 2021 23:22:04 -0600
Subject: [PATCH] check_guc: fix absurd number of false positives

Avoid false positives;
Avoid various assumptions;
Avoid list of exceptions;
Simplify shell/awk/sed/grep;

This requires GNU awk for RS as a regex.
---
 src/backend/utils/misc/check_guc | 69 +++++---------------------------
 1 file changed, 10 insertions(+), 59 deletions(-)

diff --git a/src/backend/utils/misc/check_guc b/src/backend/utils/misc/check_guc
index b171ef0e4f..323ca13191 100755
--- a/src/backend/utils/misc/check_guc
+++ b/src/backend/utils/misc/check_guc
@@ -1,78 +1,29 @@
-#!/bin/sh
+#! /bin/sh
+set -e
 
-## currently, this script makes a lot of assumptions:
+## this script makes some assumptions about the formatting of files it parses.
 ## in postgresql.conf.sample:
 ##   1) the valid config settings may be preceded by a '#', but NOT '# '
 ##      (we use this to skip comments)
-##   2) the valid config settings will be followed immediately by  ' ='
-##      (at least one space preceding the '=')
-## in guc.c:
-##   3) the options have PGC_ on the same line as the option
-##   4) the options have '{' on the same line as the option
-
-##  Problems
-## 1) Don't know what to do with TRANSACTION ISOLATION LEVEL
-
-## if an option is valid but shows up in only one file (guc.c but not
-## postgresql.conf.sample), it should be listed here so that it
-## can be ignored
-INTENTIONALLY_NOT_INCLUDED="debug_deadlocks in_hot_standby \
-is_superuser lc_collate lc_ctype lc_messages lc_monetary lc_numeric lc_time \
-pre_auth_delay role seed server_encoding server_version server_version_num \
-session_authorization trace_lock_oidmin trace_lock_table trace_locks trace_lwlocks \
-trace_notify trace_userlocks transaction_isolation transaction_read_only \
-zero_damaged_pages"
 
 ### What options are listed in postgresql.conf.sample, but don't appear
 ### in guc.c?
 
-# grab everything that looks like a setting and convert it to lower case
-SETTINGS=`grep ' =' postgresql.conf.sample |
-grep -v '^# ' | # strip comments
-sed -e 's/^#//' |
-awk '{print $1}'`
-
-SETTINGS=`echo "$SETTINGS" | tr 'A-Z' 'a-z'`
+# grab everything that looks like a setting
+SETTINGS=`sed '/^#[[:alnum:]]/!d; s/^#//; s/ =.*//; /^include/d' postgresql.conf.sample`
 
 for i in $SETTINGS ; do
-  hidden=0
   ## it sure would be nice to replace this with an sql "not in" statement
-  ## it doesn't seem to make sense to have things in .sample and not in guc.c
-#  for hidethis in $INTENTIONALLY_NOT_INCLUDED ; do
-#    if [ "$hidethis" = "$i" ] ; then
-#      hidden=1
-#    fi
-#  done
-  if [ "$hidden" -eq 0 ] ; then
-    grep -i '"'$i'"' guc.c > /dev/null
-    if [ $? -ne 0 ] ; then
-      echo "$i seems to be missing from guc.c";
-    fi;
-  fi
+  grep -i "\"$i\"" guc.c >/dev/null ||
+    echo "$i seems to be missing from guc.c";
 done
 
 ### What options are listed in guc.c, but don't appear
 ### in postgresql.conf.sample?
 
 # grab everything that looks like a setting and convert it to lower case
-
-SETTINGS=`grep '{.* PGC_' guc.c | awk '{print $1}' | \
-          sed -e 's/{//g' -e 's/"//g' -e 's/,//'`
-
-SETTINGS=`echo "$SETTINGS" | tr 'A-Z' 'a-z'`
-
+SETTINGS=`gawk -F '[",]' 'BEGIN{RS="\n\t\\\\{\n"} /",[[:space:]]*PGC_.*.*gettext_noop/ && !/NOT_IN_SAMPLE/{print tolower($2)}' guc.c`
 for i in $SETTINGS ; do
-  hidden=0
-  ## it sure would be nice to replace this with an sql "not in" statement
-  for hidethis in $INTENTIONALLY_NOT_INCLUDED ; do
-    if [ "$hidethis" = "$i" ] ; then
-      hidden=1
-    fi
-  done
-  if [ "$hidden" -eq 0 ] ; then
-    grep -i '#'$i' ' postgresql.conf.sample > /dev/null
-    if [ $? -ne 0 ] ; then
-      echo "$i seems to be missing from postgresql.conf.sample";
-    fi
-  fi
+  grep "#$i " postgresql.conf.sample >/dev/null ||
+    echo "$i seems to be missing from postgresql.conf.sample";
 done
-- 
2.17.0


--Zr5irScySb2At5ws--