Skip to content

Commit 1888ff8

Browse files
tglsfdctglsfdc
committed
Fix handling of -d "connection string" in pg_dump/pg_restore.
Parallel pg_dump failed if its -d parameter was a connection string containing any essential information other than host, port, or username. The same was true for pg_restore with --create. The reason is that these scenarios failed to preserve the connection string from the command line; the code felt free to replace that with just the database name when reconnecting from a pg_dump parallel worker or after creating the target database. By chance, parallel pg_restore did not suffer this defect, as long as you didn't say --create. In practice it seems that the error would be obvious only if the connstring included essential, non-default SSL or GSS parameters. This may explain why it took us so long to notice. (It also makes it very difficult to craft a regression test case illustrating the problem, since the test would fail in builds without those options.) Fix by refactoring so that ConnectDatabase always receives all the relevant options directly from the command line, rather than reconstructed values. Inject a different database name, when necessary, by relying on libpq's rules for handling multiple "dbname" parameters. While here, let's get rid of the essentially duplicate _connectDB function, as well as some obsolete nearby cruft. Per bug #16604 from Zsolt Ero. Back-patch to all supported branches. Discussion: https://postgr.es/m/16604-933f4b8791227b15@postgresql.org
1 parent dd36d6b commit 1888ff8

File tree

6 files changed

+131
-291
lines changed

6 files changed

+131
-291
lines changed

‎src/bin/pg_dump/pg_backup.h

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,20 @@ typedef enum _teSection
5858
SECTION_POST_DATA /* stuff to be processed after data */
5959
} teSection;
6060

61+
/* Parameters needed by ConnectDatabase; same for dump and restore */
62+
typedef struct _connParams
63+
{
64+
/* These fields record the actual command line parameters */
65+
char *dbname; /* this may be a connstring! */
66+
char *pgport;
67+
char *pghost;
68+
char *username;
69+
trivalue promptPassword;
70+
/* If not NULL, this overrides the dbname obtained from command line */
71+
/* (but *only* the DB name, not anything else in the connstring) */
72+
char *override_dbname;
73+
} ConnParams;
74+
6175
typedef struct _restoreOptions
6276
{
6377
int createDB; /* Issue commands to create the database */
@@ -106,12 +120,9 @@ typedef struct _restoreOptions
106120
SimpleStringList tableNames;
107121

108122
int useDB;
109-
char *dbname; /* subject to expand_dbname */
110-
char *pgport;
111-
char *pghost;
112-
char *username;
123+
ConnParams cparams; /* parameters to use if useDB */
124+
113125
int noDataForFailedTables;
114-
trivalue promptPassword;
115126
int exit_on_error;
116127
int compression;
117128
int suppressDumpWarnings; /* Suppress output of WARNING entries
@@ -126,10 +137,8 @@ typedef struct _restoreOptions
126137

127138
typedef struct _dumpOptions
128139
{
129-
const char *dbname; /* subject to expand_dbname */
130-
const char *pghost;
131-
const char *pgport;
132-
const char *username;
140+
ConnParams cparams;
141+
133142
bool oids;
134143

135144
int binary_upgrade;
@@ -245,12 +254,9 @@ typedef void (*SetupWorkerPtrType) (Archive *AH);
245254
* Main archiver interface.
246255
*/
247256

248-
extern void ConnectDatabase(Archive *AH,
249-
const char *dbname,
250-
const char *pghost,
251-
const char *pgport,
252-
const char *username,
253-
trivalue prompt_password);
257+
extern void ConnectDatabase(Archive *AHX,
258+
const ConnParams *cparams,
259+
bool isReconnect);
254260
extern void DisconnectDatabase(Archive *AHX);
255261
extern PGconn *GetConnection(Archive *AHX);
256262

‎src/bin/pg_dump/pg_backup_archiver.c

Lines changed: 21 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,7 @@ InitDumpOptions(DumpOptions *opts)
144144
memset(opts, 0, sizeof(DumpOptions));
145145
/* set any fields that shouldn't default to zeroes */
146146
opts->include_everything = true;
147+
opts->cparams.promptPassword = TRI_DEFAULT;
147148
opts->dumpSections = DUMP_UNSECTIONED;
148149
}
149150

@@ -157,6 +158,11 @@ dumpOptionsFromRestoreOptions(RestoreOptions *ropt)
157158
DumpOptions *dopt = NewDumpOptions();
158159

159160
/* this is the inverse of what's at the end of pg_dump.c's main() */
161+
dopt->cparams.dbname = ropt->cparams.dbname ? pg_strdup(ropt->cparams.dbname) : NULL;
162+
dopt->cparams.pgport = ropt->cparams.pgport ? pg_strdup(ropt->cparams.pgport) : NULL;
163+
dopt->cparams.pghost = ropt->cparams.pghost ? pg_strdup(ropt->cparams.pghost) : NULL;
164+
dopt->cparams.username = ropt->cparams.username ? pg_strdup(ropt->cparams.username) : NULL;
165+
dopt->cparams.promptPassword = ropt->cparams.promptPassword;
160166
dopt->outputClean = ropt->dropSchema;
161167
dopt->dataOnly = ropt->dataOnly;
162168
dopt->schemaOnly = ropt->schemaOnly;
@@ -401,9 +407,7 @@ RestoreArchive(Archive *AHX)
401407
AHX->minRemoteVersion = 0;
402408
AHX->maxRemoteVersion = 9999999;
403409

404-
ConnectDatabase(AHX, ropt->dbname,
405-
ropt->pghost, ropt->pgport, ropt->username,
406-
ropt->promptPassword);
410+
ConnectDatabase(AHX, &ropt->cparams, false);
407411

408412
/*
409413
* If we're talking to the DB directly, don't send comments since they
@@ -823,16 +827,8 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel)
823827
/* If we created a DB, connect to it... */
824828
if (strcmp(te->desc, "DATABASE") == 0)
825829
{
826-
PQExpBufferData connstr;
827-
828-
initPQExpBuffer(&connstr);
829-
appendPQExpBufferStr(&connstr, "dbname=");
830-
appendConnStrVal(&connstr, te->tag);
831-
/* Abandon struct, but keep its buffer until process exit. */
832-
833830
ahlog(AH, 1, "connecting to new database \"%s\"\n", te->tag);
834831
_reconnectToDB(AH, te->tag);
835-
ropt->dbname = connstr.data;
836832
}
837833
}
838834

@@ -966,7 +962,7 @@ NewRestoreOptions(void)
966962

967963
/* set any fields that shouldn't default to zeroes */
968964
opts->format = archUnknown;
969-
opts->promptPassword = TRI_DEFAULT;
965+
opts->cparams.promptPassword = TRI_DEFAULT;
970966
opts->dumpSections = DUMP_UNSECTIONED;
971967

972968
return opts;
@@ -2398,8 +2394,6 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt,
23982394
else
23992395
AH->format = fmt;
24002396

2401-
AH->promptPassword = TRI_DEFAULT;
2402-
24032397
switch (AH->format)
24042398
{
24052399
case archCustom:
@@ -3188,27 +3182,20 @@ _doSetWithOids(ArchiveHandle *AH, const bool withOids)
31883182
* If we're currently restoring right into a database, this will
31893183
* actually establish a connection. Otherwise it puts a \connect into
31903184
* the script output.
3191-
*
3192-
* NULL dbname implies reconnecting to the current DB (pretty useless).
31933185
*/
31943186
static void
31953187
_reconnectToDB(ArchiveHandle *AH, const char *dbname)
31963188
{
31973189
if (RestoringToDB(AH))
3198-
ReconnectToServer(AH, dbname, NULL);
3190+
ReconnectToServer(AH, dbname);
31993191
else
32003192
{
3201-
if (dbname)
3202-
{
3203-
PQExpBufferData connectbuf;
3193+
PQExpBufferData connectbuf;
32043194

3205-
initPQExpBuffer(&connectbuf);
3206-
appendPsqlMetaConnect(&connectbuf, dbname);
3207-
ahprintf(AH, "%s\n", connectbuf.data);
3208-
termPQExpBuffer(&connectbuf);
3209-
}
3210-
else
3211-
ahprintf(AH, "%s\n", "\\connect -\n");
3195+
initPQExpBuffer(&connectbuf);
3196+
appendPsqlMetaConnect(&connectbuf, dbname);
3197+
ahprintf(AH, "%s\n", connectbuf.data);
3198+
termPQExpBuffer(&connectbuf);
32123199
}
32133200

32143201
/*
@@ -4138,9 +4125,7 @@ restore_toc_entries_postfork(ArchiveHandle *AH, TocEntry *pending_list)
41384125
/*
41394126
* Now reconnect the single parent connection.
41404127
*/
4141-
ConnectDatabase((Archive *) AH, ropt->dbname,
4142-
ropt->pghost, ropt->pgport, ropt->username,
4143-
ropt->promptPassword);
4128+
ConnectDatabase((Archive *) AH, &ropt->cparams, true);
41444129

41454130
/* re-establish fixed state */
41464131
_doSetFixedOutputState(AH);
@@ -4729,54 +4714,15 @@ CloneArchive(ArchiveHandle *AH)
47294714
clone->public.n_errors = 0;
47304715

47314716
/*
4732-
* Connect our new clone object to the database: In parallel restore the
4733-
* parent is already disconnected, because we can connect the worker
4734-
* processes independently to the database (no snapshot sync required). In
4735-
* parallel backup we clone the parent's existing connection.
4717+
* Connect our new clone object to the database, using the same connection
4718+
* parameters used for the original connection.
47364719
*/
4737-
if (AH->mode == archModeRead)
4738-
{
4739-
RestoreOptions *ropt = AH->public.ropt;
4740-
4741-
Assert(AH->connection == NULL);
4742-
4743-
/* this also sets clone->connection */
4744-
ConnectDatabase((Archive *) clone, ropt->dbname,
4745-
ropt->pghost, ropt->pgport, ropt->username,
4746-
ropt->promptPassword);
4720+
ConnectDatabase((Archive *) clone, &clone->public.ropt->cparams, true);
47474721

4748-
/* re-establish fixed state */
4722+
/* re-establish fixed state */
4723+
if (AH->mode == archModeRead)
47494724
_doSetFixedOutputState(clone);
4750-
}
4751-
else
4752-
{
4753-
PQExpBufferData connstr;
4754-
char *pghost;
4755-
char *pgport;
4756-
char *username;
4757-
4758-
Assert(AH->connection != NULL);
4759-
4760-
/*
4761-
* Even though we are technically accessing the parent's database
4762-
* object here, these functions are fine to be called like that
4763-
* because all just return a pointer and do not actually send/receive
4764-
* any data to/from the database.
4765-
*/
4766-
initPQExpBuffer(&connstr);
4767-
appendPQExpBuffer(&connstr, "dbname=");
4768-
appendConnStrVal(&connstr, PQdb(AH->connection));
4769-
pghost = PQhost(AH->connection);
4770-
pgport = PQport(AH->connection);
4771-
username = PQuser(AH->connection);
4772-
4773-
/* this also sets clone->connection */
4774-
ConnectDatabase((Archive *) clone, connstr.data,
4775-
pghost, pgport, username, TRI_NO);
4776-
4777-
termPQExpBuffer(&connstr);
4778-
/* setupDumpWorker will fix up connection state */
4779-
}
4725+
/* in write case, setupDumpWorker will fix up connection state */
47804726

47814727
/* Let the format-specific code have a chance too */
47824728
(clone->ClonePtr) (clone);

‎src/bin/pg_dump/pg_backup_archiver.h

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,6 @@ struct _archiveHandle
308308

309309
/* Stuff for direct DB connection */
310310
char *archdbname; /* DB name *read* from archive */
311-
trivalue promptPassword;
312311
char *savedPassword; /* password for ropt->username, if known */
313312
char *use_role;
314313
PGconn *connection;
@@ -454,7 +453,7 @@ extern void InitArchiveFmt_Tar(ArchiveHandle *AH);
454453

455454
extern bool isValidTarHeader(char *header);
456455

457-
extern int ReconnectToServer(ArchiveHandle *AH, const char *dbname, const char *newUser);
456+
extern void ReconnectToServer(ArchiveHandle *AH, const char *dbname);
458457
extern void DropBlobIfExists(ArchiveHandle *AH, Oid oid);
459458

460459
void ahwrite(const void *ptr, size_t size, size_t nmemb, ArchiveHandle *AH);

0 commit comments

Comments
 (0)