From e3fcbbd623b9ccc16cdbda374654d91a4727d173 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 6 Dec 2021 12:49:49 -0500 Subject: [PATCH] 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 --- src/bin/pg_dump/pg_dump.c | 93 ++++++++++++++++++++++++++------------- src/bin/pg_dump/pg_dump.h | 4 +- 2 files changed, 64 insertions(+), 33 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 75ea57266e..294f8fcbbb 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -6272,20 +6272,23 @@ getTables(Archive *fout, int *numTables) int i_is_identity_sequence; int i_relacl; int i_acldefault; - int i_partkeydef; int i_ispartition; - int i_partbound; /* * Find all the tables and table-like objects. * + * We must fetch all tables in this phase because otherwise we cannot + * correctly identify inherited columns, owned sequences, etc. + * * We include system catalogs, so that we can work if a user table is * defined to inherit from a system catalog (pretty weird, but...) * * Note: in this phase we should collect only a minimal amount of * information about each table, basically just enough to decide if it is - * interesting. We must fetch all tables in this phase because otherwise - * we cannot correctly identify inherited columns, owned sequences, etc. + * interesting. In particular, since we do not yet have lock on any user + * table, we MUST NOT invoke any server-side data collection functions + * (for instance, pg_get_partkeydef()). Those are likely to fail or give + * wrong answers if any concurrent DDL is happening. */ appendPQExpBuffer(query, @@ -6379,10 +6382,10 @@ getTables(Archive *fout, int *numTables) if (fout->remoteVersion >= 90000) appendPQExpBufferStr(query, - "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype, "); + "c.reloftype, "); else appendPQExpBufferStr(query, - "NULL AS reloftype, "); + "0 AS reloftype, "); if (fout->remoteVersion >= 90100) appendPQExpBufferStr(query, @@ -6423,14 +6426,10 @@ getTables(Archive *fout, int *numTables) if (fout->remoteVersion >= 100000) appendPQExpBufferStr(query, - "pg_get_partkeydef(c.oid) AS partkeydef, " - "c.relispartition AS ispartition, " - "pg_get_expr(c.relpartbound, c.oid) AS partbound "); + "c.relispartition AS ispartition "); else appendPQExpBufferStr(query, - "NULL AS partkeydef, " - "false AS ispartition, " - "NULL AS partbound "); + "false AS ispartition "); /* * Left join to pg_depend to pick up dependency info linking sequences to @@ -6539,9 +6538,7 @@ getTables(Archive *fout, int *numTables) i_is_identity_sequence = PQfnumber(res, "is_identity_sequence"); i_relacl = PQfnumber(res, "relacl"); i_acldefault = PQfnumber(res, "acldefault"); - i_partkeydef = PQfnumber(res, "partkeydef"); i_ispartition = PQfnumber(res, "ispartition"); - i_partbound = PQfnumber(res, "partbound"); if (dopt->lockWaitTimeout) { @@ -6607,19 +6604,14 @@ getTables(Archive *fout, int *numTables) else tblinfo[i].checkoption = pg_strdup(PQgetvalue(res, i, i_checkoption)); tblinfo[i].toast_reloptions = pg_strdup(PQgetvalue(res, i, i_toastreloptions)); - if (PQgetisnull(res, i, i_reloftype)) - tblinfo[i].reloftype = NULL; - else - tblinfo[i].reloftype = pg_strdup(PQgetvalue(res, i, i_reloftype)); + tblinfo[i].reloftype = atooid(PQgetvalue(res, i, i_reloftype)); tblinfo[i].foreign_server = atooid(PQgetvalue(res, i, i_foreignserver)); if (PQgetisnull(res, i, i_amname)) tblinfo[i].amname = NULL; else tblinfo[i].amname = pg_strdup(PQgetvalue(res, i, i_amname)); tblinfo[i].is_identity_sequence = (strcmp(PQgetvalue(res, i, i_is_identity_sequence), "t") == 0); - tblinfo[i].partkeydef = pg_strdup(PQgetvalue(res, i, i_partkeydef)); tblinfo[i].ispartition = (strcmp(PQgetvalue(res, i, i_ispartition), "t") == 0); - tblinfo[i].partbound = pg_strdup(PQgetvalue(res, i, i_partbound)); /* other fields were zeroed above */ @@ -15651,12 +15643,34 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo) } else { + char *partkeydef = NULL; char *ftoptions = NULL; char *srvname = NULL; char *foreign = ""; + /* + * Set reltypename, and collect any relkind-specific data that we + * didn't fetch during getTables(). + */ switch (tbinfo->relkind) { + case RELKIND_PARTITIONED_TABLE: + { + PQExpBuffer query = createPQExpBuffer(); + PGresult *res; + + reltypename = "TABLE"; + + /* retrieve partition key definition */ + appendPQExpBuffer(query, + "SELECT pg_get_partkeydef('%u')", + tbinfo->dobj.catId.oid); + res = ExecuteSqlQueryForSingleRow(fout, query->data); + partkeydef = pg_strdup(PQgetvalue(res, 0, 0)); + PQclear(res); + destroyPQExpBuffer(query); + break; + } case RELKIND_FOREIGN_TABLE: { PQExpBuffer query = createPQExpBuffer(); @@ -15696,6 +15710,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo) break; default: reltypename = "TABLE"; + break; } numParents = tbinfo->numParents; @@ -15717,8 +15732,10 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo) * Attach to type, if reloftype; except in case of a binary upgrade, * we dump the table normally and attach it to the type afterward. */ - if (tbinfo->reloftype && !dopt->binary_upgrade) - appendPQExpBuffer(q, " OF %s", tbinfo->reloftype); + if (OidIsValid(tbinfo->reloftype) && !dopt->binary_upgrade) + appendPQExpBuffer(q, " OF %s", + getFormattedTypeName(fout, tbinfo->reloftype, + zeroIsError)); if (tbinfo->relkind != RELKIND_MATVIEW) { @@ -15756,7 +15773,8 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo) * Skip column if fully defined by reloftype, except in * binary upgrade */ - if (tbinfo->reloftype && !print_default && !print_notnull && + if (OidIsValid(tbinfo->reloftype) && + !print_default && !print_notnull && !dopt->binary_upgrade) continue; @@ -15789,7 +15807,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo) * table ('OF type_name'), but in binary-upgrade mode, * print it in that case too. */ - if (dopt->binary_upgrade || !tbinfo->reloftype) + if (dopt->binary_upgrade || !OidIsValid(tbinfo->reloftype)) { appendPQExpBuffer(q, " %s", tbinfo->atttypnames[j]); @@ -15852,7 +15870,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo) if (actual_atts) appendPQExpBufferStr(q, "\n)"); - else if (!(tbinfo->reloftype && !dopt->binary_upgrade)) + else if (!(OidIsValid(tbinfo->reloftype) && !dopt->binary_upgrade)) { /* * No attributes? we must have a parenthesized attribute list, @@ -15881,7 +15899,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo) } if (tbinfo->relkind == RELKIND_PARTITIONED_TABLE) - appendPQExpBuffer(q, "\nPARTITION BY %s", tbinfo->partkeydef); + appendPQExpBuffer(q, "\nPARTITION BY %s", partkeydef); if (tbinfo->relkind == RELKIND_FOREIGN_TABLE) appendPQExpBuffer(q, "\nSERVER %s", fmtId(srvname)); @@ -16064,12 +16082,13 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo) } } - if (tbinfo->reloftype) + if (OidIsValid(tbinfo->reloftype)) { appendPQExpBufferStr(q, "\n-- For binary upgrade, set up typed tables this way.\n"); appendPQExpBuffer(q, "ALTER TABLE ONLY %s OF %s;\n", qualrelname, - tbinfo->reloftype); + getFormattedTypeName(fout, tbinfo->reloftype, + zeroIsError)); } } @@ -16242,6 +16261,8 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo) tbinfo->attfdwoptions[j]); } /* end loop over columns */ + if (partkeydef) + free(partkeydef); if (ftoptions) free(ftoptions); if (srvname) @@ -16358,6 +16379,8 @@ dumpTableAttach(Archive *fout, const TableAttachInfo *attachinfo) { DumpOptions *dopt = fout->dopt; PQExpBuffer q; + PGresult *res; + char *partbound; /* Do nothing in data-only dump */ if (dopt->dataOnly) @@ -16368,14 +16391,23 @@ dumpTableAttach(Archive *fout, const TableAttachInfo *attachinfo) q = createPQExpBuffer(); - /* Perform ALTER TABLE on the parent */ + /* Fetch the partition's partbound */ appendPQExpBuffer(q, + "SELECT pg_get_expr(c.relpartbound, c.oid) " + "FROM pg_class c " + "WHERE c.oid = '%u'", + attachinfo->partitionTbl->dobj.catId.oid); + res = ExecuteSqlQueryForSingleRow(fout, q->data); + partbound = PQgetvalue(res, 0, 0); + + /* Perform ALTER TABLE on the parent */ + printfPQExpBuffer(q, "ALTER TABLE ONLY %s ", fmtQualifiedDumpable(attachinfo->parentTbl)); appendPQExpBuffer(q, "ATTACH PARTITION %s %s;\n", fmtQualifiedDumpable(attachinfo->partitionTbl), - attachinfo->partitionTbl->partbound); + partbound); /* * 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) .section = SECTION_PRE_DATA, .createStmt = q->data)); + PQclear(res); destroyPQExpBuffer(q); } diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h index b21b91f8bc..291b880a97 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -308,7 +308,7 @@ typedef struct _tableInfo uint32 toast_minmxid; /* toast table's relminmxid */ int ncheck; /* # of CHECK expressions */ Oid reltype; /* OID of table's composite type, if any */ - char *reloftype; /* underlying type for typed table */ + Oid reloftype; /* underlying type for typed table */ Oid foreign_server; /* foreign server oid, if applicable */ /* these two are set only if table is a sequence owned by a column: */ Oid owning_tab; /* OID of table owning sequence */ @@ -347,8 +347,6 @@ typedef struct _tableInfo bool *inhNotNull; /* true if NOT NULL is inherited */ struct _attrDefInfo **attrdefs; /* DEFAULT expressions */ struct _constraintInfo *checkexprs; /* CHECK constraints */ - char *partkeydef; /* partition key definition */ - char *partbound; /* partition bound definition */ bool needs_override; /* has GENERATED ALWAYS AS IDENTITY */ char *amname; /* relation access method */