public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 2/3] Allow composite types in bootstrap 14+ messages / 6 participants [nested] [flat]
* [PATCH 2/3] Allow composite types in bootstrap @ 2020-11-17 15:28 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 14+ messages in thread From: Justin Pryzby @ 2020-11-17 15:28 UTC (permalink / raw) --- src/backend/bootstrap/bootstrap.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 18eb62ca47..e4fc75ab84 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -916,6 +916,7 @@ gettype(char *type) { if (Typ != NIL) { + static bool did_reread PG_USED_FOR_ASSERTS_ONLY = false; /* Already reread pg_types */ ListCell *lc; foreach (lc, Typ) @@ -927,6 +928,33 @@ gettype(char *type) return app->am_oid; } } + + /* + * The type wasn't known; check again to handle composite + * types, added since first populating the array. + */ + + /* + * Once all the types are populated and we handled composite + * types, shouldn't need to do that again. + */ + Assert(!did_reread); + did_reread = true; + + list_free_deep(Typ); + Typ = NULL; + populate_typ_array(); + + /* Need to avoid infinite recursion... */ + foreach (lc, Typ) + { + struct typmap *app = lfirst(lc); + if (strncmp(NameStr(app->am_typ.typname), type, NAMEDATALEN) == 0) + { + Ap = app; + return app->am_oid; + } + } } else { -- 2.26.2 --------------76B8D0DC8AE327D516738282 Content-Type: text/x-patch; charset=UTF-8; name="0003-Extended-statistics-on-expressions-20210108.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0003-Extended-statistics-on-expressions-20210108.patch" ^ permalink raw reply [nested|flat] 14+ messages in thread
* initdb's -c option behaves wrong way? @ 2023-09-28 07:49 Kyotaro Horiguchi <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Kyotaro Horiguchi @ 2023-09-28 07:49 UTC (permalink / raw) To: [email protected] 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 Attachments: [text/x-patch] initdb-c_ignore_letter_case.patch (2.6K, ../../[email protected]/2-initdb-c_ignore_letter_case.patch) download | inline diff: 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; ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: initdb's -c option behaves wrong way? @ 2024-01-16 11:17 Daniel Gustafsson <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Daniel Gustafsson @ 2024-01-16 11:17 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected] > On 28 Sep 2023, at 09:49, Kyotaro Horiguchi <[email protected]> wrote: > 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 > The original intention was apparently to overwrite the existing > line. Furthermore, I surmise that preserving the original letter > casing is preferable. 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 Attachments: [application/octet-stream] v2-0001-Make-initdb-c-option-case-insensitive.patch (4.3K, ../../[email protected]/2-v2-0001-Make-initdb-c-option-case-insensitive.patch) download | inline diff: From cbdfe0eefbd2707401f542e3e5a4e08c7202a77c Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson <[email protected]> Date: Tue, 16 Jan 2024 12:02:21 +0100 Subject: [PATCH v2] 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 update 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 | 46 ++++++++++++++++++++++------------ src/bin/initdb/t/001_initdb.pl | 12 +++++++++ 2 files changed, 42 insertions(+), 16 deletions(-) diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index ac409b0006..aa858ece8a 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -471,20 +471,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 @@ -494,15 +488,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); + + /* + * Body of line has been completed, now continue with potential + * indentation and comment. + */ where = strrchr(where, '#'); if (where) { @@ -549,16 +560,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; diff --git a/src/bin/initdb/t/001_initdb.pl b/src/bin/initdb/t/001_initdb.pl index 03376cc0f7..1c048bba8e 100644 --- a/src/bin/initdb/t/001_initdb.pl +++ b/src/bin/initdb/t/001_initdb.pl @@ -199,4 +199,16 @@ 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/dataX" + ], + 'multiple -c options with different case'); + +my $conf = slurp_file("$tempdir/dataX/postgresql.conf"); +ok($conf =~ qr/^work_mem = 512/m, "work_mem configured"); +ok($conf !~ qr/^WORK_MEM = 512/m, "WORK_MEM should not be in config"); + done_testing(); -- 2.32.1 (Apple Git-133) ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: initdb's -c option behaves wrong way? @ 2024-01-17 16:15 Alvaro Herrera <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Alvaro Herrera @ 2024-01-17 16:15 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected] On 2024-Jan-16, Daniel Gustafsson wrote: > > On 28 Sep 2023, at 09:49, Kyotaro Horiguchi <[email protected]> wrote: > > > 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 > > > The original intention was apparently to overwrite the existing > > line. Furthermore, I surmise that preserving the original letter > > casing is preferable. > > Circling back to an old thread, I agree that this seems odd and the original > 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/ ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: initdb's -c option behaves wrong way? @ 2024-01-17 17:05 Tom Lane <[email protected]> parent: Alvaro Herrera <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Tom Lane @ 2024-01-17 17:05 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Alvaro Herrera <[email protected]> writes: > Hmm, how about raising an error if multiple options are given targetting > the same GUC? 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 ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: initdb's -c option behaves wrong way? @ 2024-01-17 19:30 Daniel Gustafsson <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Daniel Gustafsson @ 2024-01-17 19:30 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] > On 17 Jan 2024, at 18:05, Tom Lane <[email protected]> wrote: > > Alvaro Herrera <[email protected]> writes: >> Hmm, how about raising an error if multiple options are given targetting >> the same GUC? > > 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. 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 ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: initdb's -c option behaves wrong way? @ 2024-01-17 19:57 Robert Haas <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Robert Haas @ 2024-01-17 19:57 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Tom Lane <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On Wed, Jan 17, 2024 at 2:31 PM Daniel Gustafsson <[email protected]> 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. 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 ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: initdb's -c option behaves wrong way? @ 2024-01-17 20:26 Tom Lane <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Tom Lane @ 2024-01-17 20:26 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Robert Haas <[email protected]> writes: > On Wed, Jan 17, 2024 at 2:31 PM Daniel Gustafsson <[email protected]> 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. > 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. 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 ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: initdb's -c option behaves wrong way? @ 2024-01-17 20:33 Tom Lane <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Tom Lane @ 2024-01-17 20:33 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] 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. regards, tom lane ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: initdb's -c option behaves wrong way? @ 2024-01-17 22:47 Daniel Gustafsson <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Daniel Gustafsson @ 2024-01-17 22:47 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[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) ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: initdb's -c option behaves wrong way? @ 2024-01-18 04:49 Kyotaro Horiguchi <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Kyotaro Horiguchi @ 2024-01-18 04:49 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected] Thank you for upicking this up. At Wed, 17 Jan 2024 23:47:41 +0100, Daniel Gustafsson <[email protected]> wrote in > > 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? 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. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: initdb's -c option behaves wrong way? @ 2024-01-19 14:53 Daniel Gustafsson <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Daniel Gustafsson @ 2024-01-19 14:53 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; [email protected] > On 18 Jan 2024, at 05:49, Kyotaro Horiguchi <[email protected]> wrote: > > Thank you for upicking this up. > > At Wed, 17 Jan 2024 23:47:41 +0100, Daniel Gustafsson <[email protected]> wrote in >>> 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? > > Just to clarify, the current code breaks out after processing the > first matching line. I haven't changed that behavior. Yup. > 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. 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 ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: initdb's -c option behaves wrong way? @ 2024-01-19 16:33 Tom Lane <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Tom Lane @ 2024-01-19 16:33 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; [email protected] Daniel Gustafsson <[email protected]> writes: > I'll give some more time for opinions, then I'll go ahead with one of the > 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 Attachments: [text/x-diff] v4-0001-Make-initdb-c-option-case-insensitive.patch (2.0K, ../../[email protected]/2-v4-0001-Make-initdb-c-option-case-insensitive.patch) download | inline diff: 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; /* * Look for a line assigning to guc_name. Typically it will be @@ -494,15 +495,19 @@ 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; + namestart = where; where += namelen; while (isspace((unsigned char) *where)) 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(); ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: initdb's -c option behaves wrong way? @ 2024-01-22 10:09 Daniel Gustafsson <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 14+ messages in thread From: Daniel Gustafsson @ 2024-01-22 10:09 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; [email protected] > On 19 Jan 2024, at 17:33, Tom Lane <[email protected]> wrote: > > Daniel Gustafsson <[email protected]> writes: >> I'll give some more time for opinions, then I'll go ahead with one of the >> 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. 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 ^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2024-01-22 10:09 UTC | newest] Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-17 15:28 [PATCH 2/3] Allow composite types in bootstrap Justin Pryzby <[email protected]> 2023-09-28 07:49 initdb's -c option behaves wrong way? Kyotaro Horiguchi <[email protected]> 2024-01-16 11:17 ` Re: initdb's -c option behaves wrong way? Daniel Gustafsson <[email protected]> 2024-01-17 16:15 ` Re: initdb's -c option behaves wrong way? Alvaro Herrera <[email protected]> 2024-01-17 17:05 ` Re: initdb's -c option behaves wrong way? Tom Lane <[email protected]> 2024-01-17 19:30 ` Re: initdb's -c option behaves wrong way? Daniel Gustafsson <[email protected]> 2024-01-17 19:57 ` Re: initdb's -c option behaves wrong way? Robert Haas <[email protected]> 2024-01-17 20:26 ` Re: initdb's -c option behaves wrong way? Tom Lane <[email protected]> 2024-01-17 20:33 ` Re: initdb's -c option behaves wrong way? Tom Lane <[email protected]> 2024-01-17 22:47 ` Re: initdb's -c option behaves wrong way? Daniel Gustafsson <[email protected]> 2024-01-18 04:49 ` Re: initdb's -c option behaves wrong way? Kyotaro Horiguchi <[email protected]> 2024-01-19 14:53 ` Re: initdb's -c option behaves wrong way? Daniel Gustafsson <[email protected]> 2024-01-19 16:33 ` Re: initdb's -c option behaves wrong way? Tom Lane <[email protected]> 2024-01-22 10:09 ` Re: initdb's -c option behaves wrong way? Daniel Gustafsson <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox