Subject: initdb's -c option behaves wrong way?

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 28 Sep 2023 16:49:04 +0900 (JST)

Hello.

I noticed that -c option of initdb behaves in an unexpected
manner. Identical variable names with variations in letter casing are
treated as distinct variables.

$ initdb -cwork_mem=100 -cWORK_MEM=1000 -cWork_mem=2000
...
$ grep -i 'work_mem ' $PGDATA/postgresql.conf
work_mem = 100                          # min 64kB
#maintenance_work_mem = 64MB            # min 1MB
#autovacuum_work_mem = -1               # min 1MB, or -1 to use maintenance_work_mem
#logical_decoding_work_mem = 64MB       # min 64kB
WORK_MEM = 1000
Work_mem = 2000

The original intention was apparently to overwrite the existing
line. Furthermore, I surmise that preserving the original letter
casing is preferable.

Attached is a patch to address this issue.  To retrieve the variable
name from the existing line, the code is slightly restructured.
Alternatively, should we just down-case the provided variable names?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 0c6f5ceb0a..04419840f4 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -470,20 +470,14 @@ replace_guc_value(char **lines, const char *guc_name, const char *guc_value,
 	int			namelen = strlen(guc_name);
 	PQExpBuffer newline = createPQExpBuffer();
 	int			i;
+	const char *where;
+	const char *pname;
 
-	/* prepare the replacement line, except for possible comment and newline */
 	if (mark_as_comment)
 		appendPQExpBufferChar(newline, '#');
-	appendPQExpBuffer(newline, "%s = ", guc_name);
-	if (guc_value_requires_quotes(guc_value))
-		appendPQExpBuffer(newline, "'%s'", escape_quotes(guc_value));
-	else
-		appendPQExpBufferStr(newline, guc_value);
 
 	for (i = 0; lines[i]; i++)
 	{
-		const char *where;
-
 		/*
 		 * Look for a line assigning to guc_name.  Typically it will be
 		 * preceded by '#', but that might not be the case if a -c switch
@@ -493,15 +487,32 @@ replace_guc_value(char **lines, const char *guc_name, const char *guc_value,
 		where = lines[i];
 		while (*where == '#' || isspace((unsigned char) *where))
 			where++;
-		if (strncmp(where, guc_name, namelen) != 0)
+		if (strncasecmp(where, guc_name, namelen) != 0)
 			continue;
+
+		pname = where;
 		where += namelen;
 		while (isspace((unsigned char) *where))
 			where++;
-		if (*where != '=')
-			continue;
 
-		/* found it -- append the original comment if any */
+		/* assume there's only one match */
+		if (*where == '=')
+			break;
+	}
+
+	if (lines[i])
+	{
+		/* found it, rewrite the line preserving the original comment if any */
+		appendPQExpBuffer(newline, "%.*s = ", namelen, pname);
+		if (guc_value_requires_quotes(guc_value))
+			appendPQExpBuffer(newline, "'%s'", escape_quotes(guc_value));
+		else
+			appendPQExpBufferStr(newline, guc_value);
+
+		/*
+		 * completed body of line, now continue with potential indentation and
+		 * comment
+		 */
 		where = strrchr(where, '#');
 		if (where)
 		{
@@ -548,16 +559,19 @@ replace_guc_value(char **lines, const char *guc_name, const char *guc_value,
 
 		free(lines[i]);
 		lines[i] = newline->data;
-
-		break;					/* assume there's only one match */
 	}
-
-	if (lines[i] == NULL)
+	else
 	{
 		/*
 		 * No match, so append a new entry.  (We rely on the bootstrap server
 		 * to complain if it's not a valid GUC name.)
 		 */
+		appendPQExpBuffer(newline, "%s = ", guc_name);
+		if (guc_value_requires_quotes(guc_value))
+			appendPQExpBuffer(newline, "'%s'", escape_quotes(guc_value));
+		else
+			appendPQExpBufferStr(newline, guc_value);
+
 		appendPQExpBufferChar(newline, '\n');
 		lines = pg_realloc_array(lines, char *, i + 2);
 		lines[i++] = newline->data;

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 16 Jan 2024 12:17:23 +0100

> On 28 Sep 2023, at 09:49, Kyotaro Horiguchi <[email-masked]> wrote:

> line. Furthermore, I surmise that preserving the original letter

Circling back to an old thread, I agree that this seems odd and the original
thread [0] makes no mention of it being intentional.

The patch seems fine to me, the attached version is rebased, pgindented and has
a test case added.

--
Daniel Gustafsson

[0] https://www.postgresql.org/message-id/flat/2844176.1674681919%40sss.pgh.pa.us

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 17 Jan 2024 17:15:53 +0100

> thread [0] makes no mention of it being intentional.

Hmm, how about raising an error if multiple options are given targetting
the same GUC?

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

---

From: Tom Lane <[email-masked]>
Date: Wed, 17 Jan 2024 12:05:27 -0500

> Hmm, how about raising an error if multiple options are given targetting

I don't see any reason to do that.  The underlying configuration
files don't complain about duplicate entries, they just take the
last setting.

			regards, tom lane

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 17 Jan 2024 20:30:48 +0100

> On 17 Jan 2024, at 18:05, Tom Lane <[email-masked]> wrote:
> 

Agreed, I think the patch as it stands now where it replaces case insensitive,
while keeping the original casing, is the best path forward.  The issue exist
in 16 as well so I propose a backpatch to there.

--
Daniel Gustafsson

---

From: Robert Haas <[email-masked]>
Date: Wed, 17 Jan 2024 14:57:35 -0500

> in 16 as well so I propose a backpatch to there.

I like that approach, too. I could go either way on back-patching. It
doesn't seem important, but it probably won't break anything, either.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Tom Lane <[email-masked]>
Date: Wed, 17 Jan 2024 15:26:10 -0500

> doesn't seem important, but it probably won't break anything, either.

We just added this switch in 16, so I think backpatching to keep all
the branches consistent is a good idea.

However ... I don't like the patch much.  It seems to have left
the code in a rather random state.  Why, for example, didn't you
keep all the code that constructs the "newline" value together?
I'm also unconvinced by the removal of the "assume there's only
one match" break --- if we need to support multiple matches
I doubt that's sufficient.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Wed, 17 Jan 2024 15:33:02 -0500

> keep all the code that constructs the "newline" value together?

After thinking about it a bit more, I don't see why you didn't just
s/strncmp/strncasecmp/ and call it good.  The messiness seems to be
a result of your choice to replace the GUC's case as shown in the
file with the case used on the command line, which is not better
IMO.  We don't change our mind about the canonical spelling of a
GUC because somebody varied the case in a SET command.

			regards, tom lane

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 17 Jan 2024 23:47:41 +0100

> On 17 Jan 2024, at 21:33, Tom Lane <[email-masked]> wrote:
> 
> I wrote:

The original patch was preserving the case of the file throwing away the case
from the commandline.  The attached is a slimmed down version which only moves
the assembly of newline to the different cases (replace vs.  new) keeping the
rest of the code intact.  Keeping it in one place gets sort of messy too since
it needs to use different values for a replacement and a new variable.  Not
sure if there is a cleaner approach?

--
Daniel Gustafsson

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 18 Jan 2024 13:49:44 +0900 (JST)

Thank you for upicking this up.

At Wed, 17 Jan 2024 23:47:41 +0100, Daniel Gustafsson <[email-masked]> wrote in 
> sure if there is a cleaner approach?

Just to clarify, the current code breaks out after processing the
first matching line. I haven't changed that behavior.  The reason I
moved the rewrite processing code out of the loop was I wanted to
avoid adding more lines that are executed only once into the
loop. However, it is in exchange of additional complexity to remember
the original spelling of the parameter name. Personally, I believe
separating the search and rewrite processing is better, but I'm not
particularly attached to the approach, so either way is fine with me.

regards.

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 19 Jan 2024 15:53:59 +0100

> On 18 Jan 2024, at 05:49, Kyotaro Horiguchi <[email-masked]> wrote:
> 
> first matching line. I haven't changed that behavior.

Yup.

> The reason I

I'll give some more time for opinions, then I'll go ahead with one of the
patches with a backpatch to v16.

--
Daniel Gustafsson

---

From: Tom Lane <[email-masked]>
Date: Fri, 19 Jan 2024 11:33:57 -0500

> patches with a backpatch to v16.

OK, I take back my previous complaint that just using strncasecmp
would be enough --- I was misremembering how the code worked, and
you're right that it would use the spelling from the command line
rather than that from the file.

However, the v3 patch is flat broken.  You can't assume you have
found a match until you've verified that whitespace and '='
appear next --- otherwise, you'll be fooled by a guc_name that
is a prefix of one that appears in the file.  I think the simplest
change that does it correctly is as attached.

Now, since I was the one who wrote the existing code, I freely
concede that I may have too high an opinion of its readability.
Maybe some larger refactoring is appropriate.  But I didn't
find that what you'd done improved the readability.  I'd still
rather keep the newline-assembly code together as much as possible.
Maybe we should do the search part before we build any of newline?

			regards, tom lane

diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index ac409b0006..6a620c9d5f 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -484,6 +484,7 @@ replace_guc_value(char **lines, const char *guc_name, const char *guc_value,
 	for (i = 0; lines[i]; i++)
 	{
 		const char *where;
+		const char *namestart;
 
 		/*
@@ -494,15 +495,19 @@ replace_guc_value(char **lines, const char *guc_name, const char *guc_value,
+		namestart = where;
 		if (*where != '=')
 			continue;
 
-		/* found it -- append the original comment if any */
+		/* found it -- let's use the canonical casing shown in the file */
+		memcpy(&newline->data[mark_as_comment ? 1 : 0], namestart, namelen);
+
+		/* now append the original comment if any */
 		where = strrchr(where, '#');
 		if (where)
 		{
diff --git a/src/bin/initdb/t/001_initdb.pl b/src/bin/initdb/t/001_initdb.pl
index 03376cc0f7..413a5eca67 100644
--- a/src/bin/initdb/t/001_initdb.pl
+++ b/src/bin/initdb/t/001_initdb.pl
@@ -199,4 +199,17 @@ command_fails(
 command_fails([ 'initdb', '--no-sync', '--set', 'foo=bar', "$tempdir/dataX" ],
 	'fails for invalid --set option');
 
+# Make sure multiple invocations of -c parameters are added case insensitive
+command_ok(
+	[
+		'initdb', '-cwork_mem=128', '-cWork_Mem=256', '-cWORK_MEM=512',
+		"$tempdir/dataY"
+	],
+	'multiple -c options with different case');
+
+my $conf = slurp_file("$tempdir/dataY/postgresql.conf");
+ok($conf !~ qr/^WORK_MEM = /m, "WORK_MEM should not be configured");
+ok($conf !~ qr/^Work_Mem = /m, "Work_Mem should not be configured");
+ok($conf =~ qr/^work_mem = 512/m, "work_mem should be in config");
+
 done_testing();

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 22 Jan 2024 11:09:14 +0100

> On 19 Jan 2024, at 17:33, Tom Lane <[email-masked]> wrote:
> 

The attached v4 looks good to me, I don't think it moves the needle wrt
readability (ie, no need to move the search).  Feel free to go ahead with that
version, or let me know if you want me to deal with it.

--
Daniel Gustafsson