public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 7/8] Ignore correlation for new BRIN opclasses 3+ messages / 3 participants [nested] [flat]
* [PATCH 7/8] Ignore correlation for new BRIN opclasses @ 2020-09-12 13:07 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Tomas Vondra @ 2020-09-12 13:07 UTC (permalink / raw) The new BRIN opclasses (bloom and minmax-multi) are less sensitive to poorly correlated data, so just assume the data is perfectly correlated during costing. Author: Tomas Vondra <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/backend/access/brin/brin_bloom.c | 1 + src/backend/access/brin/brin_minmax_multi.c | 1 + src/backend/utils/adt/selfuncs.c | 19 ++++++++++++++++++- src/include/access/brin_internal.h | 3 +++ 4 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/backend/access/brin/brin_bloom.c b/src/backend/access/brin/brin_bloom.c index c086e83236..22590b2351 100644 --- a/src/backend/access/brin/brin_bloom.c +++ b/src/backend/access/brin/brin_bloom.c @@ -412,6 +412,7 @@ brin_bloom_opcinfo(PG_FUNCTION_ARGS) result = palloc0(MAXALIGN(SizeofBrinOpcInfo(1)) + sizeof(BloomOpaque)); + result->oi_ignore_correlation = true; result->oi_nstored = 1; result->oi_regular_nulls = true; result->oi_opaque = (BloomOpaque *) diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c index d19ebf08c2..6957be0bdb 100644 --- a/src/backend/access/brin/brin_minmax_multi.c +++ b/src/backend/access/brin/brin_minmax_multi.c @@ -1737,6 +1737,7 @@ brin_minmax_multi_opcinfo(PG_FUNCTION_ARGS) result = palloc0(MAXALIGN(SizeofBrinOpcInfo(1)) + sizeof(MinmaxMultiOpaque)); + result->oi_ignore_correlation = true; result->oi_nstored = 1; result->oi_regular_nulls = true; result->oi_opaque = (MinmaxMultiOpaque *) diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c index 52314d3aa1..0320d128f6 100644 --- a/src/backend/utils/adt/selfuncs.c +++ b/src/backend/utils/adt/selfuncs.c @@ -98,6 +98,7 @@ #include <math.h> #include "access/brin.h" +#include "access/brin_internal.h" #include "access/brin_page.h" #include "access/gin.h" #include "access/table.h" @@ -7352,7 +7353,8 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count, double minimalRanges; double estimatedRanges; double selec; - Relation indexRel; + Relation indexRel = NULL; + TupleDesc tupdesc = NULL; ListCell *l; VariableStatData vardata; @@ -7374,6 +7376,7 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count, */ indexRel = index_open(index->indexoid, NoLock); brinGetStats(indexRel, &statsData); + tupdesc = RelationGetDescr(indexRel); index_close(indexRel, NoLock); /* work out the actual number of ranges in the index */ @@ -7407,6 +7410,17 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count, { IndexClause *iclause = lfirst_node(IndexClause, l); AttrNumber attnum = index->indexkeys[iclause->indexcol]; + FmgrInfo *opcInfoFn; + BrinOpcInfo *opcInfo; + Form_pg_attribute attr = TupleDescAttr(tupdesc, iclause->indexcol); + bool ignore_correlation; + + opcInfoFn = index_getprocinfo(indexRel, iclause->indexcol + 1, BRIN_PROCNUM_OPCINFO); + + opcInfo = (BrinOpcInfo *) + DatumGetPointer(FunctionCall1(opcInfoFn, attr->atttypid)); + + ignore_correlation = opcInfo->oi_ignore_correlation; /* attempt to lookup stats in relation for this index column */ if (attnum != 0) @@ -7477,6 +7491,9 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count, if (sslot.nnumbers > 0) varCorrelation = Abs(sslot.numbers[0]); + if (ignore_correlation) + varCorrelation = 1.0; + if (varCorrelation > *indexCorrelation) *indexCorrelation = varCorrelation; diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h index 89254b5766..063b703208 100644 --- a/src/include/access/brin_internal.h +++ b/src/include/access/brin_internal.h @@ -30,6 +30,9 @@ typedef struct BrinOpcInfo /* Regular processing of NULLs in BrinValues? */ bool oi_regular_nulls; + /* Ignore correlation during cost estimation */ + bool oi_ignore_correlation; + /* Opaque pointer for the opclass' private use */ void *oi_opaque; -- 2.26.2 --------------D524CD37E77B36A2883A7617 Content-Type: text/x-patch; charset=UTF-8; name="0008-Consider-selectivity-in-choose_bitmap_and-20210311.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0008-Consider-selectivity-in-choose_bitmap_and-20210311.patc"; filename*1="h" ^ permalink raw reply [nested|flat] 3+ messages in thread
* Clean up command argument assembly @ 2023-06-26 09:33 Peter Eisentraut <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Peter Eisentraut @ 2023-06-26 09:33 UTC (permalink / raw) To: pgsql-hackers This is a small code cleanup patch. Several commands internally assemble command lines to call other commands. This includes initdb, pg_dumpall, and pg_regress. (Also pg_ctl, but that is different enough that I didn't consider it here.) This has all evolved a bit organically, with fixed-size buffers, and various optional command-line arguments being injected with confusing-looking code, and the spacing between options handled in inconsistent ways. This patch cleans all this up a bit to look clearer and be more easily extensible with new arguments and options. We start each command with printfPQExpBuffer(), and then append arguments as necessary with appendPQExpBuffer(). Also standardize on using initPQExpBuffer() over createPQExpBuffer() where possible. pg_regress uses StringInfo instead of PQExpBuffer, but many of the same ideas apply. From dbd9d7b7ff2c069a131c9efa8bff2597c0d9e1c8 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Mon, 26 Jun 2023 11:30:24 +0200 Subject: [PATCH] Clean up command argument assembly Several commands internally assemble command lines to call other commands. This includes initdb, pg_dumpall, and pg_regress. (Also pg_ctl, but that is different enough that I didn't consider it here.) This has all evolved a bit organically, with fixed-size buffers, and various optional command-line arguments being injected with confusing-looking code, and the spacing between options handled in inconsistent ways. Clean all this up a bit to look clearer and be more easily extensible with new arguments and options. We start each command with printfPQExpBuffer(), and then append arguments as necessary with appendPQExpBuffer(). Also standardize on using initPQExpBuffer() over createPQExpBuffer() where possible. pg_regress uses StringInfo instead of PQExpBuffer, but many of the same ideas apply. --- src/bin/initdb/initdb.c | 56 +++++++++++++++++++---------------- src/bin/pg_dump/pg_dumpall.c | 27 +++++++++-------- src/test/regress/pg_regress.c | 26 +++++++++------- 3 files changed, 62 insertions(+), 47 deletions(-) diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index fc1fb363e7..3f4167682a 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -309,16 +309,16 @@ void initialize_data_directory(void); /* * macros for running pipes to postgres */ -#define PG_CMD_DECL char cmd[MAXPGPATH]; FILE *cmdfd +#define PG_CMD_DECL FILE *cmdfd -#define PG_CMD_OPEN \ +#define PG_CMD_OPEN(cmd) \ do { \ cmdfd = popen_check(cmd, "w"); \ if (cmdfd == NULL) \ exit(1); /* message already printed by popen_check */ \ } while (0) -#define PG_CMD_CLOSE \ +#define PG_CMD_CLOSE() \ do { \ if (pclose_check(cmdfd)) \ exit(1); /* message already printed by pclose_check */ \ @@ -1138,13 +1138,15 @@ test_config_settings(void) static bool test_specific_config_settings(int test_conns, int test_buffs) { - PQExpBuffer cmd = createPQExpBuffer(); + PQExpBufferData cmd; _stringlist *gnames, *gvalues; int status; + initPQExpBuffer(&cmd); + /* Set up the test postmaster invocation */ - printfPQExpBuffer(cmd, + printfPQExpBuffer(&cmd, "\"%s\" --check %s %s " "-c max_connections=%d " "-c shared_buffers=%d " @@ -1158,18 +1160,18 @@ test_specific_config_settings(int test_conns, int test_buffs) gnames != NULL; /* assume lists have the same length */ gnames = gnames->next, gvalues = gvalues->next) { - appendPQExpBuffer(cmd, " -c %s=", gnames->str); - appendShellString(cmd, gvalues->str); + appendPQExpBuffer(&cmd, " -c %s=", gnames->str); + appendShellString(&cmd, gvalues->str); } - appendPQExpBuffer(cmd, + appendPQExpBuffer(&cmd, " < \"%s\" > \"%s\" 2>&1", DEVNULL, DEVNULL); fflush(NULL); - status = system(cmd->data); + status = system(cmd.data); - destroyPQExpBuffer(cmd); + termPQExpBuffer(&cmd); return (status == 0); } @@ -1469,6 +1471,7 @@ static void bootstrap_template1(void) { PG_CMD_DECL; + PQExpBufferData cmd; char **line; char **bki_lines; char headerline[MAXPGPATH]; @@ -1530,16 +1533,17 @@ bootstrap_template1(void) /* Also ensure backend isn't confused by this environment var: */ unsetenv("PGCLIENTENCODING"); - snprintf(cmd, sizeof(cmd), - "\"%s\" --boot -X %d %s %s %s %s", - backend_exec, - wal_segment_size_mb * (1024 * 1024), - data_checksums ? "-k" : "", - boot_options, extra_options, - debug ? "-d 5" : ""); + initPQExpBuffer(&cmd); + + printfPQExpBuffer(&cmd, "\"%s\" --boot %s %s", backend_exec, boot_options, extra_options); + appendPQExpBuffer(&cmd, " -X %d", wal_segment_size_mb * (1024 * 1024)); + if (data_checksums) + appendPQExpBuffer(&cmd, " -k"); + if (debug) + appendPQExpBuffer(&cmd, " -d 5"); - PG_CMD_OPEN; + PG_CMD_OPEN(cmd.data); for (line = bki_lines; *line != NULL; line++) { @@ -1547,8 +1551,9 @@ bootstrap_template1(void) free(*line); } - PG_CMD_CLOSE; + PG_CMD_CLOSE(); + termPQExpBuffer(&cmd); free(bki_lines); check_ok(); @@ -2951,6 +2956,7 @@ void initialize_data_directory(void) { PG_CMD_DECL; + PQExpBufferData cmd; int i; setup_signals(); @@ -3014,12 +3020,11 @@ initialize_data_directory(void) fputs(_("performing post-bootstrap initialization ... "), stdout); fflush(stdout); - snprintf(cmd, sizeof(cmd), - "\"%s\" %s %s template1 >%s", - backend_exec, backend_options, extra_options, - DEVNULL); + initPQExpBuffer(&cmd); + printfPQExpBuffer(&cmd, "\"%s\" %s %s template1 >%s", + backend_exec, backend_options, extra_options, DEVNULL); - PG_CMD_OPEN; + PG_CMD_OPEN(cmd.data); setup_auth(cmdfd); @@ -3054,7 +3059,8 @@ initialize_data_directory(void) make_postgres(cmdfd); - PG_CMD_CLOSE; + PG_CMD_CLOSE(); + termPQExpBuffer(&cmd); check_ok(); } diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c index 3627b69e2a..36f134137f 100644 --- a/src/bin/pg_dump/pg_dumpall.c +++ b/src/bin/pg_dump/pg_dumpall.c @@ -1564,11 +1564,14 @@ dumpDatabases(PGconn *conn) static int runPgDump(const char *dbname, const char *create_opts) { - PQExpBuffer connstrbuf = createPQExpBuffer(); - PQExpBuffer cmd = createPQExpBuffer(); + PQExpBufferData connstrbuf; + PQExpBufferData cmd; int ret; - appendPQExpBuffer(cmd, "\"%s\" %s %s", pg_dump_bin, + initPQExpBuffer(&connstrbuf); + initPQExpBuffer(&cmd); + + printfPQExpBuffer(&cmd, "\"%s\" %s %s", pg_dump_bin, pgdumpopts->data, create_opts); /* @@ -1576,27 +1579,27 @@ runPgDump(const char *dbname, const char *create_opts) * format. */ if (filename) - appendPQExpBufferStr(cmd, " -Fa "); + appendPQExpBufferStr(&cmd, " -Fa "); else - appendPQExpBufferStr(cmd, " -Fp "); + appendPQExpBufferStr(&cmd, " -Fp "); /* * Append the database name to the already-constructed stem of connection * string. */ - appendPQExpBuffer(connstrbuf, "%s dbname=", connstr); - appendConnStrVal(connstrbuf, dbname); + appendPQExpBuffer(&connstrbuf, "%s dbname=", connstr); + appendConnStrVal(&connstrbuf, dbname); - appendShellString(cmd, connstrbuf->data); + appendShellString(&cmd, connstrbuf.data); - pg_log_info("running \"%s\"", cmd->data); + pg_log_info("running \"%s\"", cmd.data); fflush(NULL); - ret = system(cmd->data); + ret = system(cmd.data); - destroyPQExpBuffer(cmd); - destroyPQExpBuffer(connstrbuf); + termPQExpBuffer(&cmd); + termPQExpBuffer(&connstrbuf); return ret; } diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c index 60d34a40b2..3e4fd918dd 100644 --- a/src/test/regress/pg_regress.c +++ b/src/test/regress/pg_regress.c @@ -2293,6 +2293,7 @@ regression_main(int argc, char *argv[], if (temp_instance) { + StringInfoData cmd; FILE *pg_conf; const char *env_wait; int wait_seconds; @@ -2318,23 +2319,28 @@ regression_main(int argc, char *argv[], make_directory(buf); /* initdb */ - snprintf(buf, sizeof(buf), - "\"%s%sinitdb\" -D \"%s/data\" --no-clean --no-sync%s%s > \"%s/log/initdb.log\" 2>&1", - bindir ? bindir : "", - bindir ? "/" : "", - temp_instance, - debug ? " --debug" : "", - nolocale ? " --no-locale" : "", - outputdir); + initStringInfo(&cmd); + appendStringInfo(&cmd, + "\"%s%sinitdb\" -D \"%s/data\" --no-clean --no-sync", + bindir ? bindir : "", + bindir ? "/" : "", + temp_instance); + if (debug) + appendStringInfo(&cmd, " --debug"); + if (nolocale) + appendStringInfo(&cmd, " --no-locale"); + appendStringInfo(&cmd, " > \"%s/log/initdb.log\" 2>&1", outputdir); fflush(NULL); - if (system(buf)) + if (system(cmd.data)) { bail("initdb failed\n" "# Examine \"%s/log/initdb.log\" for the reason.\n" "# Command was: %s", - outputdir, buf); + outputdir, cmd.data); } + pfree(cmd.data); + /* * Adjust the default postgresql.conf for regression testing. The user * can specify a file to be appended; in any case we expand logging -- 2.41.0 Attachments: [text/plain] 0001-Clean-up-command-argument-assembly.patch (8.0K, ../../[email protected]/2-0001-Clean-up-command-argument-assembly.patch) download | inline diff: From dbd9d7b7ff2c069a131c9efa8bff2597c0d9e1c8 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Mon, 26 Jun 2023 11:30:24 +0200 Subject: [PATCH] Clean up command argument assembly Several commands internally assemble command lines to call other commands. This includes initdb, pg_dumpall, and pg_regress. (Also pg_ctl, but that is different enough that I didn't consider it here.) This has all evolved a bit organically, with fixed-size buffers, and various optional command-line arguments being injected with confusing-looking code, and the spacing between options handled in inconsistent ways. Clean all this up a bit to look clearer and be more easily extensible with new arguments and options. We start each command with printfPQExpBuffer(), and then append arguments as necessary with appendPQExpBuffer(). Also standardize on using initPQExpBuffer() over createPQExpBuffer() where possible. pg_regress uses StringInfo instead of PQExpBuffer, but many of the same ideas apply. --- src/bin/initdb/initdb.c | 56 +++++++++++++++++++---------------- src/bin/pg_dump/pg_dumpall.c | 27 +++++++++-------- src/test/regress/pg_regress.c | 26 +++++++++------- 3 files changed, 62 insertions(+), 47 deletions(-) diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index fc1fb363e7..3f4167682a 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -309,16 +309,16 @@ void initialize_data_directory(void); /* * macros for running pipes to postgres */ -#define PG_CMD_DECL char cmd[MAXPGPATH]; FILE *cmdfd +#define PG_CMD_DECL FILE *cmdfd -#define PG_CMD_OPEN \ +#define PG_CMD_OPEN(cmd) \ do { \ cmdfd = popen_check(cmd, "w"); \ if (cmdfd == NULL) \ exit(1); /* message already printed by popen_check */ \ } while (0) -#define PG_CMD_CLOSE \ +#define PG_CMD_CLOSE() \ do { \ if (pclose_check(cmdfd)) \ exit(1); /* message already printed by pclose_check */ \ @@ -1138,13 +1138,15 @@ test_config_settings(void) static bool test_specific_config_settings(int test_conns, int test_buffs) { - PQExpBuffer cmd = createPQExpBuffer(); + PQExpBufferData cmd; _stringlist *gnames, *gvalues; int status; + initPQExpBuffer(&cmd); + /* Set up the test postmaster invocation */ - printfPQExpBuffer(cmd, + printfPQExpBuffer(&cmd, "\"%s\" --check %s %s " "-c max_connections=%d " "-c shared_buffers=%d " @@ -1158,18 +1160,18 @@ test_specific_config_settings(int test_conns, int test_buffs) gnames != NULL; /* assume lists have the same length */ gnames = gnames->next, gvalues = gvalues->next) { - appendPQExpBuffer(cmd, " -c %s=", gnames->str); - appendShellString(cmd, gvalues->str); + appendPQExpBuffer(&cmd, " -c %s=", gnames->str); + appendShellString(&cmd, gvalues->str); } - appendPQExpBuffer(cmd, + appendPQExpBuffer(&cmd, " < \"%s\" > \"%s\" 2>&1", DEVNULL, DEVNULL); fflush(NULL); - status = system(cmd->data); + status = system(cmd.data); - destroyPQExpBuffer(cmd); + termPQExpBuffer(&cmd); return (status == 0); } @@ -1469,6 +1471,7 @@ static void bootstrap_template1(void) { PG_CMD_DECL; + PQExpBufferData cmd; char **line; char **bki_lines; char headerline[MAXPGPATH]; @@ -1530,16 +1533,17 @@ bootstrap_template1(void) /* Also ensure backend isn't confused by this environment var: */ unsetenv("PGCLIENTENCODING"); - snprintf(cmd, sizeof(cmd), - "\"%s\" --boot -X %d %s %s %s %s", - backend_exec, - wal_segment_size_mb * (1024 * 1024), - data_checksums ? "-k" : "", - boot_options, extra_options, - debug ? "-d 5" : ""); + initPQExpBuffer(&cmd); + + printfPQExpBuffer(&cmd, "\"%s\" --boot %s %s", backend_exec, boot_options, extra_options); + appendPQExpBuffer(&cmd, " -X %d", wal_segment_size_mb * (1024 * 1024)); + if (data_checksums) + appendPQExpBuffer(&cmd, " -k"); + if (debug) + appendPQExpBuffer(&cmd, " -d 5"); - PG_CMD_OPEN; + PG_CMD_OPEN(cmd.data); for (line = bki_lines; *line != NULL; line++) { @@ -1547,8 +1551,9 @@ bootstrap_template1(void) free(*line); } - PG_CMD_CLOSE; + PG_CMD_CLOSE(); + termPQExpBuffer(&cmd); free(bki_lines); check_ok(); @@ -2951,6 +2956,7 @@ void initialize_data_directory(void) { PG_CMD_DECL; + PQExpBufferData cmd; int i; setup_signals(); @@ -3014,12 +3020,11 @@ initialize_data_directory(void) fputs(_("performing post-bootstrap initialization ... "), stdout); fflush(stdout); - snprintf(cmd, sizeof(cmd), - "\"%s\" %s %s template1 >%s", - backend_exec, backend_options, extra_options, - DEVNULL); + initPQExpBuffer(&cmd); + printfPQExpBuffer(&cmd, "\"%s\" %s %s template1 >%s", + backend_exec, backend_options, extra_options, DEVNULL); - PG_CMD_OPEN; + PG_CMD_OPEN(cmd.data); setup_auth(cmdfd); @@ -3054,7 +3059,8 @@ initialize_data_directory(void) make_postgres(cmdfd); - PG_CMD_CLOSE; + PG_CMD_CLOSE(); + termPQExpBuffer(&cmd); check_ok(); } diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c index 3627b69e2a..36f134137f 100644 --- a/src/bin/pg_dump/pg_dumpall.c +++ b/src/bin/pg_dump/pg_dumpall.c @@ -1564,11 +1564,14 @@ dumpDatabases(PGconn *conn) static int runPgDump(const char *dbname, const char *create_opts) { - PQExpBuffer connstrbuf = createPQExpBuffer(); - PQExpBuffer cmd = createPQExpBuffer(); + PQExpBufferData connstrbuf; + PQExpBufferData cmd; int ret; - appendPQExpBuffer(cmd, "\"%s\" %s %s", pg_dump_bin, + initPQExpBuffer(&connstrbuf); + initPQExpBuffer(&cmd); + + printfPQExpBuffer(&cmd, "\"%s\" %s %s", pg_dump_bin, pgdumpopts->data, create_opts); /* @@ -1576,27 +1579,27 @@ runPgDump(const char *dbname, const char *create_opts) * format. */ if (filename) - appendPQExpBufferStr(cmd, " -Fa "); + appendPQExpBufferStr(&cmd, " -Fa "); else - appendPQExpBufferStr(cmd, " -Fp "); + appendPQExpBufferStr(&cmd, " -Fp "); /* * Append the database name to the already-constructed stem of connection * string. */ - appendPQExpBuffer(connstrbuf, "%s dbname=", connstr); - appendConnStrVal(connstrbuf, dbname); + appendPQExpBuffer(&connstrbuf, "%s dbname=", connstr); + appendConnStrVal(&connstrbuf, dbname); - appendShellString(cmd, connstrbuf->data); + appendShellString(&cmd, connstrbuf.data); - pg_log_info("running \"%s\"", cmd->data); + pg_log_info("running \"%s\"", cmd.data); fflush(NULL); - ret = system(cmd->data); + ret = system(cmd.data); - destroyPQExpBuffer(cmd); - destroyPQExpBuffer(connstrbuf); + termPQExpBuffer(&cmd); + termPQExpBuffer(&connstrbuf); return ret; } diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c index 60d34a40b2..3e4fd918dd 100644 --- a/src/test/regress/pg_regress.c +++ b/src/test/regress/pg_regress.c @@ -2293,6 +2293,7 @@ regression_main(int argc, char *argv[], if (temp_instance) { + StringInfoData cmd; FILE *pg_conf; const char *env_wait; int wait_seconds; @@ -2318,23 +2319,28 @@ regression_main(int argc, char *argv[], make_directory(buf); /* initdb */ - snprintf(buf, sizeof(buf), - "\"%s%sinitdb\" -D \"%s/data\" --no-clean --no-sync%s%s > \"%s/log/initdb.log\" 2>&1", - bindir ? bindir : "", - bindir ? "/" : "", - temp_instance, - debug ? " --debug" : "", - nolocale ? " --no-locale" : "", - outputdir); + initStringInfo(&cmd); + appendStringInfo(&cmd, + "\"%s%sinitdb\" -D \"%s/data\" --no-clean --no-sync", + bindir ? bindir : "", + bindir ? "/" : "", + temp_instance); + if (debug) + appendStringInfo(&cmd, " --debug"); + if (nolocale) + appendStringInfo(&cmd, " --no-locale"); + appendStringInfo(&cmd, " > \"%s/log/initdb.log\" 2>&1", outputdir); fflush(NULL); - if (system(buf)) + if (system(cmd.data)) { bail("initdb failed\n" "# Examine \"%s/log/initdb.log\" for the reason.\n" "# Command was: %s", - outputdir, buf); + outputdir, cmd.data); } + pfree(cmd.data); + /* * Adjust the default postgresql.conf for regression testing. The user * can specify a file to be appended; in any case we expand logging -- 2.41.0 ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Clean up command argument assembly @ 2023-07-04 12:14 Heikki Linnakangas <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Heikki Linnakangas @ 2023-07-04 12:14 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; pgsql-hackers On 26/06/2023 12:33, Peter Eisentraut wrote: > This is a small code cleanup patch. > > Several commands internally assemble command lines to call other > commands. This includes initdb, pg_dumpall, and pg_regress. (Also > pg_ctl, but that is different enough that I didn't consider it here.) > This has all evolved a bit organically, with fixed-size buffers, and > various optional command-line arguments being injected with > confusing-looking code, and the spacing between options handled in > inconsistent ways. This patch cleans all this up a bit to look clearer > and be more easily extensible with new arguments and options. +1 > We start each command with printfPQExpBuffer(), and then append > arguments as necessary with appendPQExpBuffer(). Also standardize on > using initPQExpBuffer() over createPQExpBuffer() where possible. > pg_regress uses StringInfo instead of PQExpBuffer, but many of the > same ideas apply. It's a bit bogus to use PQExpBuffer for these. If you run out of memory, you silently get an empty string instead. StringInfo, which exits the process on OOM, would be more appropriate. We have tons of such inappropriate uses of PQExpBuffer in all our client programs, though, so I don't insist on fixing this particular case right now. -- Heikki Linnakangas Neon (https://neon.tech) ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2023-07-04 12:14 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-09-12 13:07 [PATCH 7/8] Ignore correlation for new BRIN opclasses Tomas Vondra <[email protected]> 2023-06-26 09:33 Clean up command argument assembly Peter Eisentraut <[email protected]> 2023-07-04 12:14 ` Re: Clean up command argument assembly Heikki Linnakangas <[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