Fix pg_dump for better handling of inherited columns.
authorTom Lane
Fri, 10 Feb 2012 18:28:05 +0000 (13:28 -0500)
committerTom Lane
Fri, 10 Feb 2012 18:28:05 +0000 (13:28 -0500)
Revise pg_dump's handling of inherited columns, which was last looked at
seriously in 2001, to eliminate several misbehaviors associated with
inherited default expressions and NOT NULL flags.  In particular make sure
that a column is printed in a child table's CREATE TABLE command if and
only if it has attislocal = true; the former behavior would sometimes cause
a column to become marked attislocal when it was not so marked in the
source database.  Also, stop relying on textual comparison of default
expressions to decide if they're inherited; instead, don't use
default-expression inheritance at all, but just install the default
explicitly at each level of the hierarchy.  This fixes the
search-path-related misbehavior recently exhibited by Chester Young, and
also removes some dubious assumptions about the order in which ALTER TABLE
SET DEFAULT commands would be executed.

Back-patch to all supported branches.

src/bin/pg_dump/common.c
src/bin/pg_dump/pg_dump.c
src/bin/pg_dump/pg_dump.h
src/bin/pg_dump/pg_dump_sort.c

index 5a72d0da917a368ebbdbf164406c183693eb939c..266441df61d2c1eedd7faa39b8987254fe076f1c 100644 (file)
@@ -281,7 +281,13 @@ flagInhTables(TableInfo *tblinfo, int numTables,
 
 /* flagInhAttrs -
  *  for each dumpable table in tblinfo, flag its inherited attributes
- * so when we dump the table out, we don't dump out the inherited attributes
+ *
+ * What we need to do here is detect child columns that inherit NOT NULL
+ * bits from their parents (so that we needn't specify that again for the
+ * child) and child columns that have DEFAULT NULL when their parents had
+ * some non-null default.  In the latter case, we make up a dummy AttrDefInfo
+ * object so that we'll correctly emit the necessary DEFAULT NULL clause;
+ * otherwise the backend will apply an inherited default to the column.
  *
  * modifies tblinfo
  */
@@ -297,7 +303,6 @@ flagInhAttrs(TableInfo *tblinfo, int numTables)
        TableInfo  *tbinfo = &(tblinfo[i]);
        int         numParents;
        TableInfo **parents;
-       TableInfo  *parent;
 
        /* Sequences and views never have parents */
        if (tbinfo->relkind == RELKIND_SEQUENCE ||
@@ -314,132 +319,70 @@ flagInhAttrs(TableInfo *tblinfo, int numTables)
        if (numParents == 0)
            continue;           /* nothing to see here, move along */
 
-       /*----------------------------------------------------------------
-        * For each attr, check the parent info: if no parent has an attr
-        * with the same name, then it's not inherited. If there *is* an
-        * attr with the same name, then only dump it if:
-        *
-        * - it is NOT NULL and zero parents are NOT NULL
-        *   OR
-        * - it has a default value AND the default value does not match
-        *   all parent default values, or no parents specify a default.
-        *
-        * See discussion on -hackers around 2-Apr-2001.
-        *----------------------------------------------------------------
-        */
+       /* For each column, search for matching column names in parent(s) */
        for (j = 0; j < tbinfo->numatts; j++)
        {
-           bool        foundAttr;      /* Attr was found in a parent */
            bool        foundNotNull;   /* Attr was NOT NULL in a parent */
-           bool        defaultsMatch;  /* All non-empty defaults match */
-           bool        defaultsFound;  /* Found a default in a parent */
-           AttrDefInfo *attrDef;
-
-           foundAttr = false;
-           foundNotNull = false;
-           defaultsMatch = true;
-           defaultsFound = false;
+           bool        foundDefault;   /* Found a default in a parent */
 
-           attrDef = tbinfo->attrdefs[j];
+           /* no point in examining dropped columns */
+           if (tbinfo->attisdropped[j])
+               continue;
 
+           foundNotNull = false;
+           foundDefault = false;
            for (k = 0; k < numParents; k++)
            {
+               TableInfo  *parent = parents[k];
                int         inhAttrInd;
 
-               parent = parents[k];
                inhAttrInd = strInArray(tbinfo->attnames[j],
                                        parent->attnames,
                                        parent->numatts);
-
-               if (inhAttrInd != -1)
+               if (inhAttrInd >= 0)
                {
-                   AttrDefInfo *inhDef = parent->attrdefs[inhAttrInd];
-
-                   foundAttr = true;
                    foundNotNull |= parent->notnull[inhAttrInd];
-                   if (inhDef != NULL)
-                   {
-                       defaultsFound = true;
-
-                       /*
-                        * If any parent has a default and the child doesn't,
-                        * we have to emit an explicit DEFAULT NULL clause for
-                        * the child, else the parent's default will win.
-                        */
-                       if (attrDef == NULL)
-                       {
-                           attrDef = (AttrDefInfo *) pg_malloc(sizeof(AttrDefInfo));
-                           attrDef->dobj.objType = DO_ATTRDEF;
-                           attrDef->dobj.catId.tableoid = 0;
-                           attrDef->dobj.catId.oid = 0;
-                           AssignDumpId(&attrDef->dobj);
-                           attrDef->adtable = tbinfo;
-                           attrDef->adnum = j + 1;
-                           attrDef->adef_expr = pg_strdup("NULL");
-
-                           attrDef->dobj.name = pg_strdup(tbinfo->dobj.name);
-                           attrDef->dobj.namespace = tbinfo->dobj.namespace;
-
-                           attrDef->dobj.dump = tbinfo->dobj.dump;
-
-                           attrDef->separate = false;
-                           addObjectDependency(&tbinfo->dobj,
-                                               attrDef->dobj.dumpId);
-
-                           tbinfo->attrdefs[j] = attrDef;
-                       }
-                       if (strcmp(attrDef->adef_expr, inhDef->adef_expr) != 0)
-                       {
-                           defaultsMatch = false;
-
-                           /*
-                            * Whenever there is a non-matching parent
-                            * default, add a dependency to force the parent
-                            * default to be dumped first, in case the
-                            * defaults end up being dumped as separate
-                            * commands.  Otherwise the parent default will
-                            * override the child's when it is applied.
-                            */
-                           addObjectDependency(&attrDef->dobj,
-                                               inhDef->dobj.dumpId);
-                       }
-                   }
+                   foundDefault |= (parent->attrdefs[inhAttrInd] != NULL);
                }
            }
 
-           /*
-            * Based on the scan of the parents, decide if we can rely on the
-            * inherited attr
-            */
-           if (foundAttr)      /* Attr was inherited */
+           /* Remember if we found inherited NOT NULL */
+           tbinfo->inhNotNull[j] = foundNotNull;
+
+           /* Manufacture a DEFAULT NULL clause if necessary */
+           if (foundDefault && tbinfo->attrdefs[j] == NULL)
            {
-               /* Set inherited flag by default */
-               tbinfo->inhAttrs[j] = true;
-               tbinfo->inhAttrDef[j] = true;
-               tbinfo->inhNotNull[j] = true;
-
-               /*
-                * Clear it if attr had a default, but parents did not, or
-                * mismatch
-                */
-               if ((attrDef != NULL) && (!defaultsFound || !defaultsMatch))
+               AttrDefInfo *attrDef;
+
+               attrDef = (AttrDefInfo *) pg_malloc(sizeof(AttrDefInfo));
+               attrDef->dobj.objType = DO_ATTRDEF;
+               attrDef->dobj.catId.tableoid = 0;
+               attrDef->dobj.catId.oid = 0;
+               AssignDumpId(&attrDef->dobj);
+               attrDef->dobj.name = pg_strdup(tbinfo->dobj.name);
+               attrDef->dobj.namespace = tbinfo->dobj.namespace;
+               attrDef->dobj.dump = tbinfo->dobj.dump;
+
+               attrDef->adtable = tbinfo;
+               attrDef->adnum = j + 1;
+               attrDef->adef_expr = pg_strdup("NULL");
+
+               /* Will column be dumped explicitly? */
+               if (shouldPrintColumn(tbinfo, j))
                {
-                   tbinfo->inhAttrs[j] = false;
-                   tbinfo->inhAttrDef[j] = false;
+                   attrDef->separate = false;
+                   /* No dependency needed: NULL cannot have dependencies */
                }
-
-               /*
-                * Clear it if NOT NULL and none of the parents were NOT NULL
-                */
-               if (tbinfo->notnull[j] && !foundNotNull)
+               else
                {
-                   tbinfo->inhAttrs[j] = false;
-                   tbinfo->inhNotNull[j] = false;
+                   /* column will be suppressed, print default separately */
+                   attrDef->separate = true;
+                   /* ensure it comes out after the table */
+                   addObjectDependency(&attrDef->dobj,
+                                       tbinfo->dobj.dumpId);
                }
 
-               /* Clear it if attr has local definition */
-               if (tbinfo->attislocal[j])
-                   tbinfo->inhAttrs[j] = false;
+               tbinfo->attrdefs[j] = attrDef;
            }
        }
    }
index c91e0983f4de2d0a27a999dea19e05800cda2daa..c55f71cd9a057404bc6b620a06e44a9ae4b7aa36 100644 (file)
@@ -5833,6 +5833,9 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
             * attstattarget doesn't exist in 7.1.  It does exist in 7.2, but
             * we don't dump it because we can't tell whether it's been
             * explicitly set or was just a default.
+            *
+            * attislocal doesn't exist before 7.3, either; in older databases
+            * we just assume that inherited columns had no local definition.
             */
            appendPQExpBuffer(q, "SELECT a.attnum, a.attname, a.atttypmod, "
                              "-1 AS attstattarget, a.attstorage, "
@@ -5900,14 +5903,12 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
        tbinfo->attlen = (int *) pg_malloc(ntups * sizeof(int));
        tbinfo->attalign = (char *) pg_malloc(ntups * sizeof(char));
        tbinfo->attislocal = (bool *) pg_malloc(ntups * sizeof(bool));
-       tbinfo->notnull = (bool *) pg_malloc(ntups * sizeof(bool));
-       tbinfo->attrdefs = (AttrDefInfo **) pg_malloc(ntups * sizeof(AttrDefInfo *));
        tbinfo->attoptions = (char **) pg_malloc(ntups * sizeof(char *));
        tbinfo->attcollation = (Oid *) pg_malloc(ntups * sizeof(Oid));
        tbinfo->attfdwoptions = (char **) pg_malloc(ntups * sizeof(char *));
-       tbinfo->inhAttrs = (bool *) pg_malloc(ntups * sizeof(bool));
-       tbinfo->inhAttrDef = (bool *) pg_malloc(ntups * sizeof(bool));
+       tbinfo->notnull = (bool *) pg_malloc(ntups * sizeof(bool));
        tbinfo->inhNotNull = (bool *) pg_malloc(ntups * sizeof(bool));
+       tbinfo->attrdefs = (AttrDefInfo **) pg_malloc(ntups * sizeof(AttrDefInfo *));
        hasdefaults = false;
 
        for (j = 0; j < ntups; j++)
@@ -5936,8 +5937,6 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
            if (PQgetvalue(res, j, i_atthasdef)[0] == 't')
                hasdefaults = true;
            /* these flags will be set in flagInhAttrs() */
-           tbinfo->inhAttrs[j] = false;
-           tbinfo->inhAttrDef[j] = false;
            tbinfo->inhNotNull[j] = false;
        }
 
@@ -6000,12 +5999,28 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
            {
                int         adnum;
 
+               adnum = atoi(PQgetvalue(res, j, 2));
+
+               if (adnum <= 0 || adnum > ntups)
+               {
+                   write_msg(NULL, "invalid adnum value %d for table \"%s\"\n",
+                             adnum, tbinfo->dobj.name);
+                   exit_nicely();
+               }
+
+               /*
+                * dropped columns shouldn't have defaults, but just in case,
+                * ignore 'em
+                */
+               if (tbinfo->attisdropped[adnum - 1])
+                   continue;
+
                attrdefs[j].dobj.objType = DO_ATTRDEF;
                attrdefs[j].dobj.catId.tableoid = atooid(PQgetvalue(res, j, 0));
                attrdefs[j].dobj.catId.oid = atooid(PQgetvalue(res, j, 1));
                AssignDumpId(&attrdefs[j].dobj);
                attrdefs[j].adtable = tbinfo;
-               attrdefs[j].adnum = adnum = atoi(PQgetvalue(res, j, 2));
+               attrdefs[j].adnum = adnum;
                attrdefs[j].adef_expr = pg_strdup(PQgetvalue(res, j, 3));
 
                attrdefs[j].dobj.name = pg_strdup(tbinfo->dobj.name);
@@ -6016,9 +6031,8 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
                /*
                 * Defaults on a VIEW must always be dumped as separate ALTER
                 * TABLE commands.  Defaults on regular tables are dumped as
-                * part of the CREATE TABLE if possible.  To check if it's
-                * safe, we mark the default as needing to appear before the
-                * CREATE.
+                * part of the CREATE TABLE if possible, which it won't be
+                * if the column is not going to be emitted explicitly.
                 */
                if (tbinfo->relkind == RELKIND_VIEW)
                {
@@ -6027,19 +6041,27 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
                    addObjectDependency(&attrdefs[j].dobj,
                                        tbinfo->dobj.dumpId);
                }
+               else if (!shouldPrintColumn(tbinfo, adnum - 1))
+               {
+                   /* column will be suppressed, print default separately */
+                   attrdefs[j].separate = true;
+                   /* needed in case pre-7.3 DB: */
+                   addObjectDependency(&attrdefs[j].dobj,
+                                       tbinfo->dobj.dumpId);
+               }
                else
                {
                    attrdefs[j].separate = false;
+                   /*
+                    * Mark the default as needing to appear before the table,
+                    * so that any dependencies it has must be emitted before
+                    * the CREATE TABLE.  If this is not possible, we'll
+                    * change to "separate" mode while sorting dependencies.
+                    */
                    addObjectDependency(&tbinfo->dobj,
                                        attrdefs[j].dobj.dumpId);
                }
 
-               if (adnum <= 0 || adnum > ntups)
-               {
-                   write_msg(NULL, "invalid adnum value %d for table \"%s\"\n",
-                             adnum, tbinfo->dobj.name);
-                   exit_nicely();
-               }
                tbinfo->attrdefs[adnum - 1] = &attrdefs[j];
            }
            PQclear(res);
@@ -6223,6 +6245,28 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
    destroyPQExpBuffer(q);
 }
 
+/*
+ * Test whether a column should be printed as part of table's CREATE TABLE.
+ * Column number is zero-based.
+ *
+ * Normally this is always true, but it's false for dropped columns, as well
+ * as those that were inherited without any local definition.  (If we print
+ * such a column it will mistakenly get pg_attribute.attislocal set to true.)
+ * However, in binary_upgrade mode, we must print all such columns anyway and
+ * fix the attislocal/attisdropped state later, so as to keep control of the
+ * physical column order.
+ *
+ * This function exists because there are scattered nonobvious places that
+ * must be kept in sync with this decision.
+ */
+bool
+shouldPrintColumn(TableInfo *tbinfo, int colno)
+{
+   if (binary_upgrade)
+       return true;
+   return (tbinfo->attislocal[colno] && !tbinfo->attisdropped[colno]);
+}
+
 
 /*
  * getTSParsers:
@@ -12504,38 +12548,40 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
                          fmtId(tbinfo->dobj.name));
 
        /*
-        * In case of a binary upgrade, we dump the table normally and attach
-        * it to the type afterward.
+        * 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 && !binary_upgrade)
            appendPQExpBuffer(q, " OF %s", tbinfo->reloftype);
+
+       /* Dump the attributes */
        actual_atts = 0;
        for (j = 0; j < tbinfo->numatts; j++)
        {
            /*
-            * Normally, dump if it's one of the table's own attrs, and not
-            * dropped.  But for binary upgrade, dump all the columns.
+            * Normally, dump if it's locally defined in this table, and not
+            * dropped.  But for binary upgrade, we'll dump all the columns,
+            * and then fix up the dropped and nonlocal cases below.
             */
-           if ((!tbinfo->inhAttrs[j] && !tbinfo->attisdropped[j]) ||
-               binary_upgrade)
+           if (shouldPrintColumn(tbinfo, j))
            {
                /*
-                * Default value --- suppress if inherited (except in
-                * binary-upgrade case, where we're not doing normal
-                * inheritance) or if it's to be printed separately.
+                * Default value --- suppress if to be printed separately.
                 */
-               bool        has_default = (tbinfo->attrdefs[j] != NULL
-                               && (!tbinfo->inhAttrDef[j] || binary_upgrade)
-                                          && !tbinfo->attrdefs[j]->separate);
+               bool        has_default = (tbinfo->attrdefs[j] != NULL &&
+                                          !tbinfo->attrdefs[j]->separate);
 
                /*
                 * Not Null constraint --- suppress if inherited, except in
-                * binary-upgrade case.
+                * binary-upgrade case where that won't work.
                 */
-               bool        has_notnull = (tbinfo->notnull[j]
-                             && (!tbinfo->inhNotNull[j] || binary_upgrade));
+               bool        has_notnull = (tbinfo->notnull[j] &&
+                                          (!tbinfo->inhNotNull[j] ||
+                                           binary_upgrade));
 
-               if (tbinfo->reloftype && !has_default && !has_notnull && !binary_upgrade)
+               /* Skip column if fully defined by reloftype */
+               if (tbinfo->reloftype &&
+                   !has_default && !has_notnull && !binary_upgrade)
                    continue;
 
                /* Format properly if not first attr */
@@ -12799,16 +12845,36 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
            }
        }
 
-       /* Loop dumping statistics and storage statements */
+       /*
+        * Dump additional per-column properties that we can't handle in the
+        * main CREATE TABLE command.
+        */
        for (j = 0; j < tbinfo->numatts; j++)
        {
+           /* None of this applies to dropped columns */
+           if (tbinfo->attisdropped[j])
+               continue;
+
+           /*
+            * If we didn't dump the column definition explicitly above, and
+            * it is NOT NULL and did not inherit that property from a parent,
+            * we have to mark it separately.
+            */
+           if (!shouldPrintColumn(tbinfo, j) &&
+               tbinfo->notnull[j] && !tbinfo->inhNotNull[j])
+           {
+               appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
+                                 fmtId(tbinfo->dobj.name));
+               appendPQExpBuffer(q, "ALTER COLUMN %s SET NOT NULL;\n",
+                                 fmtId(tbinfo->attnames[j]));
+           }
+
            /*
             * Dump per-column statistics information. We only issue an ALTER
             * TABLE statement if the attstattarget entry for this column is
             * non-negative (i.e. it's not the default value)
             */
-           if (tbinfo->attstattarget[j] >= 0 &&
-               !tbinfo->attisdropped[j])
+           if (tbinfo->attstattarget[j] >= 0)
            {
                appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
                                  fmtId(tbinfo->dobj.name));
@@ -12822,7 +12888,7 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
             * Dump per-column storage information.  The statement is only
             * dumped if the storage has been changed from the type's default.
             */
-           if (!tbinfo->attisdropped[j] && tbinfo->attstorage[j] != tbinfo->typstorage[j])
+           if (tbinfo->attstorage[j] != tbinfo->typstorage[j])
            {
                switch (tbinfo->attstorage[j])
                {
@@ -12935,18 +13001,18 @@ dumpAttrDef(Archive *fout, AttrDefInfo *adinfo)
    PQExpBuffer q;
    PQExpBuffer delq;
 
-   /* Only print it if "separate" mode is selected */
-   if (!tbinfo->dobj.dump || !adinfo->separate || dataOnly)
+   /* Skip if table definition not to be dumped */
+   if (!tbinfo->dobj.dump || dataOnly)
        return;
 
-   /* Don't print inherited defaults, either, except for binary upgrade */
-   if (tbinfo->inhAttrDef[adnum - 1] && !binary_upgrade)
+   /* Skip if not "separate"; it was dumped in the table's definition */
+   if (!adinfo->separate)
        return;
 
    q = createPQExpBuffer();
    delq = createPQExpBuffer();
 
-   appendPQExpBuffer(q, "ALTER TABLE %s ",
+   appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
                      fmtId(tbinfo->dobj.name));
    appendPQExpBuffer(q, "ALTER COLUMN %s SET DEFAULT %s;\n",
                      fmtId(tbinfo->attnames[adnum - 1]),
index 44f7c6bdf0a8495ba8cccf277bf4ef93cc3df312..421847e5d5aa1b9a0ea10183020987a42d1e8742 100644 (file)
@@ -277,17 +277,9 @@ typedef struct _tableInfo
    char      **attoptions;     /* per-attribute options */
    Oid        *attcollation;   /* per-attribute collation selection */
    char      **attfdwoptions;  /* per-attribute fdw options */
-
-   /*
-    * Note: we need to store per-attribute notnull, default, and constraint
-    * stuff for all interesting tables so that we can tell which constraints
-    * were inherited.
-    */
-   bool       *notnull;        /* Not null constraints on attributes */
-   struct _attrDefInfo **attrdefs;     /* DEFAULT expressions */
-   bool       *inhAttrs;       /* true if each attribute is inherited */
-   bool       *inhAttrDef;     /* true if attr's default is inherited */
+   bool       *notnull;        /* NOT NULL constraints on attributes */
    bool       *inhNotNull;     /* true if NOT NULL is inherited */
+   struct _attrDefInfo **attrdefs;     /* DEFAULT expressions */
    struct _constraintInfo *checkexprs; /* CHECK constraints */
 
    /*
@@ -300,7 +292,7 @@ typedef struct _tableInfo
 
 typedef struct _attrDefInfo
 {
-   DumpableObject dobj;
+   DumpableObject dobj;        /* note: dobj.name is name of table */
    TableInfo  *adtable;        /* link to table of attribute */
    int         adnum;
    char       *adef_expr;      /* decompiled DEFAULT expression */
@@ -556,6 +548,7 @@ extern void getTriggers(Archive *fout, TableInfo tblinfo[], int numTables);
 extern ProcLangInfo *getProcLangs(Archive *fout, int *numProcLangs);
 extern CastInfo *getCasts(Archive *fout, int *numCasts);
 extern void getTableAttrs(Archive *fout, TableInfo *tbinfo, int numTables);
+extern bool shouldPrintColumn(TableInfo *tbinfo, int colno);
 extern TSParserInfo *getTSParsers(Archive *fout, int *numTSParsers);
 extern TSDictInfo *getTSDictionaries(Archive *fout, int *numTSDicts);
 extern TSTemplateInfo *getTSTemplates(Archive *fout, int *numTSTemplates);
index 4d1ae94ef5142ed27d55a0a90544edaf4658530d..6f61c0d848fc5c9e414859a2b3fca57e011a4481 100644 (file)
@@ -188,6 +188,15 @@ DOTypeNameCompare(const void *p1, const void *p2)
        if (cmpval != 0)
            return cmpval;
    }
+   else if (obj1->objType == DO_ATTRDEF)
+   {
+       AttrDefInfo *adobj1 = *(AttrDefInfo * const *) p1;
+       AttrDefInfo *adobj2 = *(AttrDefInfo * const *) p2;
+
+       cmpval = (adobj1->adnum - adobj2->adnum);
+       if (cmpval != 0)
+           return cmpval;
+   }
 
    /* Usually shouldn't get here, but if we do, sort by OID */
    return oidcmp(obj1->catId.oid, obj2->catId.oid);