From e46e986baef04b9127a991f8a08d5c057671cb45 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 | 150 +++++++++++++++++++++----------------- src/bin/pg_dump/pg_dump.h | 4 +- 2 files changed, 84 insertions(+), 70 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 147a860b9b..76f8c51b6d 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -6129,14 +6129,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...) * @@ -6150,8 +6151,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 @@ -6160,9 +6163,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(); @@ -6179,13 +6180,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) @@ -6226,7 +6222,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, " @@ -6246,9 +6242,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 " @@ -6274,9 +6268,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, @@ -6319,7 +6311,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, " @@ -6328,9 +6320,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 " @@ -6372,7 +6362,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, " @@ -6381,9 +6371,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 " @@ -6425,7 +6413,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, " @@ -6434,9 +6422,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 " @@ -6478,16 +6464,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 " @@ -6527,16 +6511,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 " @@ -6575,16 +6557,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 " @@ -6623,16 +6603,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 " @@ -6670,16 +6648,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 " @@ -6747,9 +6723,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) @@ -6798,10 +6772,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)) { @@ -6857,10 +6828,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)); @@ -15934,12 +15903,34 @@ dumpTableSchema(Archive *fout, 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(); @@ -15979,6 +15970,7 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo) break; default: reltypename = "TABLE"; + break; } numParents = tbinfo->numParents; @@ -16000,8 +15992,10 @@ dumpTableSchema(Archive *fout, 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) { @@ -16039,7 +16033,8 @@ dumpTableSchema(Archive *fout, 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; @@ -16072,7 +16067,7 @@ dumpTableSchema(Archive *fout, 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]); @@ -16135,7 +16130,7 @@ dumpTableSchema(Archive *fout, 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, @@ -16164,7 +16159,7 @@ dumpTableSchema(Archive *fout, 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)); @@ -16347,12 +16342,13 @@ dumpTableSchema(Archive *fout, 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)); } } @@ -16365,16 +16361,34 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo) */ if (tbinfo->ispartition) { + PGresult *ares; + char *partbound; + PQExpBuffer q2; + /* With partitions there can only be one parent */ if (tbinfo->numParents != 1) fatal("invalid number of parents %d for table \"%s\"", tbinfo->numParents, tbinfo->dobj.name); + q2 = createPQExpBuffer(); + + /* Fetch the partition's partbound */ + appendPQExpBuffer(q2, + "SELECT pg_get_expr(c.relpartbound, c.oid) " + "FROM pg_class c " + "WHERE c.oid = '%u'", + tbinfo->dobj.catId.oid); + ares = ExecuteSqlQueryForSingleRow(fout, q2->data); + partbound = PQgetvalue(ares, 0, 0); + /* Perform ALTER TABLE on the parent */ appendPQExpBuffer(q, "ALTER TABLE ONLY %s ATTACH PARTITION %s %s;\n", fmtQualifiedDumpable(parents[0]), - qualrelname, tbinfo->partbound); + qualrelname, partbound); + + PQclear(ares); + destroyPQExpBuffer(q2); } /* @@ -16519,6 +16533,8 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo) tbinfo->attfdwoptions[j]); } + if (partkeydef) + free(partkeydef); if (ftoptions) free(ftoptions); if (srvname) diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h index 7d41064b8a..1e00426fc1 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -284,7 +284,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 */ @@ -322,8 +322,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 */