Skip to content

Commit 1738a61

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 c5a5bd0 commit 1738a61

File tree

6 files changed

+130
-279
lines changed

6 files changed

+130
-279
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 */
@@ -107,12 +121,9 @@ typedef struct _restoreOptions
107121
SimpleStringList tableNames;
108122

109123
int useDB;
110-
char *dbname; /* subject to expand_dbname */
111-
char *pgport;
112-
char *pghost;
113-
char *username;
124+
ConnParams cparams; /* parameters to use if useDB */
125+
114126
int noDataForFailedTables;
115-
trivalue promptPassword;
116127
int exit_on_error;
117128
int compression;
118129
int suppressDumpWarnings; /* Suppress output of WARNING entries
@@ -127,10 +138,8 @@ typedef struct _restoreOptions
127138

128139
typedef struct _dumpOptions
129140
{
130-
const char *dbname; /* subject to expand_dbname */
131-
const char *pghost;
132-
const char *pgport;
133-
const char *username;
141+
ConnParams cparams;
142+
134143
bool oids;
135144

136145
int binary_upgrade;
@@ -248,12 +257,9 @@ typedef void (*SetupWorkerPtrType) (Archive *AH);
248257
* Main archiver interface.
249258
*/
250259

251-
extern void ConnectDatabase(Archive *AH,
252-
const char *dbname,
253-
const char *pghost,
254-
const char *pgport,
255-
const char *username,
256-
trivalue prompt_password);
260+
extern void ConnectDatabase(Archive *AHX,
261+
const ConnParams *cparams,
262+
bool isReconnect);
257263
extern void DisconnectDatabase(Archive *AHX);
258264
extern PGconn *GetConnection(Archive *AHX);
259265

‎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
@@ -819,16 +823,8 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel)
819823
if (strcmp(te->desc, "DATABASE") == 0 ||
820824
strcmp(te->desc, "DATABASE PROPERTIES") == 0)
821825
{
822-
PQExpBufferData connstr;
823-
824-
initPQExpBuffer(&connstr);
825-
appendPQExpBufferStr(&connstr, "dbname=");
826-
appendConnStrVal(&connstr, te->tag);
827-
/* Abandon struct, but keep its buffer until process exit. */
828-
829826
ahlog(AH, 1, "connecting to new database \"%s\"\n", te->tag);
830827
_reconnectToDB(AH, te->tag);
831-
ropt->dbname = connstr.data;
832828
}
833829
}
834830

@@ -960,7 +956,7 @@ NewRestoreOptions(void)
960956

961957
/* set any fields that shouldn't default to zeroes */
962958
opts->format = archUnknown;
963-
opts->promptPassword = TRI_DEFAULT;
959+
opts->cparams.promptPassword = TRI_DEFAULT;
964960
opts->dumpSections = DUMP_UNSECTIONED;
965961

966962
return opts;
@@ -2386,8 +2382,6 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt,
23862382
else
23872383
AH->format = fmt;
23882384

2389-
AH->promptPassword = TRI_DEFAULT;
2390-
23912385
switch (AH->format)
23922386
{
23932387
case archCustom:
@@ -3256,27 +3250,20 @@ _doSetWithOids(ArchiveHandle *AH, const bool withOids)
32563250
* If we're currently restoring right into a database, this will
32573251
* actually establish a connection. Otherwise it puts a \connect into
32583252
* the script output.
3259-
*
3260-
* NULL dbname implies reconnecting to the current DB (pretty useless).
32613253
*/
32623254
static void
32633255
_reconnectToDB(ArchiveHandle *AH, const char *dbname)
32643256
{
32653257
if (RestoringToDB(AH))
3266-
ReconnectToServer(AH, dbname, NULL);
3258+
ReconnectToServer(AH, dbname);
32673259
else
32683260
{
3269-
if (dbname)
3270-
{
3271-
PQExpBufferData connectbuf;
3261+
PQExpBufferData connectbuf;
32723262

3273-
initPQExpBuffer(&connectbuf);
3274-
appendPsqlMetaConnect(&connectbuf, dbname);
3275-
ahprintf(AH, "%s\n", connectbuf.data);
3276-
termPQExpBuffer(&connectbuf);
3277-
}
3278-
else
3279-
ahprintf(AH, "%s\n", "\\connect -\n");
3263+
initPQExpBuffer(&connectbuf);
3264+
appendPsqlMetaConnect(&connectbuf, dbname);
3265+
ahprintf(AH, "%s\n", connectbuf.data);
3266+
termPQExpBuffer(&connectbuf);
32803267
}
32813268

32823269
/*
@@ -4186,9 +4173,7 @@ restore_toc_entries_postfork(ArchiveHandle *AH, TocEntry *pending_list)
41864173
/*
41874174
* Now reconnect the single parent connection.
41884175
*/
4189-
ConnectDatabase((Archive *) AH, ropt->dbname,
4190-
ropt->pghost, ropt->pgport, ropt->username,
4191-
ropt->promptPassword);
4176+
ConnectDatabase((Archive *) AH, &ropt->cparams, true);
41924177

41934178
/* re-establish fixed state */
41944179
_doSetFixedOutputState(AH);
@@ -4777,54 +4762,15 @@ CloneArchive(ArchiveHandle *AH)
47774762
clone->public.n_errors = 0;
47784763

47794764
/*
4780-
* Connect our new clone object to the database: In parallel restore the
4781-
* parent is already disconnected, because we can connect the worker
4782-
* processes independently to the database (no snapshot sync required). In
4783-
* parallel backup we clone the parent's existing connection.
4765+
* Connect our new clone object to the database, using the same connection
4766+
* parameters used for the original connection.
47844767
*/
4785-
if (AH->mode == archModeRead)
4786-
{
4787-
RestoreOptions *ropt = AH->public.ropt;
4788-
4789-
Assert(AH->connection == NULL);
4790-
4791-
/* this also sets clone->connection */
4792-
ConnectDatabase((Archive *) clone, ropt->dbname,
4793-
ropt->pghost, ropt->pgport, ropt->username,
4794-
ropt->promptPassword);
4768+
ConnectDatabase((Archive *) clone, &clone->public.ropt->cparams, true);
47954769

4796-
/* re-establish fixed state */
4770+
/* re-establish fixed state */
4771+
if (AH->mode == archModeRead)
47974772
_doSetFixedOutputState(clone);
4798-
}
4799-
else
4800-
{
4801-
PQExpBufferData connstr;
4802-
char *pghost;
4803-
char *pgport;
4804-
char *username;
4805-
4806-
Assert(AH->connection != NULL);
4807-
4808-
/*
4809-
* Even though we are technically accessing the parent's database
4810-
* object here, these functions are fine to be called like that
4811-
* because all just return a pointer and do not actually send/receive
4812-
* any data to/from the database.
4813-
*/
4814-
initPQExpBuffer(&connstr);
4815-
appendPQExpBuffer(&connstr, "dbname=");
4816-
appendConnStrVal(&connstr, PQdb(AH->connection));
4817-
pghost = PQhost(AH->connection);
4818-
pgport = PQport(AH->connection);
4819-
username = PQuser(AH->connection);
4820-
4821-
/* this also sets clone->connection */
4822-
ConnectDatabase((Archive *) clone, connstr.data,
4823-
pghost, pgport, username, TRI_NO);
4824-
4825-
termPQExpBuffer(&connstr);
4826-
/* setupDumpWorker will fix up connection state */
4827-
}
4773+
/* in write case, setupDumpWorker will fix up connection state */
48284774

48294775
/* Let the format-specific code have a chance too */
48304776
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;
@@ -455,7 +454,7 @@ extern void InitArchiveFmt_Tar(ArchiveHandle *AH);
455454

456455
extern bool isValidTarHeader(char *header);
457456

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

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

0 commit comments

Comments
 (0)