From: Tom Lane <tgl@sss.pgh.pa.us>
To: Zsolt Ero <zsolt.ero@gmail.com>
cc: Magnus Hagander <magnus@hagander.net>,
        PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>,
        Daniel Gustafsson <daniel@yesql.se>
Subject: Re: BUG #16604: pg_dump with --jobs breaks SSL connections
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>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Thu, 24 Sep 2020 12:11:36 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1663574.1600968135.0@sss.pgh.pa.us>
Date: Thu, 24 Sep 2020 13:24:43 -0400
Message-ID: <1663666.1600968283@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1663574.1600968135.1@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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="fix-parallel-pg-dump-with-dbname-connstring.patch";
	charset="us-ascii"
Content-ID: <1663574.1600968135.2@sss.pgh.pa.us>
Content-Description: fix-parallel-pg-dump-with-dbname-connstring.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_bac=
kup_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 =3D NewDumpOptions();
 =

 	/* this is the inverse of what's at the end of pg_dump.c's main() */
+	dopt->dbname =3D ropt->dbname ? pg_strdup(ropt->dbname) : NULL;
+	dopt->pgport =3D ropt->pgport ? pg_strdup(ropt->pgport) : NULL;
+	dopt->pghost =3D ropt->pghost ? pg_strdup(ropt->pghost) : NULL;
+	dopt->username =3D ropt->username ? pg_strdup(ropt->username) : NULL;
 	dopt->outputClean =3D ropt->dropSchema;
 	dopt->dataOnly =3D ropt->dataOnly;
 	dopt->schemaOnly =3D ropt->schemaOnly;
@@ -4157,11 +4161,12 @@ restore_toc_entries_postfork(ArchiveHandle *AH, To=
cEntry *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 =3D AH->public.ropt;
 =

 	/* Make a "flat" copy */
 	clone =3D (ArchiveHandle *) pg_malloc(sizeof(ArchiveHandle));
@@ -4823,54 +4829,18 @@ CloneArchive(ArchiveHandle *AH)
 	clone->public.n_errors =3D 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 connecti=
on
+	 * parameters used for the original connection; except that workers shou=
ld
+	 * never re-ask for a password.
 	 */
-	if (AH->mode =3D=3D archModeRead)
-	{
-		RestoreOptions *ropt =3D AH->public.ropt;
-
-		Assert(AH->connection =3D=3D 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 =3D=3D archModeRead)
 		_doSetFixedOutputState(clone);
-	}
-	else
-	{
-		PQExpBufferData connstr;
-		char	   *pghost;
-		char	   *pgport;
-		char	   *username;
-
-		Assert(AH->connection !=3D 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=3D");
-		appendConnStrVal(&connstr, PQdb(AH->connection));
-		pghost =3D PQhost(AH->connection);
-		pgport =3D PQport(AH->connection);
-		username =3D 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 =3D filename;
 =

 	/* if you change this list, see dumpOptionsFromRestoreOptions */
+	ropt->dbname =3D dopt.dbname ? pg_strdup(dopt.dbname) : NULL;
+	ropt->pgport =3D dopt.pgport ? pg_strdup(dopt.pgport) : NULL;
+	ropt->pghost =3D dopt.pghost ? pg_strdup(dopt.pghost) : NULL;
+	ropt->username =3D dopt.username ? pg_strdup(dopt.username) : NULL;
 	ropt->dropSchema =3D dopt.outputClean;
 	ropt->dataOnly =3D dopt.dataOnly;
 	ropt->schemaOnly =3D dopt.schemaOnly;

------- =_aaaaaaaaaa0--