Subject: Real config values for bytes needs quotes?

From: Kyotaro Horiguchi <[email-masked]>
Date: Mon, 27 Feb 2023 17:32:17 +0900 (JST)

Hello.

I found it frustrating that the line "shared_buffers = 0.1GB" in
postgresql.conf postgresql.conf was causing an error and that the
value required (additional) surrounding single quotes.  The attached
patch makes the parser accept the use of non-quoted real values
followed by a unit for such variables. I'm not sure if that syntax
fully covers the input syntax of strtod, but I beieve it is suffucient
for most use cases.

Is the following a correct English sentense?

Do you folks think this makes sense?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index 41d62a9f23..6727f1952f 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -76,7 +76,7 @@ UNIT_LETTER		[a-zA-Z]
 INTEGER			{SIGN}?({DIGIT}+|0x{HEXDIGIT}+){UNIT_LETTER}*
 
 EXPONENT		[Ee]{SIGN}?{DIGIT}+
-REAL			{SIGN}?{DIGIT}*"."{DIGIT}*{EXPONENT}?
+REAL			{SIGN}?{DIGIT}*("."{DIGIT}*{EXPONENT}?|{EXPONENT}){UNIT_LETTER}*
 
 LETTER			[A-Za-z_\200-\377]
 LETTER_OR_DIGIT [A-Za-z_0-9\200-\377]

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 2 Mar 2023 11:54:10 +0100

> fully covers the input syntax of strtod, but I beieve it is suffucient

This seems sensible to fix.  If you're not sure about the details, write 
some test cases. :)

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 03 Mar 2023 14:38:03 +0900 (JST)

At Thu, 2 Mar 2023 11:54:10 +0100, Peter Eisentraut <[email-masked]> wrote in 
> This seems sensible to fix.  If you're not sure about the details,
> write some test cases. :)

Thanks. I initially intended to limit the change for REAL to accept
units following a value. However, actually I also modified it to
accept '1e1'.

man strtod says it accepts the following format.

> The  expected  form  of the (initial portion of the) string is optional
> leading white space as recognized by isspace(3), an optional plus ('+')
> or  minus  sign  ('-')  and then either (i) a decimal number, or (ii) a
> hexadecimal number, or (iii) an infinity, or (iv) a NAN (not-a-number).
>
> A decimal number consists of a nonempty sequence of decimal digits pos‐
> sibly  containing  a  radix character (decimal point, locale-dependent,
> usually '.'), optionally followed by a  decimal  exponent.   A  decimal
> exponent  consists  of  an  'E' or 'e', followed by an optional plus or
> minus sign, followed by a nonempty  sequence  of  decimal  digits,  and
> indicates multiplication by a power of 10.

It is written in regexp as
'\s*[-+]?(\.\d+|\d+(\.\d*)?)([Ee][-+]?\d+)?'.  The leading whitespace
is unnecessary in this specific usage, and we also need to exclude
INTERGER from this set of matches. Therefore, it should be modified to
'[-+]?((\.\d+|\d+\.\d*)([Ee][-+]?\d+)?|\d+([Ee][-+]?\d+))'.

It is translated to the following BNF notation. UNIT_LETTER is also
needed.

{SIGN}?(("."{DIGIT}+|{DIGIT}+"."{DIGIT}*){EXPONENT}?|{DIGIT}+{EXPONENT}){UNIT_LETTER}*

The attached patch applies aforementioned change to guc-file.l and
includes tests for values in certain patters.

regards.

>From 9a413b791364152d44838d329f0efe3cc7db4d22 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 3 Mar 2023 13:39:26 +0900
Subject: [PATCH v1] Widen unquoted value acceptance in configuration file

Numeric configuration parameters can accept real numbers like '10.3',
but the values cannot be followed by a unit if they are not quoted. In
addition, certain formats like '1e4' are not accepted unless they are
not quoted. This commit changes the configuration file parser to
accept all of these values without the need for quotes.
---
 src/backend/utils/misc/guc-file.l             |  2 +-
 src/test/modules/test_misc/t/003_check_guc.pl | 41 +++++++++++++++++++
 2 files changed, 42 insertions(+), 1 deletion(-)

index 41d62a9f23..ca4add9710 100644
 
 EXPONENT		[Ee]{SIGN}?{DIGIT}+
-REAL			{SIGN}?{DIGIT}*"."{DIGIT}*{EXPONENT}?
+REAL			{SIGN}?(("."{DIGIT}+|{DIGIT}+"."{DIGIT}*){EXPONENT}?|{DIGIT}+{EXPONENT}){UNIT_LETTER}*
 
 LETTER			[A-Za-z_\200-\377]
 LETTER_OR_DIGIT [A-Za-z_0-9\200-\377]
diff --git a/src/test/modules/test_misc/t/003_check_guc.pl b/src/test/modules/test_misc/t/003_check_guc.pl
index e9f33f3c77..053fbe336d 100644
--- a/src/test/modules/test_misc/t/003_check_guc.pl
+++ b/src/test/modules/test_misc/t/003_check_guc.pl
@@ -106,4 +106,45 @@ foreach my $param (@sample_intersect)
 	);
 }
 
+# check if GUC parser accepts some unquoted values accepted by strtod()
+my $conffile = $node->data_dir . "/postgresql.conf";
+my $conf = slurp_file($conffile);
+## without units
+my $lines =
+  join("\n", map {"checkpoint_completion_target = $_"}
+	   ('0.1', '1.', '.1', '1e-1', '.1e0', '1.e-01'));
+## with units
+$lines .= "\n" .
+  join("\n", map {"work_mem = ${_}MB"}
+	   ('0.3', '1.', '.3', '+3e-1', '-.3e0', '1.e01'));
+$node->append_conf('postgresql.conf', $lines);
+
+$node->reload();
+## has the last value been read?
+ok ($node->poll_query_until('postgres',
+			"SELECT setting::int = 10240 FROM pg_settings WHERE name = 'work_mem'"),
+	'all variations of real number parameter values passed');
+
+# check if parser correctly rejects some invalid patterns
+my $n = 0;
+foreach my $val ('.', '.e3')
+{
+	$n++ if (test_one_value($node, $val, $conf));
+}
+ok($n == 2, "parser succesfully rejected invalid values");
+
 done_testing();
+
+sub test_one_value
+{
+  my ($node, $value, $base_conf) = @_;
+
+  open(my $fh, '>', $conffile) || die "failed to open $conffile";
+  print $fh $conf . "\nwork_mem=$value\n";
+  close($fh);
+
+  my $logstart = -s $node->logfile;
+  $node->reload();
+  return $node->wait_for_log('syntax error in file ".*postgresql.conf"',
+							 $logstart);
+}
-- 
2.31.1