From 160a4f62ee7b8a96984f8bef19c90488aa6c8045 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 23 Jan 2018 10:55:08 -0500 Subject: [PATCH] In pg_dump, force reconnection after issuing ALTER DATABASE SET command(s). The folly of not doing this was exposed by the buildfarm: in some cases, the GUC settings applied through ALTER DATABASE SET may be essential to interpreting the reloaded data correctly. Another argument why we can't really get away with the scheme proposed in commit b3f840120 is that it cannot work for parallel restore: even if the parent process manages to hang onto the previous GUC state, worker processes would see the state post-ALTER-DATABASE. (Perhaps we could have dodged that bullet by delaying DATABASE PROPERTIES restoration to the end of the run, but that does nothing for the data semantics problem.) This leaves us with no solution for the default_transaction_read_only issue that commit 4bd371f6f intended to work around, other than "you gotta remove such settings before dumping/upgrading". However, in view of the fact that parallel restore broke that hack years ago and no one has noticed, it's fair to question how many people care. I'm unexcited about adding a large dollop of new complexity to handle that corner case. This would be a one-liner fix, except it turns out that ReconnectToServer tries to optimize away "redundant" reconnections. While that may have been valuable when coded, a quick survey of current callers shows that there are no cases where that's actually useful, so just remove that check. While at it, remove the function's useless return value. Discussion: https://postgr.es/m/12453.1516655001@sss.pgh.pa.us --- src/bin/pg_dump/pg_backup_archiver.c | 9 +++++++-- src/bin/pg_dump/pg_backup_archiver.h | 2 +- src/bin/pg_dump/pg_backup_db.c | 15 ++------------- src/bin/pg_dump/pg_dump.c | 12 ++++++------ 4 files changed, 16 insertions(+), 22 deletions(-) diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index ab009e6fe3..f55aa36c49 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -833,8 +833,13 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel) } } - /* If we created a DB, connect to it... */ - if (strcmp(te->desc, "DATABASE") == 0) + /* + * If we created a DB, connect to it. Also, if we changed DB + * properties, reconnect to ensure that relevant GUC settings are + * applied to our session. + */ + if (strcmp(te->desc, "DATABASE") == 0 || + strcmp(te->desc, "DATABASE PROPERTIES") == 0) { PQExpBufferData connstr; diff --git a/src/bin/pg_dump/pg_backup_archiver.h b/src/bin/pg_dump/pg_backup_archiver.h index fd0d01b506..becfee6e81 100644 --- a/src/bin/pg_dump/pg_backup_archiver.h +++ b/src/bin/pg_dump/pg_backup_archiver.h @@ -448,7 +448,7 @@ extern void InitArchiveFmt_Tar(ArchiveHandle *AH); extern bool isValidTarHeader(char *header); -extern int ReconnectToServer(ArchiveHandle *AH, const char *dbname, const char *newUser); +extern void ReconnectToServer(ArchiveHandle *AH, const char *dbname, const char *newUser); extern void DropBlobIfExists(ArchiveHandle *AH, Oid oid); void ahwrite(const void *ptr, size_t size, size_t nmemb, ArchiveHandle *AH); diff --git a/src/bin/pg_dump/pg_backup_db.c b/src/bin/pg_dump/pg_backup_db.c index 216853d627..3b7dd24151 100644 --- a/src/bin/pg_dump/pg_backup_db.c +++ b/src/bin/pg_dump/pg_backup_db.c @@ -76,13 +76,9 @@ _check_database_version(ArchiveHandle *AH) /* * Reconnect to the server. If dbname is not NULL, use that database, * else the one associated with the archive handle. If username is - * not NULL, use that user name, else the one from the handle. If - * both the database and the user match the existing connection already, - * nothing will be done. - * - * Returns 1 in any case. + * not NULL, use that user name, else the one from the handle. */ -int +void ReconnectToServer(ArchiveHandle *AH, const char *dbname, const char *username) { PGconn *newConn; @@ -99,11 +95,6 @@ ReconnectToServer(ArchiveHandle *AH, const char *dbname, const char *username) else newusername = username; - /* Let's see if the request is already satisfied */ - if (strcmp(newdbname, PQdb(AH->connection)) == 0 && - strcmp(newusername, PQuser(AH->connection)) == 0) - return 1; - newConn = _connectDB(AH, newdbname, newusername); /* Update ArchiveHandle's connCancel before closing old connection */ @@ -111,8 +102,6 @@ ReconnectToServer(ArchiveHandle *AH, const char *dbname, const char *username) PQfinish(AH->connection); AH->connection = newConn; - - return 1; } /* diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 11e1ba04cc..d65ea54a69 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -2819,10 +2819,11 @@ dumpDatabase(Archive *fout) /* * Now construct a DATABASE PROPERTIES archive entry to restore any - * non-default database-level properties. We want to do this after - * reconnecting so that these properties won't apply during the restore - * session. In this way, restoring works even if there is, say, an ALTER - * DATABASE SET that turns on default_transaction_read_only. + * non-default database-level properties. (The reason this must be + * separate is that we cannot put any additional commands into the TOC + * entry that has CREATE DATABASE. pg_restore would execute such a group + * in an implicit transaction block, and the backend won't allow CREATE + * DATABASE in that context.) */ resetPQExpBuffer(creaQry); resetPQExpBuffer(delQry); @@ -2854,8 +2855,7 @@ dumpDatabase(Archive *fout) /* * We stick this binary-upgrade query into the DATABASE PROPERTIES archive - * entry, too. It can't go into the DATABASE entry because that would - * result in an implicit transaction block around the CREATE DATABASE. + * entry, too, for lack of a better place. */ if (dopt->binary_upgrade) { -- 2.40.0