Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit cccdbc5

Browse filesBrowse files
peterepetere
committed
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. Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi> Discussion: https://www.postgresql.org/message-id/flat/16d0beac-a141-e5d3-60e9-323da75f49bf@eisentraut.org
1 parent fa88928 commit cccdbc5
Copy full SHA for cccdbc5

File tree

3 files changed

+62
-47
lines changed
Filter options

3 files changed

+62
-47
lines changed

‎src/bin/initdb/initdb.c

Copy file name to clipboardExpand all lines: src/bin/initdb/initdb.c
+31-25Lines changed: 31 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -309,16 +309,16 @@ void initialize_data_directory(void);
309309
/*
310310
* macros for running pipes to postgres
311311
*/
312-
#define PG_CMD_DECL char cmd[MAXPGPATH]; FILE *cmdfd
312+
#define PG_CMD_DECL FILE *cmdfd
313313

314-
#define PG_CMD_OPEN \
314+
#define PG_CMD_OPEN(cmd) \
315315
do { \
316316
cmdfd = popen_check(cmd, "w"); \
317317
if (cmdfd == NULL) \
318318
exit(1); /* message already printed by popen_check */ \
319319
} while (0)
320320

321-
#define PG_CMD_CLOSE \
321+
#define PG_CMD_CLOSE() \
322322
do { \
323323
if (pclose_check(cmdfd)) \
324324
exit(1); /* message already printed by pclose_check */ \
@@ -1138,13 +1138,15 @@ test_config_settings(void)
11381138
static bool
11391139
test_specific_config_settings(int test_conns, int test_buffs)
11401140
{
1141-
PQExpBuffer cmd = createPQExpBuffer();
1141+
PQExpBufferData cmd;
11421142
_stringlist *gnames,
11431143
*gvalues;
11441144
int status;
11451145

1146+
initPQExpBuffer(&cmd);
1147+
11461148
/* Set up the test postmaster invocation */
1147-
printfPQExpBuffer(cmd,
1149+
printfPQExpBuffer(&cmd,
11481150
"\"%s\" --check %s %s "
11491151
"-c max_connections=%d "
11501152
"-c shared_buffers=%d "
@@ -1158,18 +1160,18 @@ test_specific_config_settings(int test_conns, int test_buffs)
11581160
gnames != NULL; /* assume lists have the same length */
11591161
gnames = gnames->next, gvalues = gvalues->next)
11601162
{
1161-
appendPQExpBuffer(cmd, " -c %s=", gnames->str);
1162-
appendShellString(cmd, gvalues->str);
1163+
appendPQExpBuffer(&cmd, " -c %s=", gnames->str);
1164+
appendShellString(&cmd, gvalues->str);
11631165
}
11641166

1165-
appendPQExpBuffer(cmd,
1167+
appendPQExpBuffer(&cmd,
11661168
" < \"%s\" > \"%s\" 2>&1",
11671169
DEVNULL, DEVNULL);
11681170

11691171
fflush(NULL);
1170-
status = system(cmd->data);
1172+
status = system(cmd.data);
11711173

1172-
destroyPQExpBuffer(cmd);
1174+
termPQExpBuffer(&cmd);
11731175

11741176
return (status == 0);
11751177
}
@@ -1469,6 +1471,7 @@ static void
14691471
bootstrap_template1(void)
14701472
{
14711473
PG_CMD_DECL;
1474+
PQExpBufferData cmd;
14721475
char **line;
14731476
char **bki_lines;
14741477
char headerline[MAXPGPATH];
@@ -1530,25 +1533,27 @@ bootstrap_template1(void)
15301533
/* Also ensure backend isn't confused by this environment var: */
15311534
unsetenv("PGCLIENTENCODING");
15321535

1533-
snprintf(cmd, sizeof(cmd),
1534-
"\"%s\" --boot -X %d %s %s %s %s",
1535-
backend_exec,
1536-
wal_segment_size_mb * (1024 * 1024),
1537-
data_checksums ? "-k" : "",
1538-
boot_options, extra_options,
1539-
debug ? "-d 5" : "");
1536+
initPQExpBuffer(&cmd);
1537+
1538+
printfPQExpBuffer(&cmd, "\"%s\" --boot %s %s", backend_exec, boot_options, extra_options);
1539+
appendPQExpBuffer(&cmd, " -X %d", wal_segment_size_mb * (1024 * 1024));
1540+
if (data_checksums)
1541+
appendPQExpBuffer(&cmd, " -k");
1542+
if (debug)
1543+
appendPQExpBuffer(&cmd, " -d 5");
15401544

15411545

1542-
PG_CMD_OPEN;
1546+
PG_CMD_OPEN(cmd.data);
15431547

15441548
for (line = bki_lines; *line != NULL; line++)
15451549
{
15461550
PG_CMD_PUTS(*line);
15471551
free(*line);
15481552
}
15491553

1550-
PG_CMD_CLOSE;
1554+
PG_CMD_CLOSE();
15511555

1556+
termPQExpBuffer(&cmd);
15521557
free(bki_lines);
15531558

15541559
check_ok();
@@ -2951,6 +2956,7 @@ void
29512956
initialize_data_directory(void)
29522957
{
29532958
PG_CMD_DECL;
2959+
PQExpBufferData cmd;
29542960
int i;
29552961

29562962
setup_signals();
@@ -3014,12 +3020,11 @@ initialize_data_directory(void)
30143020
fputs(_("performing post-bootstrap initialization ... "), stdout);
30153021
fflush(stdout);
30163022

3017-
snprintf(cmd, sizeof(cmd),
3018-
"\"%s\" %s %s template1 >%s",
3019-
backend_exec, backend_options, extra_options,
3020-
DEVNULL);
3023+
initPQExpBuffer(&cmd);
3024+
printfPQExpBuffer(&cmd, "\"%s\" %s %s template1 >%s",
3025+
backend_exec, backend_options, extra_options, DEVNULL);
30213026

3022-
PG_CMD_OPEN;
3027+
PG_CMD_OPEN(cmd.data);
30233028

30243029
setup_auth(cmdfd);
30253030

@@ -3054,7 +3059,8 @@ initialize_data_directory(void)
30543059

30553060
make_postgres(cmdfd);
30563061

3057-
PG_CMD_CLOSE;
3062+
PG_CMD_CLOSE();
3063+
termPQExpBuffer(&cmd);
30583064

30593065
check_ok();
30603066
}

‎src/bin/pg_dump/pg_dumpall.c

Copy file name to clipboardExpand all lines: src/bin/pg_dump/pg_dumpall.c
+15-12Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1564,39 +1564,42 @@ dumpDatabases(PGconn *conn)
15641564
static int
15651565
runPgDump(const char *dbname, const char *create_opts)
15661566
{
1567-
PQExpBuffer connstrbuf = createPQExpBuffer();
1568-
PQExpBuffer cmd = createPQExpBuffer();
1567+
PQExpBufferData connstrbuf;
1568+
PQExpBufferData cmd;
15691569
int ret;
15701570

1571-
appendPQExpBuffer(cmd, "\"%s\" %s %s", pg_dump_bin,
1571+
initPQExpBuffer(&connstrbuf);
1572+
initPQExpBuffer(&cmd);
1573+
1574+
printfPQExpBuffer(&cmd, "\"%s\" %s %s", pg_dump_bin,
15721575
pgdumpopts->data, create_opts);
15731576

15741577
/*
15751578
* If we have a filename, use the undocumented plain-append pg_dump
15761579
* format.
15771580
*/
15781581
if (filename)
1579-
appendPQExpBufferStr(cmd, " -Fa ");
1582+
appendPQExpBufferStr(&cmd, " -Fa ");
15801583
else
1581-
appendPQExpBufferStr(cmd, " -Fp ");
1584+
appendPQExpBufferStr(&cmd, " -Fp ");
15821585

15831586
/*
15841587
* Append the database name to the already-constructed stem of connection
15851588
* string.
15861589
*/
1587-
appendPQExpBuffer(connstrbuf, "%s dbname=", connstr);
1588-
appendConnStrVal(connstrbuf, dbname);
1590+
appendPQExpBuffer(&connstrbuf, "%s dbname=", connstr);
1591+
appendConnStrVal(&connstrbuf, dbname);
15891592

1590-
appendShellString(cmd, connstrbuf->data);
1593+
appendShellString(&cmd, connstrbuf.data);
15911594

1592-
pg_log_info("running \"%s\"", cmd->data);
1595+
pg_log_info("running \"%s\"", cmd.data);
15931596

15941597
fflush(NULL);
15951598

1596-
ret = system(cmd->data);
1599+
ret = system(cmd.data);
15971600

1598-
destroyPQExpBuffer(cmd);
1599-
destroyPQExpBuffer(connstrbuf);
1601+
termPQExpBuffer(&cmd);
1602+
termPQExpBuffer(&connstrbuf);
16001603

16011604
return ret;
16021605
}

‎src/test/regress/pg_regress.c

Copy file name to clipboardExpand all lines: src/test/regress/pg_regress.c
+16-10Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2293,6 +2293,7 @@ regression_main(int argc, char *argv[],
22932293

22942294
if (temp_instance)
22952295
{
2296+
StringInfoData cmd;
22962297
FILE *pg_conf;
22972298
const char *env_wait;
22982299
int wait_seconds;
@@ -2318,23 +2319,28 @@ regression_main(int argc, char *argv[],
23182319
make_directory(buf);
23192320

23202321
/* initdb */
2321-
snprintf(buf, sizeof(buf),
2322-
"\"%s%sinitdb\" -D \"%s/data\" --no-clean --no-sync%s%s > \"%s/log/initdb.log\" 2>&1",
2323-
bindir ? bindir : "",
2324-
bindir ? "/" : "",
2325-
temp_instance,
2326-
debug ? " --debug" : "",
2327-
nolocale ? " --no-locale" : "",
2328-
outputdir);
2322+
initStringInfo(&cmd);
2323+
appendStringInfo(&cmd,
2324+
"\"%s%sinitdb\" -D \"%s/data\" --no-clean --no-sync",
2325+
bindir ? bindir : "",
2326+
bindir ? "/" : "",
2327+
temp_instance);
2328+
if (debug)
2329+
appendStringInfo(&cmd, " --debug");
2330+
if (nolocale)
2331+
appendStringInfo(&cmd, " --no-locale");
2332+
appendStringInfo(&cmd, " > \"%s/log/initdb.log\" 2>&1", outputdir);
23292333
fflush(NULL);
2330-
if (system(buf))
2334+
if (system(cmd.data))
23312335
{
23322336
bail("initdb failed\n"
23332337
"# Examine \"%s/log/initdb.log\" for the reason.\n"
23342338
"# Command was: %s",
2335-
outputdir, buf);
2339+
outputdir, cmd.data);
23362340
}
23372341

2342+
pfree(cmd.data);
2343+
23382344
/*
23392345
* Adjust the default postgresql.conf for regression testing. The user
23402346
* can specify a file to be appended; in any case we expand logging

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.