A couple of postgresql.conf.sample discrepancies

Started by Thomas Munroover 8 years ago4 messages
#1Thomas Munro
thomas.munro@enterprisedb.com
2 attachment(s)

Hi hackers,

Here's a script that reminds you about GUCs you forgot to put in
postgresql.conf.sample. It probably needs some work. Does this
already happen somewhere else? I guess not, because it found two
discrepancies:

$ ./src/tools/check_sample_config.pl
enable_gathermerge appears in guc.c but not in postgresql.conf.sample
trace_recovery_messages appears in guc.c but not in postgresql.conf.sample

I think the first should be listed in postgresql.conf.sample, but the
second should probably be flagged as GUC_NOT_IN_SAMPLE. See attached.

--
Thomas Munro
http://www.enterprisedb.com

Attachments:

fix-sample-config.patchapplication/octet-stream; name=fix-sample-config.patchDownload
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 82e54c084b8..be423e2c262 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3802,7 +3802,8 @@ static struct config_enum ConfigureNamesEnum[] =
 		{"trace_recovery_messages", PGC_SIGHUP, DEVELOPER_OPTIONS,
 			gettext_noop("Enables logging of recovery-related debugging information."),
 			gettext_noop("Each level includes all the levels that follow it. The later"
-						 " the level, the fewer messages are sent.")
+						 " the level, the fewer messages are sent."),
+			GUC_NOT_IN_SAMPLE
 		},
 		&trace_recovery_messages,
 
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 2b1ebb797ec..c3894b67ef7 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -287,6 +287,7 @@
 # - Planner Method Configuration -
 
 #enable_bitmapscan = on
+#enable_gathermerge = on
 #enable_hashagg = on
 #enable_hashjoin = on
 #enable_indexscan = on
check-sample-config-v1.patchapplication/octet-stream; name=check-sample-config-v1.patchDownload
diff --git a/src/tools/check_sample_config.pl b/src/tools/check_sample_config.pl
new file mode 100755
index 00000000000..4f00174691e
--- /dev/null
+++ b/src/tools/check_sample_config.pl
@@ -0,0 +1,79 @@
+#!/usr/bin/env perl
+
+#################################################################
+#
+# Check for discrepancies between guc.c and postgresql.conf.sample.
+#
+# Execute with the top of the PostgreSQL source tree as current
+# directory.
+#
+# Copyright (c) 2017, PostgreSQL Global Development Group
+#
+#################################################################
+
+use strict;
+use warnings;
+
+sub scan_sample_config {
+  my $path = "src/backend/utils/misc/postgresql.conf.sample";
+  my @result = ();
+  open my $file, $path or die "Could not open $path: $!";
+  while (my $line = <$file>) {
+    if ($line =~ /^#([^ ]+) = /) {
+      push @result, lc $1;
+    }
+  }
+  close $file;
+  return @result;
+}
+
+sub scan_gucs {
+  my $path = "src/backend/utils/misc/guc.c";
+  my @result = ();
+  my $guc = "";
+  open my $file, $path or die "Could not open $path: $!";
+  while (my $line = <$file>) {
+    if ($line =~ /^		\{ *"([^"]+)",/) {
+      $guc = $1;
+    } elsif ($line =~ /GUC_NOT_IN_SAMPLE/) {
+      $guc = "";
+    } elsif ($line =~ /^\W\},$/) {
+      if ($guc ne "") {
+        push @result, lc $guc;
+      }
+    }
+  }
+  close $file;
+  return @result;
+}
+
+sub is_in {
+  my ($needle, @haystack) = @_;
+  foreach my $value (@haystack) {
+    if ($needle eq $value) {
+      return 1;
+    }
+  }
+  return 0;
+}
+
+my @sample_config = scan_sample_config();
+my @gucs = scan_gucs();
+my @ignore = ("include", "include_if_exists", "include_dir", "config_file");
+my $fail = 0;
+
+foreach my $guc (@sample_config) {
+  if (!is_in($guc, @gucs) && !is_in($guc, @ignore)) {
+    printf "$guc appears in postgresql.conf.sample but not in guc.c\n";
+    $fail = 1;
+  }
+}
+
+foreach my $guc (@gucs) {
+  if (!is_in($guc, @sample_config) && !is_in($guc, @ignore)) {
+    printf "$guc appears in guc.c but not in postgresql.conf.sample\n";
+    $fail = 1;
+  }
+}
+
+exit 1 if $fail;
#2Aleksander Alekseev
a.alekseev@postgrespro.ru
In reply to: Thomas Munro (#1)
Re: A couple of postgresql.conf.sample discrepancies

Hi Thomas,

Here's a script that reminds you about GUCs you forgot to put in
postgresql.conf.sample. It probably needs some work. Does this
already happen somewhere else? I guess not, because it found two
discrepancies:

$ ./src/tools/check_sample_config.pl
enable_gathermerge appears in guc.c but not in postgresql.conf.sample
trace_recovery_messages appears in guc.c but not in postgresql.conf.sample

I like the idea. However maybe it worth considering to turn it into a
TAP test? Otherwise there is a good chance everybody will forget to run
it. For similar reason I would advise to add this patch to the next
commitfest.

--
Best regards,
Aleksander Alekseev

#3Michael Paquier
michael.paquier@gmail.com
In reply to: Aleksander Alekseev (#2)
Re: A couple of postgresql.conf.sample discrepancies

On Thu, Jul 27, 2017 at 10:27 AM, Aleksander Alekseev
<a.alekseev@postgrespro.ru> wrote:

Here's a script that reminds you about GUCs you forgot to put in
postgresql.conf.sample. It probably needs some work. Does this
already happen somewhere else? I guess not, because it found two
discrepancies:

$ ./src/tools/check_sample_config.pl
enable_gathermerge appears in guc.c but not in postgresql.conf.sample
trace_recovery_messages appears in guc.c but not in postgresql.conf.sample

I like the idea. However maybe it worth considering to turn it into a
TAP test? Otherwise there is a good chance everybody will forget to run
it. For similar reason I would advise to add this patch to the next
commitfest.

Bonus points if the script can detect that a parameter's comment
forgets to include "(change requires restart)".
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Robert Haas
robertmhaas@gmail.com
In reply to: Aleksander Alekseev (#2)
Re: A couple of postgresql.conf.sample discrepancies

On Thu, Jul 27, 2017 at 4:27 AM, Aleksander Alekseev
<a.alekseev@postgrespro.ru> wrote:

I like the idea. However maybe it worth considering to turn it into a
TAP test? Otherwise there is a good chance everybody will forget to run
it. For similar reason I would advise to add this patch to the next
commitfest.

Instead of adding it to a TAP test, I think we should add it to the
build process, so you get a compile failure if it finds any problems.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers