database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-08-27 07:42:10 +03:00
Code Activity

Fix handling of -d "connection string" in pg_dump/pg_restore.

Browse Source 
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
This commit is contained in:
Tom Lane
2020-09-24 18:19:39 -04:00
parent 23d8b3577d
commit 56b46d3a1a
6 changed files with 130 additions and 295 deletions
Download Patch File Download Diff File Expand all files Collapse all files

96
src/bin/pg_dump/pg_backup_archiver.c
View File

@@ -135,6 +135,7 @@ InitDumpOptions(DumpOptions *opts)
memset(opts, 0, sizeof(DumpOptions));
/* set any fields that shouldn't default to zeroes */
opts->include_everything = true;
opts->cparams.promptPassword = TRI_DEFAULT;
opts->dumpSections = DUMP_UNSECTIONED;
}
@@ -148,6 +149,11 @@ dumpOptionsFromRestoreOptions(RestoreOptions *ropt)
DumpOptions *dopt = NewDumpOptions();
/* this is the inverse of what's at the end of pg_dump.c's main() */
dopt->cparams.dbname = ropt->cparams.dbname ? pg_strdup(ropt->cparams.dbname) : NULL;
dopt->cparams.pgport = ropt->cparams.pgport ? pg_strdup(ropt->cparams.pgport) : NULL;
dopt->cparams.pghost = ropt->cparams.pghost ? pg_strdup(ropt->cparams.pghost) : NULL;
dopt->cparams.username = ropt->cparams.username ? pg_strdup(ropt->cparams.username) : NULL;
dopt->cparams.promptPassword = ropt->cparams.promptPassword;
dopt->outputClean = ropt->dropSchema;
dopt->dataOnly = ropt->dataOnly;
dopt->schemaOnly = ropt->schemaOnly;
@@ -383,9 +389,7 @@ RestoreArchive(Archive *AHX)
AHX->minRemoteVersion = 0;
AHX->maxRemoteVersion = 999999;
ConnectDatabase(AHX, ropt->dbname,
ropt->pghost, ropt->pgport, ropt->username,
ropt->promptPassword);
ConnectDatabase(AHX, &ropt->cparams, false);
/*
* 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)
/* If we created a DB, connect to it... */
if (strcmp(te->desc, "DATABASE") == 0)
{
PQExpBufferData connstr;
initPQExpBuffer(&connstr);
appendPQExpBufferStr(&connstr, "dbname=");
appendConnStrVal(&connstr, te->tag);
/* Abandon struct, but keep its buffer until process exit. */
ahlog(AH, 1, "connecting to new database \"%s\"\n", te->tag);
_reconnectToDB(AH, te->tag);
ropt->dbname = connstr.data;
}
}
@@ -948,7 +944,7 @@ NewRestoreOptions(void)
/* set any fields that shouldn't default to zeroes */
opts->format = archUnknown;
opts->promptPassword = TRI_DEFAULT;
opts->cparams.promptPassword = TRI_DEFAULT;
opts->dumpSections = DUMP_UNSECTIONED;
return opts;
@@ -2376,8 +2372,6 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt,
else
AH->format = fmt;
AH->promptPassword = TRI_DEFAULT;
switch (AH->format)
{
case archCustom:
@@ -3072,27 +3066,20 @@ _doSetWithOids(ArchiveHandle *AH, const bool withOids)
* If we're currently restoring right into a database, this will
* actually establish a connection. Otherwise it puts a \connect into
* the script output.
*
* NULL dbname implies reconnecting to the current DB (pretty useless).
*/
static void
_reconnectToDB(ArchiveHandle *AH, const char *dbname)
{
if (RestoringToDB(AH))
ReconnectToServer(AH, dbname, NULL);
ReconnectToServer(AH, dbname);
else
{
if (dbname)
{
PQExpBufferData connectbuf;
PQExpBufferData connectbuf;
initPQExpBuffer(&connectbuf);
appendPsqlMetaConnect(&connectbuf, dbname);
ahprintf(AH, "%s\n", connectbuf.data);
termPQExpBuffer(&connectbuf);
}
else
ahprintf(AH, "%s\n", "\\connect -\n");
initPQExpBuffer(&connectbuf);
appendPsqlMetaConnect(&connectbuf, dbname);
ahprintf(AH, "%s\n", connectbuf.data);
termPQExpBuffer(&connectbuf);
}
/*
@@ -4039,9 +4026,7 @@ restore_toc_entries_postfork(ArchiveHandle *AH, TocEntry *pending_list)
/*
* Now reconnect the single parent connection.
*/
ConnectDatabase((Archive *) AH, ropt->dbname,
ropt->pghost, ropt->pgport, ropt->username,
ropt->promptPassword);
ConnectDatabase((Archive *) AH, &ropt->cparams, true);
/* re-establish fixed state */
_doSetFixedOutputState(AH);
@@ -4634,54 +4619,15 @@ CloneArchive(ArchiveHandle *AH)
clone->public.n_errors = 0;
/*
* Connect our new clone object to the database: In parallel restore the
* parent is already disconnected, because we can connect the worker
* processes independently to the database (no snapshot sync required). In
* parallel backup we clone the parent's existing connection.
* Connect our new clone object to the database, using the same connection
* parameters used for the original connection.
*/
ConnectDatabase((Archive *) clone, &clone->public.ropt->cparams, true);
/* re-establish fixed state */
if (AH->mode == archModeRead)
{
RestoreOptions *ropt = AH->public.ropt;
Assert(AH->connection == NULL);
/* this also sets clone->connection */
ConnectDatabase((Archive *) clone, ropt->dbname,
ropt->pghost, ropt->pgport, ropt->username,
ropt->promptPassword);
/* re-establish fixed state */
_doSetFixedOutputState(clone);
}
else
{
PQExpBufferData connstr;
char *pghost;
char *pgport;
char *username;
Assert(AH->connection != NULL);
/*
* Even though we are technically accessing the parent's database
* object here, these functions are fine to be called like that
* because all just return a pointer and do not actually send/receive
* any data to/from the database.
*/
initPQExpBuffer(&connstr);
appendPQExpBuffer(&connstr, "dbname=");
appendConnStrVal(&connstr, PQdb(AH->connection));
pghost = PQhost(AH->connection);
pgport = PQport(AH->connection);
username = PQuser(AH->connection);
/* this also sets clone->connection */
ConnectDatabase((Archive *) clone, connstr.data,
pghost, pgport, username, TRI_NO);
termPQExpBuffer(&connstr);
/* setupDumpWorker will fix up connection state */
}
/* in write case, setupDumpWorker will fix up connection state */
/* Let the format-specific code have a chance too */
(clone->ClonePtr) (clone);