Re: BUG #16604: pg_dump with --jobs breaks SSL connections - Mailing list pgsql-bugs

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

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 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.c
index 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;