From 55f30e6c7640c80fbb7be90fad668ee4bffa0e97 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 19 Nov 2022 11:40:30 -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. Back-patch of commit e3fcbbd62. That's been in v15/HEAD long enough to have some confidence about it, so now let's fix the problem in older branches. Original patch by me; thanks to Gilles Darold for back-patching legwork. Discussion: https://postgr.es/m/2273648.1634764485@sss.pgh.pa.us Discussion: https://postgr.es/m/7d7eb6128f40401d81b3b7a898b6b4de@W2012-02.nidsa.loc Discussion: https://postgr.es/m/45c93d57-9973-248e-d2df-e02ca9af48d4@darold.net --- src/bin/pg_dump/pg_dump.c | 146 ++++++++++++++++++++------------------ src/bin/pg_dump/pg_dump.h | 4 +- 2 files changed, 79 insertions(+), 71 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index e110257395..8c5c4f0166 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -6322,14 +6322,15 @@ getTables(Archive *fout, int *numTables) int i_foreignserver; int i_is_identity_sequence; int i_changed_acl; - int i_partkeydef; int i_ispartition; - int i_partbound; int i_amname; /* * 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...) * @@ -6343,8 +6344,10 @@ getTables(Archive *fout, int *numTables) * * 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. * * We purposefully ignore toast OIDs for partitioned tables; the reason is * that versions 10 and 11 have them, but 12 does not, so emitting them @@ -6353,9 +6356,7 @@ getTables(Archive *fout, int *numTables) if (fout->remoteVersion >= 90600) { - char *partkeydef = "NULL"; char *ispartition = "false"; - char *partbound = "NULL"; char *relhasoids = "c.relhasoids"; PQExpBuffer acl_subquery = createPQExpBuffer(); @@ -6372,13 +6373,8 @@ getTables(Archive *fout, int *numTables) * Collect the information about any partitioned tables, which were * added in PG10. */ - if (fout->remoteVersion >= 100000) - { - partkeydef = "pg_get_partkeydef(c.oid)"; ispartition = "c.relispartition"; - partbound = "pg_get_expr(c.relpartbound, c.oid)"; - } /* In PG12 upwards WITH OIDS does not exist anymore. */ if (fout->remoteVersion >= 120000) @@ -6419,7 +6415,7 @@ getTables(Archive *fout, int *numTables) "CASE WHEN c.relkind = 'f' THEN " "(SELECT ftserver FROM pg_catalog.pg_foreign_table WHERE ftrelid = c.oid) " "ELSE 0 END AS foreignserver, " - "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype, " + "c.reloftype, " "d.refobjid AS owning_tab, " "d.refobjsubid AS owning_col, " "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, " @@ -6439,9 +6435,7 @@ getTables(Archive *fout, int *numTables) "OR %s IS NOT NULL" "))" "AS changed_acl, " - "%s AS partkeydef, " - "%s AS ispartition, " - "%s AS partbound " + "%s AS ispartition " "FROM pg_class c " "LEFT JOIN pg_depend d ON " "(c.relkind = '%c' AND " @@ -6467,9 +6461,7 @@ getTables(Archive *fout, int *numTables) attracl_subquery->data, attinitacl_subquery->data, attinitracl_subquery->data, - partkeydef, ispartition, - partbound, RELKIND_SEQUENCE, RELKIND_PARTITIONED_TABLE, RELKIND_RELATION, RELKIND_SEQUENCE, @@ -6512,7 +6504,7 @@ getTables(Archive *fout, int *numTables) "CASE WHEN c.relkind = 'f' THEN " "(SELECT ftserver FROM pg_catalog.pg_foreign_table WHERE ftrelid = c.oid) " "ELSE 0 END AS foreignserver, " - "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype, " + "c.reloftype, " "d.refobjid AS owning_tab, " "d.refobjsubid AS owning_col, " "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, " @@ -6521,9 +6513,7 @@ getTables(Archive *fout, int *numTables) "WHEN 'check_option=cascaded' = ANY (c.reloptions) THEN 'CASCADED'::text ELSE NULL END AS checkoption, " "tc.reloptions AS toast_reloptions, " "NULL AS changed_acl, " - "NULL AS partkeydef, " - "false AS ispartition, " - "NULL AS partbound " + "false AS ispartition " "FROM pg_class c " "LEFT JOIN pg_depend d ON " "(c.relkind = '%c' AND " @@ -6565,7 +6555,7 @@ getTables(Archive *fout, int *numTables) "CASE WHEN c.relkind = 'f' THEN " "(SELECT ftserver FROM pg_catalog.pg_foreign_table WHERE ftrelid = c.oid) " "ELSE 0 END AS foreignserver, " - "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype, " + "c.reloftype, " "d.refobjid AS owning_tab, " "d.refobjsubid AS owning_col, " "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, " @@ -6574,9 +6564,7 @@ getTables(Archive *fout, int *numTables) "WHEN 'check_option=cascaded' = ANY (c.reloptions) THEN 'CASCADED'::text ELSE NULL END AS checkoption, " "tc.reloptions AS toast_reloptions, " "NULL AS changed_acl, " - "NULL AS partkeydef, " - "false AS ispartition, " - "NULL AS partbound " + "false AS ispartition " "FROM pg_class c " "LEFT JOIN pg_depend d ON " "(c.relkind = '%c' AND " @@ -6618,7 +6606,7 @@ getTables(Archive *fout, int *numTables) "CASE WHEN c.relkind = 'f' THEN " "(SELECT ftserver FROM pg_catalog.pg_foreign_table WHERE ftrelid = c.oid) " "ELSE 0 END AS foreignserver, " - "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype, " + "c.reloftype, " "d.refobjid AS owning_tab, " "d.refobjsubid AS owning_col, " "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, " @@ -6627,9 +6615,7 @@ getTables(Archive *fout, int *numTables) "WHEN 'check_option=cascaded' = ANY (c.reloptions) THEN 'CASCADED'::text ELSE NULL END AS checkoption, " "tc.reloptions AS toast_reloptions, " "NULL AS changed_acl, " - "NULL AS partkeydef, " - "false AS ispartition, " - "NULL AS partbound " + "false AS ispartition " "FROM pg_class c " "LEFT JOIN pg_depend d ON " "(c.relkind = '%c' AND " @@ -6671,16 +6657,14 @@ getTables(Archive *fout, int *numTables) "CASE WHEN c.relkind = 'f' THEN " "(SELECT ftserver FROM pg_catalog.pg_foreign_table WHERE ftrelid = c.oid) " "ELSE 0 END AS foreignserver, " - "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype, " + "c.reloftype, " "d.refobjid AS owning_tab, " "d.refobjsubid AS owning_col, " "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, " "c.reloptions AS reloptions, " "tc.reloptions AS toast_reloptions, " "NULL AS changed_acl, " - "NULL AS partkeydef, " - "false AS ispartition, " - "NULL AS partbound " + "false AS ispartition " "FROM pg_class c " "LEFT JOIN pg_depend d ON " "(c.relkind = '%c' AND " @@ -6720,16 +6704,14 @@ getTables(Archive *fout, int *numTables) "'d' AS relreplident, c.relpages, " "NULL AS amname, " "NULL AS foreignserver, " - "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype, " + "c.reloftype, " "d.refobjid AS owning_tab, " "d.refobjsubid AS owning_col, " "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, " "c.reloptions AS reloptions, " "tc.reloptions AS toast_reloptions, " "NULL AS changed_acl, " - "NULL AS partkeydef, " - "false AS ispartition, " - "NULL AS partbound " + "false AS ispartition " "FROM pg_class c " "LEFT JOIN pg_depend d ON " "(c.relkind = '%c' AND " @@ -6768,16 +6750,14 @@ getTables(Archive *fout, int *numTables) "'d' AS relreplident, c.relpages, " "NULL AS amname, " "NULL AS foreignserver, " - "NULL AS reloftype, " + "0 AS reloftype, " "d.refobjid AS owning_tab, " "d.refobjsubid AS owning_col, " "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, " "c.reloptions AS reloptions, " "tc.reloptions AS toast_reloptions, " "NULL AS changed_acl, " - "NULL AS partkeydef, " - "false AS ispartition, " - "NULL AS partbound " + "false AS ispartition " "FROM pg_class c " "LEFT JOIN pg_depend d ON " "(c.relkind = '%c' AND " @@ -6816,16 +6796,14 @@ getTables(Archive *fout, int *numTables) "'d' AS relreplident, c.relpages, " "NULL AS amname, " "NULL AS foreignserver, " - "NULL AS reloftype, " + "0 AS reloftype, " "d.refobjid AS owning_tab, " "d.refobjsubid AS owning_col, " "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, " "c.reloptions AS reloptions, " "NULL AS toast_reloptions, " "NULL AS changed_acl, " - "NULL AS partkeydef, " - "false AS ispartition, " - "NULL AS partbound " + "false AS ispartition " "FROM pg_class c " "LEFT JOIN pg_depend d ON " "(c.relkind = '%c' AND " @@ -6863,16 +6841,14 @@ getTables(Archive *fout, int *numTables) "'d' AS relreplident, relpages, " "NULL AS amname, " "NULL AS foreignserver, " - "NULL AS reloftype, " + "0 AS reloftype, " "d.refobjid AS owning_tab, " "d.refobjsubid AS owning_col, " "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, " "NULL AS reloptions, " "NULL AS toast_reloptions, " "NULL AS changed_acl, " - "NULL AS partkeydef, " - "false AS ispartition, " - "NULL AS partbound " + "false AS ispartition " "FROM pg_class c " "LEFT JOIN pg_depend d ON " "(c.relkind = '%c' AND " @@ -6940,9 +6916,7 @@ getTables(Archive *fout, int *numTables) i_reloftype = PQfnumber(res, "reloftype"); i_is_identity_sequence = PQfnumber(res, "is_identity_sequence"); i_changed_acl = PQfnumber(res, "changed_acl"); - i_partkeydef = PQfnumber(res, "partkeydef"); i_ispartition = PQfnumber(res, "ispartition"); - i_partbound = PQfnumber(res, "partbound"); i_amname = PQfnumber(res, "amname"); if (dopt->lockWaitTimeout) @@ -6990,10 +6964,7 @@ getTables(Archive *fout, int *numTables) tblinfo[i].toast_oid = atooid(PQgetvalue(res, i, i_toastoid)); tblinfo[i].toast_frozenxid = atooid(PQgetvalue(res, i, i_toastfrozenxid)); tblinfo[i].toast_minmxid = atooid(PQgetvalue(res, i, i_toastminmxid)); - 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].ncheck = atoi(PQgetvalue(res, i, i_relchecks)); if (PQgetisnull(res, i, i_owning_tab)) { @@ -7049,10 +7020,8 @@ getTables(Archive *fout, int *numTables) tblinfo[i].is_identity_sequence = (i_is_identity_sequence >= 0 && strcmp(PQgetvalue(res, i, i_is_identity_sequence), "t") == 0); - /* Partition key string or NULL */ - tblinfo[i].partkeydef = pg_strdup(PQgetvalue(res, i, i_partkeydef)); + /* Partition? */ tblinfo[i].ispartition = (strcmp(PQgetvalue(res, i, i_ispartition), "t") == 0); - tblinfo[i].partbound = pg_strdup(PQgetvalue(res, i, i_partbound)); /* foreign server */ tblinfo[i].foreign_server = atooid(PQgetvalue(res, i, i_foreignserver)); @@ -15930,12 +15899,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(); @@ -15975,6 +15966,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo) break; default: reltypename = "TABLE"; + break; } numParents = tbinfo->numParents; @@ -15996,8 +15988,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) { @@ -16035,7 +16029,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; @@ -16068,7 +16063,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]); @@ -16131,7 +16126,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, @@ -16160,7 +16155,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)); @@ -16343,12 +16338,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)); } } @@ -16521,6 +16517,8 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo) tbinfo->attfdwoptions[j]); } /* end loop over columns */ + if (partkeydef) + free(partkeydef); if (ftoptions) free(ftoptions); if (srvname) @@ -16628,6 +16626,8 @@ dumpTableAttach(Archive *fout, const TableAttachInfo *attachinfo) { DumpOptions *dopt = fout->dopt; PQExpBuffer q; + PGresult *res; + char *partbound; if (dopt->dataOnly) return; @@ -16637,14 +16637,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 @@ -16661,6 +16670,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 8b2a8b67e7..0d03a8d2ec 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -286,7 +286,7 @@ typedef struct _tableInfo uint32 toast_frozenxid; /* toast table's relfrozenxid, if any */ uint32 toast_minmxid; /* toast table's relminmxid */ int ncheck; /* # of CHECK expressions */ - 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 */ @@ -325,8 +325,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 */