Skip to content

Commit e3fcbbd

Browse files
tglsfdctglsfdc
committed
Postpone calls of unsafe server-side functions in pg_dump.
Avoid calling pg_get_partkeydef(), pg_get_expr(relpartbound), and regtypeout until we have lock on the relevant tables. The existing coding is at serious risk of failure if there are any concurrent DROP TABLE commands going on --- including drops of other sessions' temp tables. Arguably this is a bug fix that should be back-patched, but it's moderately invasive and we've not had all that many complaints about such failures. Let's just put it in HEAD for now. Discussion: https://postgr.es/m/2273648.1634764485@sss.pgh.pa.us Discussion: https://postgr.es/m/7d7eb6128f40401d81b3b7a898b6b4de@W2012-02.nidsa.loc
1 parent 0c9d844 commit e3fcbbd

File tree

2 files changed

+64
-33
lines changed

2 files changed

+64
-33
lines changed

‎src/bin/pg_dump/pg_dump.c

Lines changed: 63 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -6272,20 +6272,23 @@ getTables(Archive *fout, int *numTables)
62726272
int i_is_identity_sequence;
62736273
int i_relacl;
62746274
int i_acldefault;
6275-
int i_partkeydef;
62766275
int i_ispartition;
6277-
int i_partbound;
62786276

62796277
/*
62806278
* Find all the tables and table-like objects.
62816279
*
6280+
* We must fetch all tables in this phase because otherwise we cannot
6281+
* correctly identify inherited columns, owned sequences, etc.
6282+
*
62826283
* We include system catalogs, so that we can work if a user table is
62836284
* defined to inherit from a system catalog (pretty weird, but...)
62846285
*
62856286
* Note: in this phase we should collect only a minimal amount of
62866287
* information about each table, basically just enough to decide if it is
6287-
* interesting. We must fetch all tables in this phase because otherwise
6288-
* we cannot correctly identify inherited columns, owned sequences, etc.
6288+
* interesting. In particular, since we do not yet have lock on any user
6289+
* table, we MUST NOT invoke any server-side data collection functions
6290+
* (for instance, pg_get_partkeydef()). Those are likely to fail or give
6291+
* wrong answers if any concurrent DDL is happening.
62896292
*/
62906293

62916294
appendPQExpBuffer(query,
@@ -6379,10 +6382,10 @@ getTables(Archive *fout, int *numTables)
63796382

63806383
if (fout->remoteVersion >= 90000)
63816384
appendPQExpBufferStr(query,
6382-
"CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype, ");
6385+
"c.reloftype, ");
63836386
else
63846387
appendPQExpBufferStr(query,
6385-
"NULL AS reloftype, ");
6388+
"0 AS reloftype, ");
63866389

63876390
if (fout->remoteVersion >= 90100)
63886391
appendPQExpBufferStr(query,
@@ -6423,14 +6426,10 @@ getTables(Archive *fout, int *numTables)
64236426

64246427
if (fout->remoteVersion >= 100000)
64256428
appendPQExpBufferStr(query,
6426-
"pg_get_partkeydef(c.oid) AS partkeydef, "
6427-
"c.relispartition AS ispartition, "
6428-
"pg_get_expr(c.relpartbound, c.oid) AS partbound ");
6429+
"c.relispartition AS ispartition ");
64296430
else
64306431
appendPQExpBufferStr(query,
6431-
"NULL AS partkeydef, "
6432-
"false AS ispartition, "
6433-
"NULL AS partbound ");
6432+
"false AS ispartition ");
64346433

64356434
/*
64366435
* Left join to pg_depend to pick up dependency info linking sequences to
@@ -6539,9 +6538,7 @@ getTables(Archive *fout, int *numTables)
65396538
i_is_identity_sequence = PQfnumber(res, "is_identity_sequence");
65406539
i_relacl = PQfnumber(res, "relacl");
65416540
i_acldefault = PQfnumber(res, "acldefault");
6542-
i_partkeydef = PQfnumber(res, "partkeydef");
65436541
i_ispartition = PQfnumber(res, "ispartition");
6544-
i_partbound = PQfnumber(res, "partbound");
65456542

65466543
if (dopt->lockWaitTimeout)
65476544
{
@@ -6607,19 +6604,14 @@ getTables(Archive *fout, int *numTables)
66076604
else
66086605
tblinfo[i].checkoption = pg_strdup(PQgetvalue(res, i, i_checkoption));
66096606
tblinfo[i].toast_reloptions = pg_strdup(PQgetvalue(res, i, i_toastreloptions));
6610-
if (PQgetisnull(res, i, i_reloftype))
6611-
tblinfo[i].reloftype = NULL;
6612-
else
6613-
tblinfo[i].reloftype = pg_strdup(PQgetvalue(res, i, i_reloftype));
6607+
tblinfo[i].reloftype = atooid(PQgetvalue(res, i, i_reloftype));
66146608
tblinfo[i].foreign_server = atooid(PQgetvalue(res, i, i_foreignserver));
66156609
if (PQgetisnull(res, i, i_amname))
66166610
tblinfo[i].amname = NULL;
66176611
else
66186612
tblinfo[i].amname = pg_strdup(PQgetvalue(res, i, i_amname));
66196613
tblinfo[i].is_identity_sequence = (strcmp(PQgetvalue(res, i, i_is_identity_sequence), "t") == 0);
6620-
tblinfo[i].partkeydef = pg_strdup(PQgetvalue(res, i, i_partkeydef));
66216614
tblinfo[i].ispartition = (strcmp(PQgetvalue(res, i, i_ispartition), "t") == 0);
6622-
tblinfo[i].partbound = pg_strdup(PQgetvalue(res, i, i_partbound));
66236615

66246616
/* other fields were zeroed above */
66256617

@@ -15651,12 +15643,34 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
1565115643
}
1565215644
else
1565315645
{
15646+
char *partkeydef = NULL;
1565415647
char *ftoptions = NULL;
1565515648
char *srvname = NULL;
1565615649
char *foreign = "";
1565715650

15651+
/*
15652+
* Set reltypename, and collect any relkind-specific data that we
15653+
* didn't fetch during getTables().
15654+
*/
1565815655
switch (tbinfo->relkind)
1565915656
{
15657+
case RELKIND_PARTITIONED_TABLE:
15658+
{
15659+
PQExpBuffer query = createPQExpBuffer();
15660+
PGresult *res;
15661+
15662+
reltypename = "TABLE";
15663+
15664+
/* retrieve partition key definition */
15665+
appendPQExpBuffer(query,
15666+
"SELECT pg_get_partkeydef('%u')",
15667+
tbinfo->dobj.catId.oid);
15668+
res = ExecuteSqlQueryForSingleRow(fout, query->data);
15669+
partkeydef = pg_strdup(PQgetvalue(res, 0, 0));
15670+
PQclear(res);
15671+
destroyPQExpBuffer(query);
15672+
break;
15673+
}
1566015674
case RELKIND_FOREIGN_TABLE:
1566115675
{
1566215676
PQExpBuffer query = createPQExpBuffer();
@@ -15696,6 +15710,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
1569615710
break;
1569715711
default:
1569815712
reltypename = "TABLE";
15713+
break;
1569915714
}
1570015715

1570115716
numParents = tbinfo->numParents;
@@ -15717,8 +15732,10 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
1571715732
* Attach to type, if reloftype; except in case of a binary upgrade,
1571815733
* we dump the table normally and attach it to the type afterward.
1571915734
*/
15720-
if (tbinfo->reloftype && !dopt->binary_upgrade)
15721-
appendPQExpBuffer(q, " OF %s", tbinfo->reloftype);
15735+
if (OidIsValid(tbinfo->reloftype) && !dopt->binary_upgrade)
15736+
appendPQExpBuffer(q, " OF %s",
15737+
getFormattedTypeName(fout, tbinfo->reloftype,
15738+
zeroIsError));
1572215739

1572315740
if (tbinfo->relkind != RELKIND_MATVIEW)
1572415741
{
@@ -15756,7 +15773,8 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
1575615773
* Skip column if fully defined by reloftype, except in
1575715774
* binary upgrade
1575815775
*/
15759-
if (tbinfo->reloftype && !print_default && !print_notnull &&
15776+
if (OidIsValid(tbinfo->reloftype) &&
15777+
!print_default && !print_notnull &&
1576015778
!dopt->binary_upgrade)
1576115779
continue;
1576215780

@@ -15789,7 +15807,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
1578915807
* table ('OF type_name'), but in binary-upgrade mode,
1579015808
* print it in that case too.
1579115809
*/
15792-
if (dopt->binary_upgrade || !tbinfo->reloftype)
15810+
if (dopt->binary_upgrade || !OidIsValid(tbinfo->reloftype))
1579315811
{
1579415812
appendPQExpBuffer(q, " %s",
1579515813
tbinfo->atttypnames[j]);
@@ -15852,7 +15870,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
1585215870

1585315871
if (actual_atts)
1585415872
appendPQExpBufferStr(q, "\n)");
15855-
else if (!(tbinfo->reloftype && !dopt->binary_upgrade))
15873+
else if (!(OidIsValid(tbinfo->reloftype) && !dopt->binary_upgrade))
1585615874
{
1585715875
/*
1585815876
* No attributes? we must have a parenthesized attribute list,
@@ -15881,7 +15899,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
1588115899
}
1588215900

1588315901
if (tbinfo->relkind == RELKIND_PARTITIONED_TABLE)
15884-
appendPQExpBuffer(q, "\nPARTITION BY %s", tbinfo->partkeydef);
15902+
appendPQExpBuffer(q, "\nPARTITION BY %s", partkeydef);
1588515903

1588615904
if (tbinfo->relkind == RELKIND_FOREIGN_TABLE)
1588715905
appendPQExpBuffer(q, "\nSERVER %s", fmtId(srvname));
@@ -16064,12 +16082,13 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
1606416082
}
1606516083
}
1606616084

16067-
if (tbinfo->reloftype)
16085+
if (OidIsValid(tbinfo->reloftype))
1606816086
{
1606916087
appendPQExpBufferStr(q, "\n-- For binary upgrade, set up typed tables this way.\n");
1607016088
appendPQExpBuffer(q, "ALTER TABLE ONLY %s OF %s;\n",
1607116089
qualrelname,
16072-
tbinfo->reloftype);
16090+
getFormattedTypeName(fout, tbinfo->reloftype,
16091+
zeroIsError));
1607316092
}
1607416093
}
1607516094

@@ -16242,6 +16261,8 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
1624216261
tbinfo->attfdwoptions[j]);
1624316262
} /* end loop over columns */
1624416263

16264+
if (partkeydef)
16265+
free(partkeydef);
1624516266
if (ftoptions)
1624616267
free(ftoptions);
1624716268
if (srvname)
@@ -16358,6 +16379,8 @@ dumpTableAttach(Archive *fout, const TableAttachInfo *attachinfo)
1635816379
{
1635916380
DumpOptions *dopt = fout->dopt;
1636016381
PQExpBuffer q;
16382+
PGresult *res;
16383+
char *partbound;
1636116384

1636216385
/* Do nothing in data-only dump */
1636316386
if (dopt->dataOnly)
@@ -16368,14 +16391,23 @@ dumpTableAttach(Archive *fout, const TableAttachInfo *attachinfo)
1636816391

1636916392
q = createPQExpBuffer();
1637016393

16371-
/* Perform ALTER TABLE on the parent */
16394+
/* Fetch the partition's partbound */
1637216395
appendPQExpBuffer(q,
16396+
"SELECT pg_get_expr(c.relpartbound, c.oid) "
16397+
"FROM pg_class c "
16398+
"WHERE c.oid = '%u'",
16399+
attachinfo->partitionTbl->dobj.catId.oid);
16400+
res = ExecuteSqlQueryForSingleRow(fout, q->data);
16401+
partbound = PQgetvalue(res, 0, 0);
16402+
16403+
/* Perform ALTER TABLE on the parent */
16404+
printfPQExpBuffer(q,
1637316405
"ALTER TABLE ONLY %s ",
1637416406
fmtQualifiedDumpable(attachinfo->parentTbl));
1637516407
appendPQExpBuffer(q,
1637616408
"ATTACH PARTITION %s %s;\n",
1637716409
fmtQualifiedDumpable(attachinfo->partitionTbl),
16378-
attachinfo->partitionTbl->partbound);
16410+
partbound);
1637916411

1638016412
/*
1638116413
* There is no point in creating a drop query as the drop is done by table
@@ -16392,6 +16424,7 @@ dumpTableAttach(Archive *fout, const TableAttachInfo *attachinfo)
1639216424
.section = SECTION_PRE_DATA,
1639316425
.createStmt = q->data));
1639416426

16427+
PQclear(res);
1639516428
destroyPQExpBuffer(q);
1639616429
}
1639716430

‎src/bin/pg_dump/pg_dump.h

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,7 @@ typedef struct _tableInfo
308308
uint32 toast_minmxid; /* toast table's relminmxid */
309309
int ncheck; /* # of CHECK expressions */
310310
Oid reltype; /* OID of table's composite type, if any */
311-
char *reloftype; /* underlying type for typed table */
311+
Oid reloftype; /* underlying type for typed table */
312312
Oid foreign_server; /* foreign server oid, if applicable */
313313
/* these two are set only if table is a sequence owned by a column: */
314314
Oid owning_tab; /* OID of table owning sequence */
@@ -347,8 +347,6 @@ typedef struct _tableInfo
347347
bool *inhNotNull; /* true if NOT NULL is inherited */
348348
struct _attrDefInfo **attrdefs; /* DEFAULT expressions */
349349
struct _constraintInfo *checkexprs; /* CHECK constraints */
350-
char *partkeydef; /* partition key definition */
351-
char *partbound; /* partition bound definition */
352350
bool needs_override; /* has GENERATED ALWAYS AS IDENTITY */
353351
char *amname; /* relation access method */
354352

0 commit comments

Comments
 (0)