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 7c154f2

Browse filesBrowse 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 d38e159 commit 7c154f2
Copy full SHA for 7c154f2

File tree

Expand file treeCollapse file tree

6 files changed

+131
-296
lines changed
Filter options
Expand file treeCollapse file tree

6 files changed

+131
-296
lines changed

‎src/bin/pg_dump/pg_backup.h

Copy file name to clipboardExpand all lines: src/bin/pg_dump/pg_backup.h
+21-15Lines 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 */
@@ -103,12 +117,9 @@ typedef struct _restoreOptions
103117
SimpleStringList tableNames;
104118

105119
int useDB;
106-
char *dbname; /* subject to expand_dbname */
107-
char *pgport;
108-
char *pghost;
109-
char *username;
120+
ConnParams cparams; /* parameters to use if useDB */
121+
110122
int noDataForFailedTables;
111-
trivalue promptPassword;
112123
int exit_on_error;
113124
int compression;
114125
int suppressDumpWarnings; /* Suppress output of WARNING entries
@@ -122,10 +133,8 @@ typedef struct _restoreOptions
122133

123134
typedef struct _dumpOptions
124135
{
125-
const char *dbname; /* subject to expand_dbname */
126-
const char *pghost;
127-
const char *pgport;
128-
const char *username;
136+
ConnParams cparams;
137+
129138
bool oids;
130139

131140
int binary_upgrade;
@@ -237,12 +246,9 @@ typedef void (*SetupWorkerPtr) (Archive *AH);
237246
* Main archiver interface.
238247
*/
239248

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

‎src/bin/pg_dump/pg_backup_archiver.c

Copy file name to clipboardExpand all lines: src/bin/pg_dump/pg_backup_archiver.c
+21-75Lines changed: 21 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,7 @@ InitDumpOptions(DumpOptions *opts)
140140
memset(opts, 0, sizeof(DumpOptions));
141141
/* set any fields that shouldn't default to zeroes */
142142
opts->include_everything = true;
143+
opts->cparams.promptPassword = TRI_DEFAULT;
143144
opts->dumpSections = DUMP_UNSECTIONED;
144145
}
145146

@@ -153,6 +154,11 @@ dumpOptionsFromRestoreOptions(RestoreOptions *ropt)
153154
DumpOptions *dopt = NewDumpOptions();
154155

155156
/* this is the inverse of what's at the end of pg_dump.c's main() */
157+
dopt->cparams.dbname = ropt->cparams.dbname ? pg_strdup(ropt->cparams.dbname) : NULL;
158+
dopt->cparams.pgport = ropt->cparams.pgport ? pg_strdup(ropt->cparams.pgport) : NULL;
159+
dopt->cparams.pghost = ropt->cparams.pghost ? pg_strdup(ropt->cparams.pghost) : NULL;
160+
dopt->cparams.username = ropt->cparams.username ? pg_strdup(ropt->cparams.username) : NULL;
161+
dopt->cparams.promptPassword = ropt->cparams.promptPassword;
156162
dopt->outputClean = ropt->dropSchema;
157163
dopt->dataOnly = ropt->dataOnly;
158164
dopt->schemaOnly = ropt->schemaOnly;
@@ -392,9 +398,7 @@ RestoreArchive(Archive *AHX)
392398
AHX->minRemoteVersion = 0;
393399
AHX->maxRemoteVersion = 999999;
394400

395-
ConnectDatabase(AHX, ropt->dbname,
396-
ropt->pghost, ropt->pgport, ropt->username,
397-
ropt->promptPassword);
401+
ConnectDatabase(AHX, &ropt->cparams, false);
398402

399403
/*
400404
* If we're talking to the DB directly, don't send comments since they
@@ -814,16 +818,8 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel)
814818
/* If we created a DB, connect to it... */
815819
if (strcmp(te->desc, "DATABASE") == 0)
816820
{
817-
PQExpBufferData connstr;
818-
819-
initPQExpBuffer(&connstr);
820-
appendPQExpBufferStr(&connstr, "dbname=");
821-
appendConnStrVal(&connstr, te->tag);
822-
/* Abandon struct, but keep its buffer until process exit. */
823-
824821
ahlog(AH, 1, "connecting to new database \"%s\"\n", te->tag);
825822
_reconnectToDB(AH, te->tag);
826-
ropt->dbname = connstr.data;
827823
}
828824
}
829825

@@ -957,7 +953,7 @@ NewRestoreOptions(void)
957953

958954
/* set any fields that shouldn't default to zeroes */
959955
opts->format = archUnknown;
960-
opts->promptPassword = TRI_DEFAULT;
956+
opts->cparams.promptPassword = TRI_DEFAULT;
961957
opts->dumpSections = DUMP_UNSECTIONED;
962958

963959
return opts;
@@ -2389,8 +2385,6 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt,
23892385
else
23902386
AH->format = fmt;
23912387

2392-
AH->promptPassword = TRI_DEFAULT;
2393-
23942388
switch (AH->format)
23952389
{
23962390
case archCustom:
@@ -3135,27 +3129,20 @@ _doSetWithOids(ArchiveHandle *AH, const bool withOids)
31353129
* If we're currently restoring right into a database, this will
31363130
* actually establish a connection. Otherwise it puts a \connect into
31373131
* the script output.
3138-
*
3139-
* NULL dbname implies reconnecting to the current DB (pretty useless).
31403132
*/
31413133
static void
31423134
_reconnectToDB(ArchiveHandle *AH, const char *dbname)
31433135
{
31443136
if (RestoringToDB(AH))
3145-
ReconnectToServer(AH, dbname, NULL);
3137+
ReconnectToServer(AH, dbname);
31463138
else
31473139
{
3148-
if (dbname)
3149-
{
3150-
PQExpBufferData connectbuf;
3140+
PQExpBufferData connectbuf;
31513141

3152-
initPQExpBuffer(&connectbuf);
3153-
appendPsqlMetaConnect(&connectbuf, dbname);
3154-
ahprintf(AH, "%s\n", connectbuf.data);
3155-
termPQExpBuffer(&connectbuf);
3156-
}
3157-
else
3158-
ahprintf(AH, "%s\n", "\\connect -\n");
3142+
initPQExpBuffer(&connectbuf);
3143+
appendPsqlMetaConnect(&connectbuf, dbname);
3144+
ahprintf(AH, "%s\n", connectbuf.data);
3145+
termPQExpBuffer(&connectbuf);
31593146
}
31603147

31613148
/*
@@ -4102,9 +4089,7 @@ restore_toc_entries_postfork(ArchiveHandle *AH, TocEntry *pending_list)
41024089
/*
41034090
* Now reconnect the single parent connection.
41044091
*/
4105-
ConnectDatabase((Archive *) AH, ropt->dbname,
4106-
ropt->pghost, ropt->pgport, ropt->username,
4107-
ropt->promptPassword);
4092+
ConnectDatabase((Archive *) AH, &ropt->cparams, true);
41084093

41094094
/* re-establish fixed state */
41104095
_doSetFixedOutputState(AH);
@@ -4697,54 +4682,15 @@ CloneArchive(ArchiveHandle *AH)
46974682
clone->public.n_errors = 0;
46984683

46994684
/*
4700-
* Connect our new clone object to the database: In parallel restore the
4701-
* parent is already disconnected, because we can connect the worker
4702-
* processes independently to the database (no snapshot sync required). In
4703-
* parallel backup we clone the parent's existing connection.
4685+
* Connect our new clone object to the database, using the same connection
4686+
* parameters used for the original connection.
47044687
*/
4705-
if (AH->mode == archModeRead)
4706-
{
4707-
RestoreOptions *ropt = AH->public.ropt;
4708-
4709-
Assert(AH->connection == NULL);
4710-
4711-
/* this also sets clone->connection */
4712-
ConnectDatabase((Archive *) clone, ropt->dbname,
4713-
ropt->pghost, ropt->pgport, ropt->username,
4714-
ropt->promptPassword);
4688+
ConnectDatabase((Archive *) clone, &clone->public.ropt->cparams, true);
47154689

4716-
/* re-establish fixed state */
4690+
/* re-establish fixed state */
4691+
if (AH->mode == archModeRead)
47174692
_doSetFixedOutputState(clone);
4718-
}
4719-
else
4720-
{
4721-
PQExpBufferData connstr;
4722-
char *pghost;
4723-
char *pgport;
4724-
char *username;
4725-
4726-
Assert(AH->connection != NULL);
4727-
4728-
/*
4729-
* Even though we are technically accessing the parent's database
4730-
* object here, these functions are fine to be called like that
4731-
* because all just return a pointer and do not actually send/receive
4732-
* any data to/from the database.
4733-
*/
4734-
initPQExpBuffer(&connstr);
4735-
appendPQExpBuffer(&connstr, "dbname=");
4736-
appendConnStrVal(&connstr, PQdb(AH->connection));
4737-
pghost = PQhost(AH->connection);
4738-
pgport = PQport(AH->connection);
4739-
username = PQuser(AH->connection);
4740-
4741-
/* this also sets clone->connection */
4742-
ConnectDatabase((Archive *) clone, connstr.data,
4743-
pghost, pgport, username, TRI_NO);
4744-
4745-
termPQExpBuffer(&connstr);
4746-
/* setupDumpWorker will fix up connection state */
4747-
}
4693+
/* in write case, setupDumpWorker will fix up connection state */
47484694

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

‎src/bin/pg_dump/pg_backup_archiver.h

Copy file name to clipboardExpand all lines: src/bin/pg_dump/pg_backup_archiver.h
+1-2Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.