From: Tom Lane <tgl@sss.pgh.pa.us>
To: Zsolt Ero <zsolt.ero@gmail.com>
Cc: Magnus Hagander <magnus@hagander.net>
Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Cc: Daniel Gustafsson <daniel@yesql.se>
Subject: Re: BUG #16604: pg_dump with --jobs breaks SSL connections
Date: Thu, 24 Sep 2020 13:24:43 -0400
Message-ID: <1663666.1600968283@sss.pgh.pa.us> (raw)
In-Reply-To: <1638874.1600963896@sss.pgh.pa.us>
References: <16604-933f4b8791227b15@postgresql.org>
<61B1FFD2-EA7B-4C62-92A1-44C5459189C0@yesql.se>
<CABUevEwwr=oCNQnu2RUTLhKwyKxRh3C__ckum=Xgm+acgUEJ_Q@mail.gmail.com>
<CAKw-smBKagNkWRZvTujeKgx6j5ZkjazDvDPFq9bZWbBC7YXqbg@mail.gmail.com>
<1638874.1600963896@sss.pgh.pa.us>
I wrote:
> I reproduced this locally, and the problem seems to be that
> CloneArchive() is doing a far-less-than-half-baked job of
> reconstructing the original connection parameters. ...
> so parallel pg_dump is basically guaranteed to fail in any
> case with even slightly unusual connection parameters.
> Not sure why we should be trying to do it like that at
> all; it'd be better if the original command-line parameters
> got passed down in all cases. Looking at that now.
The attached patch seems to fix it, and also takes care of a nasty
habit that parallel pg_restore had of issuing repeated password
prompts if you're fool enough to specify -W. IMO it's only
sensible to apply that option on the first connection attempt.
Note, however, that AFAICS parallel pg_restore was okay on duplicating
the original connection parameters otherwise (the prompting issue
was because it went too far with that...). So I fail to confirm
the OP's claim that pg_restore had the same bug as parallel pg_dump.
If there really is an issue there, we'll need a test case that
demonstrates it.
regards, tom lane
Attachments:
[text/x-diff] fix-parallel-pg-dump-with-dbname-connstring.patch (4.2K, ../1663666.1600968283@sss.pgh.pa.us/2-fix-parallel-pg-dump-with-dbname-connstring.patch)
download | inline diff:diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.cindex 3567e9f365..bbe1774a3e 100644--- a/src/bin/pg_dump/pg_backup_archiver.c+++ b/src/bin/pg_dump/pg_backup_archiver.c@@ -178,6 +178,10 @@ dumpOptionsFromRestoreOptions(RestoreOptions *ropt)
DumpOptions *dopt = NewDumpOptions();
/* this is the inverse of what's at the end of pg_dump.c's main() */
+ dopt->dbname = ropt->dbname ? pg_strdup(ropt->dbname) : NULL;+ dopt->pgport = ropt->pgport ? pg_strdup(ropt->pgport) : NULL;+ dopt->pghost = ropt->pghost ? pg_strdup(ropt->pghost) : NULL;+ dopt->username = ropt->username ? pg_strdup(ropt->username) : NULL;
dopt->outputClean = ropt->dropSchema;
dopt->dataOnly = ropt->dataOnly;
dopt->schemaOnly = ropt->schemaOnly;
@@ -4157,11 +4161,12 @@ restore_toc_entries_postfork(ArchiveHandle *AH, TocEntry *pending_list)
pg_log_debug("entering restore_toc_entries_postfork");
/*
- * Now reconnect the single parent connection.+ * Now reconnect the single parent connection. Don't re-ask for a+ * password.
*/
ConnectDatabase((Archive *) AH, ropt->dbname,
ropt->pghost, ropt->pgport, ropt->username,
- ropt->promptPassword);+ TRI_NO);
/* re-establish fixed state */
_doSetFixedOutputState(AH);
@@ -4800,6 +4805,7 @@ ArchiveHandle *
CloneArchive(ArchiveHandle *AH)
{
ArchiveHandle *clone;
+ RestoreOptions *ropt = AH->public.ropt;
/* Make a "flat" copy */
clone = (ArchiveHandle *) pg_malloc(sizeof(ArchiveHandle));
@@ -4823,54 +4829,18 @@ 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; except that workers should+ * never re-ask for a password.
*/
- 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);+ ConnectDatabase((Archive *) clone, ropt->dbname,+ ropt->pghost, ropt->pgport, ropt->username,+ TRI_NO);- /* re-establish fixed state */+ /* re-establish fixed state */+ if (AH->mode == archModeRead)
_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);- appendPQExpBufferStr(&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);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.cindex f021bb72f4..7676b93a10 100644--- a/src/bin/pg_dump/pg_dump.c+++ b/src/bin/pg_dump/pg_dump.c@@ -918,6 +918,10 @@ main(int argc, char **argv)
ropt->filename = filename;
/* if you change this list, see dumpOptionsFromRestoreOptions */
+ ropt->dbname = dopt.dbname ? pg_strdup(dopt.dbname) : NULL;+ ropt->pgport = dopt.pgport ? pg_strdup(dopt.pgport) : NULL;+ ropt->pghost = dopt.pghost ? pg_strdup(dopt.pghost) : NULL;+ ropt->username = dopt.username ? pg_strdup(dopt.username) : NULL;
ropt->dropSchema = dopt.outputClean;
ropt->dataOnly = dopt.dataOnly;
ropt->schemaOnly = dopt.schemaOnly;
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, zsolt.ero@gmail.com, magnus@hagander.net, pgsql-bugs@lists.postgresql.org, daniel@yesql.se
Subject: Re: BUG #16604: pg_dump with --jobs breaks SSL connections
In-Reply-To: <1663666.1600968283@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox