public inbox for [email protected]
help / color / mirror / Atom feedFrom: Daniel Gustafsson <[email protected]>
To: Tom Lane <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Cc: [email protected]
Subject: Re: initdb's -c option behaves wrong way?
Date: Wed, 17 Jan 2024 23:47:41 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<CA+TgmoZYa5hNgdadYwNfpReykW7RU5egpzrFaAMwFA9qz0g=eg@mail.gmail.com>
<[email protected]>
<[email protected]>
> On 17 Jan 2024, at 21:33, Tom Lane <[email protected]> wrote:
>
> I wrote:
>> 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?
>
> 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.
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
Attachments:
[application/octet-stream] v3-0001-Make-initdb-c-option-case-insensitive.patch (3.5K, ../[email protected]/2-v3-0001-Make-initdb-c-option-case-insensitive.patch)
download | inline diff:
From c71e123c19c7b486182f02375a623d0d8698d680 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <[email protected]>
Date: Wed, 17 Jan 2024 22:47:03 +0100
Subject: [PATCH v3] Make initdb -c option case insensitive
When a -c option specifies a GUC name to replace using different
case than the existing parameter it was added as a new parameter
rather than replacing the existing. This makes -c replacements
case insensitive such that -cWORK_MEM will replace the value of
work_mem in the config.
Reported-by: Kyotaro Horiguchi <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/bin/initdb/initdb.c | 19 ++++++++++++-------
src/bin/initdb/t/001_initdb.pl | 13 +++++++++++++
2 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index ac409b0006..edfab4ed22 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -475,11 +475,6 @@ replace_guc_value(char **lines, const char *guc_name, const char *guc_value,
/* 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++)
{
@@ -494,8 +489,15 @@ 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;
+ /* make sure to preserve case of the replaced GUC */
+ appendPQExpBuffer(newline, "%.*s = ", namelen, where);
+ if (guc_value_requires_quotes(guc_value))
+ appendPQExpBuffer(newline, "'%s'", escape_quotes(guc_value));
+ else
+ appendPQExpBufferStr(newline, guc_value);
+
where += namelen;
while (isspace((unsigned char) *where))
where++;
@@ -559,7 +561,10 @@ replace_guc_value(char **lines, const char *guc_name, const char *guc_value,
* No match, so append a new entry. (We rely on the bootstrap server
* to complain if it's not a valid GUC name.)
*/
- appendPQExpBufferChar(newline, '\n');
+ if (guc_value_requires_quotes(guc_value))
+ appendPQExpBuffer(newline, "%s = '%s'\n", guc_name, escape_quotes(guc_value));
+ else
+ appendPQExpBuffer(newline, "%s = %s\n", guc_name, guc_value);
lines = pg_realloc_array(lines, char *, i + 2);
lines[i++] = newline->data;
lines[i] = NULL; /* keep the array null-terminated */
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();
--
2.32.1 (Apple Git-133)
view thread (14+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: initdb's -c option behaves wrong way?
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox