Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 763eec6

Browse files
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 9ee56df commit 763eec6

File tree

3 files changed

+34
-50
lines changed

3 files changed

+34
-50
lines changed

src/bin/pg_dump/parallel.c

+1-25
Original file line numberDiff line numberDiff line change
@@ -802,7 +802,6 @@ IsEveryWorkerIdle(ParallelState *pstate)
802802
static void
803803
lockTableForWorker(ArchiveHandle *AH, TocEntry *te)
804804
{
805-
Archive *AHX = (Archive *) AH;
806805
const char *qualId;
807806
PQExpBuffer query;
808807
PGresult *res;
@@ -813,33 +812,10 @@ lockTableForWorker(ArchiveHandle *AH, TocEntry *te)
813812

814813
query = createPQExpBuffer();
815814

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

840817
appendPQExpBuffer(query, "LOCK TABLE %s IN ACCESS SHARE MODE NOWAIT",
841818
qualId);
842-
PQclear(res);
843819

844820
res = PQexec(AH->connection, query->data);
845821

src/bin/pg_dump/pg_backup_archiver.c

+7-11
Original file line numberDiff line numberDiff line change
@@ -3893,6 +3893,7 @@ restore_toc_entries_postfork(ArchiveHandle *AH, TocEntry *pending_list)
38933893
ropt->pghost, ropt->pgport, ropt->username,
38943894
ropt->promptPassword);
38953895

3896+
/* re-establish fixed state */
38963897
_doSetFixedOutputState(AH);
38973898

38983899
/*
@@ -4071,10 +4072,9 @@ parallel_restore(ParallelArgs *args)
40714072
TocEntry *te = args->te;
40724073
int status;
40734074

4074-
_doSetFixedOutputState(AH);
4075-
40764075
Assert(AH->connection != NULL);
40774076

4077+
/* Count only errors associated with this TOC entry */
40784078
AH->public.n_errors = 0;
40794079

40804080
/* Restore the TOC item */
@@ -4443,18 +4443,21 @@ CloneArchive(ArchiveHandle *AH)
44434443
RestoreOptions *ropt = AH->public.ropt;
44444444

44454445
Assert(AH->connection == NULL);
4446+
44464447
/* this also sets clone->connection */
44474448
ConnectDatabase((Archive *) clone, ropt->dbname,
44484449
ropt->pghost, ropt->pgport, ropt->username,
44494450
ropt->promptPassword);
4451+
4452+
/* re-establish fixed state */
4453+
_doSetFixedOutputState(clone);
44504454
}
44514455
else
44524456
{
44534457
char *dbname;
44544458
char *pghost;
44554459
char *pgport;
44564460
char *username;
4457-
const char *encname;
44584461

44594462
Assert(AH->connection != NULL);
44604463

@@ -4468,18 +4471,11 @@ CloneArchive(ArchiveHandle *AH)
44684471
pghost = PQhost(AH->connection);
44694472
pgport = PQport(AH->connection);
44704473
username = PQuser(AH->connection);
4471-
encname = pg_encoding_to_char(AH->public.encoding);
44724474

44734475
/* this also sets clone->connection */
44744476
ConnectDatabase((Archive *) clone, dbname, pghost, pgport, username, TRI_NO);
44754477

4476-
/*
4477-
* Set the same encoding, whatever we set here is what we got from
4478-
* pg_encoding_to_char(), so we really shouldn't run into an error
4479-
* setting that very same value. Also see the comment in
4480-
* SetupConnection().
4481-
*/
4482-
PQsetClientEncoding(clone->connection, encname);
4478+
/* setupDumpWorker will fix up connection state */
44834479
}
44844480

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

src/bin/pg_dump/pg_dump.c

+26-14
Original file line numberDiff line numberDiff line change
@@ -952,10 +952,7 @@ setup_connection(Archive *AH, const char *dumpencoding,
952952
const char *std_strings;
953953

954954
/*
955-
* Set the client encoding if requested. If dumpencoding == NULL then
956-
* either it hasn't been requested or we're a cloned connection and then
957-
* this has already been set in CloneArchive according to the original
958-
* connection encoding.
955+
* Set the client encoding if requested.
959956
*/
960957
if (dumpencoding)
961958
{
@@ -973,7 +970,11 @@ setup_connection(Archive *AH, const char *dumpencoding,
973970
std_strings = PQparameterStatus(conn, "standard_conforming_strings");
974971
AH->std_strings = (std_strings && strcmp(std_strings, "on") == 0);
975972

976-
/* Set the role if requested */
973+
/*
974+
* Set the role if requested. In a parallel dump worker, we'll be passed
975+
* use_role == NULL, but AH->use_role is already set (if user specified it
976+
* originally) and we should use that.
977+
*/
977978
if (!use_role && AH->use_role)
978979
use_role = AH->use_role;
979980

@@ -986,9 +987,9 @@ setup_connection(Archive *AH, const char *dumpencoding,
986987
ExecuteSqlStatement(AH, query->data);
987988
destroyPQExpBuffer(query);
988989

989-
/* save this for later use on parallel connections */
990+
/* save it for possible later use by parallel workers */
990991
if (!AH->use_role)
991-
AH->use_role = strdup(use_role);
992+
AH->use_role = pg_strdup(use_role);
992993
}
993994

994995
/* Set the datestyle to ISO to ensure the dump's portability */
@@ -1074,11 +1075,12 @@ setup_connection(Archive *AH, const char *dumpencoding,
10741075
"SET TRANSACTION ISOLATION LEVEL SERIALIZABLE");
10751076

10761077
/*
1077-
* define an export snapshot, either chosen by user or needed for parallel
1078-
* dump.
1078+
* If user specified a snapshot to use, select that. In a parallel dump
1079+
* worker, we'll be passed dumpsnapshot == NULL, but AH->sync_snapshot_id
1080+
* is already set (if the server can handle it) and we should use that.
10791081
*/
10801082
if (dumpsnapshot)
1081-
AH->sync_snapshot_id = strdup(dumpsnapshot);
1083+
AH->sync_snapshot_id = pg_strdup(dumpsnapshot);
10821084

10831085
if (AH->sync_snapshot_id)
10841086
{
@@ -1104,21 +1106,31 @@ setup_connection(Archive *AH, const char *dumpencoding,
11041106
}
11051107
}
11061108

1109+
/* Set up connection for a parallel worker process */
11071110
static void
1108-
setupDumpWorker(Archive *AHX)
1111+
setupDumpWorker(Archive *AH)
11091112
{
1110-
setup_connection(AHX, NULL, NULL, NULL);
1113+
/*
1114+
* We want to re-select all the same values the master connection is
1115+
* using. We'll have inherited directly-usable values in
1116+
* AH->sync_snapshot_id and AH->use_role, but we need to translate the
1117+
* inherited encoding value back to a string to pass to setup_connection.
1118+
*/
1119+
setup_connection(AH,
1120+
pg_encoding_to_char(AH->encoding),
1121+
NULL,
1122+
NULL);
11111123
}
11121124

11131125
static char *
11141126
get_synchronized_snapshot(Archive *fout)
11151127
{
1116-
char *query = "SELECT pg_export_snapshot()";
1128+
char *query = "SELECT pg_catalog.pg_export_snapshot()";
11171129
char *result;
11181130
PGresult *res;
11191131

11201132
res = ExecuteSqlQueryForSingleRow(fout, query);
1121-
result = strdup(PQgetvalue(res, 0, 0));
1133+
result = pg_strdup(PQgetvalue(res, 0, 0));
11221134
PQclear(res);
11231135

11241136
return result;

0 commit comments

Comments
 (0)