Skip to content

Commit 56b46d3

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 23d8b35 commit 56b46d3

File tree

6 files changed

+131
-296
lines changed

6 files changed

+131
-296
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 */
@@ -102,12 +116,9 @@ typedef struct _restoreOptions
102116
SimpleStringList tableNames;
103117

104118
int useDB;
105-
char *dbname; /* subject to expand_dbname */
106-
char *pgport;
107-
char *pghost;
108-
char *username;
119+
ConnParams cparams; /* parameters to use if useDB */
120+
109121
int noDataForFailedTables;
110-
trivalue promptPassword;
111122
int exit_on_error;
112123
int compression;
113124
int suppressDumpWarnings; /* Suppress output of WARNING entries
@@ -121,10 +132,8 @@ typedef struct _restoreOptions
121132

122133
typedef struct _dumpOptions
123134
{
124-
const char *dbname; /* subject to expand_dbname */
125-
const char *pghost;
126-
const char *pgport;
127-
const char *username;
135+
ConnParams cparams;
136+
128137
bool oids;
129138

130139
int binary_upgrade;
@@ -236,12 +245,9 @@ typedef void (*SetupWorkerPtr) (Archive *AH);
236245
* Main archiver interface.
237246
*/
238247

239-
extern void ConnectDatabase(Archive *AH,
240-
const char *dbname,
241-
const char *pghost,
242-
const char *pgport,
243-
const char *username,
244-
trivalue prompt_password);
248+
extern void ConnectDatabase(Archive *AHX,
249+
const ConnParams *cparams,
250+
bool isReconnect);
245251
extern void DisconnectDatabase(Archive *AHX);
246252
extern PGconn *GetConnection(Archive *AHX);
247253

‎src/bin/pg_dump/pg_backup_archiver.c

Lines changed: 21 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,7 @@ InitDumpOptions(DumpOptions *opts)
135135
memset(opts, 0, sizeof(DumpOptions));
136136
/* set any fields that shouldn't default to zeroes */
137137
opts->include_everything = true;
138+
opts->cparams.promptPassword = TRI_DEFAULT;
138139
opts->dumpSections = DUMP_UNSECTIONED;
139140
}
140141

@@ -148,6 +149,11 @@ dumpOptionsFromRestoreOptions(RestoreOptions *ropt)
148149
DumpOptions *dopt = NewDumpOptions();
149150

150151
/* this is the inverse of what's at the end of pg_dump.c's main() */
152+
dopt->cparams.dbname = ropt->cparams.dbname ? pg_strdup(ropt->cparams.dbname) : NULL;
153+
dopt->cparams.pgport = ropt->cparams.pgport ? pg_strdup(ropt->cparams.pgport) : NULL;
154+
dopt->cparams.pghost = ropt->cparams.pghost ? pg_strdup(ropt->cparams.pghost) : NULL;
155+
dopt->cparams.username = ropt->cparams.username ? pg_strdup(ropt->cparams.username) : NULL;
156+
dopt->cparams.promptPassword = ropt->cparams.promptPassword;
151157
dopt->outputClean = ropt->dropSchema;
152158
dopt->dataOnly = ropt->dataOnly;
153159
dopt->schemaOnly = ropt->schemaOnly;
@@ -383,9 +389,7 @@ RestoreArchive(Archive *AHX)
383389
AHX->minRemoteVersion = 0;
384390
AHX->maxRemoteVersion = 999999;
385391

386-
ConnectDatabase(AHX, ropt->dbname,
387-
ropt->pghost, ropt->pgport, ropt->username,
388-
ropt->promptPassword);
392+
ConnectDatabase(AHX, &ropt->cparams, false);
389393

390394
/*
391395
* If we're talking to the DB directly, don't send comments since they
@@ -805,16 +809,8 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel)
805809
/* If we created a DB, connect to it... */
806810
if (strcmp(te->desc, "DATABASE") == 0)
807811
{
808-
PQExpBufferData connstr;
809-
810-
initPQExpBuffer(&connstr);
811-
appendPQExpBufferStr(&connstr, "dbname=");
812-
appendConnStrVal(&connstr, te->tag);
813-
/* Abandon struct, but keep its buffer until process exit. */
814-
815812
ahlog(AH, 1, "connecting to new database \"%s\"\n", te->tag);
816813
_reconnectToDB(AH, te->tag);
817-
ropt->dbname = connstr.data;
818814
}
819815
}
820816

@@ -948,7 +944,7 @@ NewRestoreOptions(void)
948944

949945
/* set any fields that shouldn't default to zeroes */
950946
opts->format = archUnknown;
951-
opts->promptPassword = TRI_DEFAULT;
947+
opts->cparams.promptPassword = TRI_DEFAULT;
952948
opts->dumpSections = DUMP_UNSECTIONED;
953949

954950
return opts;
@@ -2376,8 +2372,6 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt,
23762372
else
23772373
AH->format = fmt;
23782374

2379-
AH->promptPassword = TRI_DEFAULT;
2380-
23812375
switch (AH->format)
23822376
{
23832377
case archCustom:
@@ -3072,27 +3066,20 @@ _doSetWithOids(ArchiveHandle *AH, const bool withOids)
30723066
* If we're currently restoring right into a database, this will
30733067
* actually establish a connection. Otherwise it puts a \connect into
30743068
* the script output.
3075-
*
3076-
* NULL dbname implies reconnecting to the current DB (pretty useless).
30773069
*/
30783070
static void
30793071
_reconnectToDB(ArchiveHandle *AH, const char *dbname)
30803072
{
30813073
if (RestoringToDB(AH))
3082-
ReconnectToServer(AH, dbname, NULL);
3074+
ReconnectToServer(AH, dbname);
30833075
else
30843076
{
3085-
if (dbname)
3086-
{
3087-
PQExpBufferData connectbuf;
3077+
PQExpBufferData connectbuf;
30883078

3089-
initPQExpBuffer(&connectbuf);
3090-
appendPsqlMetaConnect(&connectbuf, dbname);
3091-
ahprintf(AH, "%s\n", connectbuf.data);
3092-
termPQExpBuffer(&connectbuf);
3093-
}
3094-
else
3095-
ahprintf(AH, "%s\n", "\\connect -\n");
3079+
initPQExpBuffer(&connectbuf);
3080+
appendPsqlMetaConnect(&connectbuf, dbname);
3081+
ahprintf(AH, "%s\n", connectbuf.data);
3082+
termPQExpBuffer(&connectbuf);
30963083
}
30973084

30983085
/*
@@ -4039,9 +4026,7 @@ restore_toc_entries_postfork(ArchiveHandle *AH, TocEntry *pending_list)
40394026
/*
40404027
* Now reconnect the single parent connection.
40414028
*/
4042-
ConnectDatabase((Archive *) AH, ropt->dbname,
4043-
ropt->pghost, ropt->pgport, ropt->username,
4044-
ropt->promptPassword);
4029+
ConnectDatabase((Archive *) AH, &ropt->cparams, true);
40454030

40464031
/* re-establish fixed state */
40474032
_doSetFixedOutputState(AH);
@@ -4634,54 +4619,15 @@ CloneArchive(ArchiveHandle *AH)
46344619
clone->public.n_errors = 0;
46354620

46364621
/*
4637-
* Connect our new clone object to the database: In parallel restore the
4638-
* parent is already disconnected, because we can connect the worker
4639-
* processes independently to the database (no snapshot sync required). In
4640-
* parallel backup we clone the parent's existing connection.
4622+
* Connect our new clone object to the database, using the same connection
4623+
* parameters used for the original connection.
46414624
*/
4642-
if (AH->mode == archModeRead)
4643-
{
4644-
RestoreOptions *ropt = AH->public.ropt;
4645-
4646-
Assert(AH->connection == NULL);
4647-
4648-
/* this also sets clone->connection */
4649-
ConnectDatabase((Archive *) clone, ropt->dbname,
4650-
ropt->pghost, ropt->pgport, ropt->username,
4651-
ropt->promptPassword);
4625+
ConnectDatabase((Archive *) clone, &clone->public.ropt->cparams, true);
46524626

4653-
/* re-establish fixed state */
4627+
/* re-establish fixed state */
4628+
if (AH->mode == archModeRead)
46544629
_doSetFixedOutputState(clone);
4655-
}
4656-
else
4657-
{
4658-
PQExpBufferData connstr;
4659-
char *pghost;
4660-
char *pgport;
4661-
char *username;
4662-
4663-
Assert(AH->connection != NULL);
4664-
4665-
/*
4666-
* Even though we are technically accessing the parent's database
4667-
* object here, these functions are fine to be called like that
4668-
* because all just return a pointer and do not actually send/receive
4669-
* any data to/from the database.
4670-
*/
4671-
initPQExpBuffer(&connstr);
4672-
appendPQExpBuffer(&connstr, "dbname=");
4673-
appendConnStrVal(&connstr, PQdb(AH->connection));
4674-
pghost = PQhost(AH->connection);
4675-
pgport = PQport(AH->connection);
4676-
username = PQuser(AH->connection);
4677-
4678-
/* this also sets clone->connection */
4679-
ConnectDatabase((Archive *) clone, connstr.data,
4680-
pghost, pgport, username, TRI_NO);
4681-
4682-
termPQExpBuffer(&connstr);
4683-
/* setupDumpWorker will fix up connection state */
4684-
}
4630+
/* in write case, setupDumpWorker will fix up connection state */
46854631

46864632
/* Let the format-specific code have a chance too */
46874633
(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
@@ -311,7 +311,6 @@ struct _archiveHandle
311311

312312
/* Stuff for direct DB connection */
313313
char *archdbname; /* DB name *read* from archive */
314-
trivalue promptPassword;
315314
char *savedPassword; /* password for ropt->username, if known */
316315
char *use_role;
317316
PGconn *connection;
@@ -456,7 +455,7 @@ extern void InitArchiveFmt_Tar(ArchiveHandle *AH);
456455

457456
extern bool isValidTarHeader(char *header);
458457

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

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

0 commit comments

Comments
 (0)