Postpone calls of unsafe server-side functions in pg_dump.
authorTom Lane
Sat, 19 Nov 2022 16:40:30 +0000 (11:40 -0500)
committerTom Lane
Sat, 19 Nov 2022 16:40:30 +0000 (11:40 -0500)
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
src/bin/pg_dump/pg_dump.h

index e110257395cbc5a37c7584cbf2d1d300bbd6d543..8c5c4f01668cbb07986090dedd3418c003944e5f 100644 (file)
@@ -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);
 }
 
index 8b2a8b67e7eeae1d3c8246cb89ca5bdc73fdb3c8..0d03a8d2ec2f8f28156363526719c231770dff9b 100644 (file)
@@ -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 */