Skip to content

Commit 0a1485f

Browse files
tglsfdctglsfdc
committed
Clean up some minor inefficiencies in parallel dump/restore.
Parallel dump did a totally pointless query to find out the name of each table to be dumped, which it already knows. Parallel restore runs issued lots of redundant SET commands because _doSetFixedOutputState() was invoked once per TOC item rather than just once at connection start. While the extra queries are insignificant if you're dumping or restoring large tables, it still seems worth getting rid of them. Also, give the responsibility for selecting the right client_encoding for a parallel dump worker to setup_connection() where it naturally belongs, instead of having ad-hoc code for that in CloneArchive(). And fix some minor bugs like use of strdup() where pg_strdup() would be safer. Back-patch to 9.3, mostly to keep the branches in sync in an area that we're still finding bugs in. Discussion: <5086.1464793073@sss.pgh.pa.us>
1 parent a84cad2 commit 0a1485f

File tree

3 files changed

+29
-47
lines changed

3 files changed

+29
-47
lines changed

‎src/bin/pg_dump/parallel.c

Lines changed: 1 addition & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -804,7 +804,6 @@ IsEveryWorkerIdle(ParallelState *pstate)
804804
static void
805805
lockTableForWorker(ArchiveHandle *AH, TocEntry *te)
806806
{
807-
Archive *AHX = (Archive *) AH;
808807
const char *qualId;
809808
PQExpBuffer query;
810809
PGresult *res;
@@ -815,33 +814,10 @@ lockTableForWorker(ArchiveHandle *AH, TocEntry *te)
815814

816815
query = createPQExpBuffer();
817816

818-
/*
819-
* XXX this is an unbelievably expensive substitute for knowing how to dig
820-
* a table name out of a TocEntry.
821-
*/
822-
appendPQExpBuffer(query,
823-
"SELECT pg_namespace.nspname,"
824-
" pg_class.relname "
825-
" FROM pg_class "
826-
" JOIN pg_namespace on pg_namespace.oid = relnamespace "
827-
" WHERE pg_class.oid = %u", te->catalogId.oid);
828-
829-
res = PQexec(AH->connection, query->data);
830-
831-
if (!res || PQresultStatus(res) != PGRES_TUPLES_OK)
832-
exit_horribly(modulename,
833-
"could not get relation name for OID %u: %s\n",
834-
te->catalogId.oid, PQerrorMessage(AH->connection));
835-
836-
resetPQExpBuffer(query);
837-
838-
qualId = fmtQualifiedId(AHX->remoteVersion,
839-
PQgetvalue(res, 0, 0),
840-
PQgetvalue(res, 0, 1));
817+
qualId = fmtQualifiedId(AH->public.remoteVersion, te->namespace, te->tag);
841818

842819
appendPQExpBuffer(query, "LOCK TABLE %s IN ACCESS SHARE MODE NOWAIT",
843820
qualId);
844-
PQclear(res);
845821

846822
res = PQexec(AH->connection, query->data);
847823

‎src/bin/pg_dump/pg_backup_archiver.c

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3647,6 +3647,7 @@ restore_toc_entries_postfork(ArchiveHandle *AH, TocEntry *pending_list)
36473647
ropt->pghost, ropt->pgport, ropt->username,
36483648
ropt->promptPassword);
36493649

3650+
/* re-establish fixed state */
36503651
_doSetFixedOutputState(AH);
36513652

36523653
/*
@@ -3826,10 +3827,9 @@ parallel_restore(ParallelArgs *args)
38263827
RestoreOptions *ropt = AH->ropt;
38273828
int status;
38283829

3829-
_doSetFixedOutputState(AH);
3830-
38313830
Assert(AH->connection != NULL);
38323831

3832+
/* Count only errors associated with this TOC entry */
38333833
AH->public.n_errors = 0;
38343834

38353835
/* Restore the TOC item */
@@ -4198,18 +4198,21 @@ CloneArchive(ArchiveHandle *AH)
41984198
RestoreOptions *ropt = AH->ropt;
41994199

42004200
Assert(AH->connection == NULL);
4201+
42014202
/* this also sets clone->connection */
42024203
ConnectDatabase((Archive *) clone, ropt->dbname,
42034204
ropt->pghost, ropt->pgport, ropt->username,
42044205
ropt->promptPassword);
4206+
4207+
/* re-establish fixed state */
4208+
_doSetFixedOutputState(clone);
42054209
}
42064210
else
42074211
{
42084212
char *dbname;
42094213
char *pghost;
42104214
char *pgport;
42114215
char *username;
4212-
const char *encname;
42134216

42144217
Assert(AH->connection != NULL);
42154218

@@ -4223,18 +4226,11 @@ CloneArchive(ArchiveHandle *AH)
42234226
pghost = PQhost(AH->connection);
42244227
pgport = PQport(AH->connection);
42254228
username = PQuser(AH->connection);
4226-
encname = pg_encoding_to_char(AH->public.encoding);
42274229

42284230
/* this also sets clone->connection */
42294231
ConnectDatabase((Archive *) clone, dbname, pghost, pgport, username, TRI_NO);
42304232

4231-
/*
4232-
* Set the same encoding, whatever we set here is what we got from
4233-
* pg_encoding_to_char(), so we really shouldn't run into an error
4234-
* setting that very same value. Also see the comment in
4235-
* SetupConnection().
4236-
*/
4237-
PQsetClientEncoding(clone->connection, encname);
4233+
/* setupDumpWorker will fix up connection state */
42384234
}
42394235

42404236
/* Let the format-specific code have a chance too */

‎src/bin/pg_dump/pg_dump.c

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -940,10 +940,7 @@ setup_connection(Archive *AH, const char *dumpencoding, char *use_role)
940940
const char *std_strings;
941941

942942
/*
943-
* Set the client encoding if requested. If dumpencoding == NULL then
944-
* either it hasn't been requested or we're a cloned connection and then
945-
* this has already been set in CloneArchive according to the original
946-
* connection encoding.
943+
* Set the client encoding if requested.
947944
*/
948945
if (dumpencoding)
949946
{
@@ -961,7 +958,11 @@ setup_connection(Archive *AH, const char *dumpencoding, char *use_role)
961958
std_strings = PQparameterStatus(conn, "standard_conforming_strings");
962959
AH->std_strings = (std_strings && strcmp(std_strings, "on") == 0);
963960

964-
/* Set the role if requested */
961+
/*
962+
* Set the role if requested. In a parallel dump worker, we'll be passed
963+
* use_role == NULL, but AH->use_role is already set (if user specified it
964+
* originally) and we should use that.
965+
*/
965966
if (!use_role && AH->use_role)
966967
use_role = AH->use_role;
967968

@@ -974,9 +975,9 @@ setup_connection(Archive *AH, const char *dumpencoding, char *use_role)
974975
ExecuteSqlStatement(AH, query->data);
975976
destroyPQExpBuffer(query);
976977

977-
/* save this for later use on parallel connections */
978+
/* save it for possible later use by parallel workers */
978979
if (!AH->use_role)
979-
AH->use_role = strdup(use_role);
980+
AH->use_role = pg_strdup(use_role);
980981
}
981982

982983
/* Set the datestyle to ISO to ensure the dump's portability */
@@ -1068,21 +1069,30 @@ setup_connection(Archive *AH, const char *dumpencoding, char *use_role)
10681069
}
10691070
}
10701071

1072+
/* Set up connection for a parallel worker process */
10711073
static void
1072-
setupDumpWorker(Archive *AHX, RestoreOptions *ropt)
1074+
setupDumpWorker(Archive *AH, RestoreOptions *ropt)
10731075
{
1074-
setup_connection(AHX, NULL, NULL);
1076+
/*
1077+
* We want to re-select all the same values the master connection is
1078+
* using. We'll have inherited directly-usable values in
1079+
* AH->sync_snapshot_id and AH->use_role, but we need to translate the
1080+
* inherited encoding value back to a string to pass to setup_connection.
1081+
*/
1082+
setup_connection(AH,
1083+
pg_encoding_to_char(AH->encoding),
1084+
NULL);
10751085
}
10761086

10771087
static char *
10781088
get_synchronized_snapshot(Archive *fout)
10791089
{
1080-
char *query = "SELECT pg_export_snapshot()";
1090+
char *query = "SELECT pg_catalog.pg_export_snapshot()";
10811091
char *result;
10821092
PGresult *res;
10831093

10841094
res = ExecuteSqlQueryForSingleRow(fout, query);
1085-
result = strdup(PQgetvalue(res, 0, 0));
1095+
result = pg_strdup(PQgetvalue(res, 0, 0));
10861096
PQclear(res);
10871097

10881098
return result;

0 commit comments

Comments
 (0)