From 182228bd747caa362664bff525fc8346a37da16e Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 10 Feb 2012 13:28:10 -0500 Subject: [PATCH] Fix pg_dump for better handling of inherited columns. 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 | 153 +++++++++++---------------------- src/bin/pg_dump/pg_dump.c | 148 ++++++++++++++++++++++--------- src/bin/pg_dump/pg_dump.h | 15 +--- src/bin/pg_dump/pg_dump_sort.c | 9 ++ 4 files changed, 168 insertions(+), 157 deletions(-) diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c index aa9c606969..54451fcafc 100644 --- a/src/bin/pg_dump/common.c +++ b/src/bin/pg_dump/common.c @@ -283,7 +283,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 */ @@ -299,7 +305,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 || @@ -316,132 +321,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 *) 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 = strdup("NULL"); - - attrDef->dobj.name = 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 *) malloc(sizeof(AttrDefInfo)); + attrDef->dobj.objType = DO_ATTRDEF; + attrDef->dobj.catId.tableoid = 0; + attrDef->dobj.catId.oid = 0; + AssignDumpId(&attrDef->dobj); + attrDef->dobj.name = 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 = 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; } } } diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 06fa3c92a4..880d1415f1 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -5672,6 +5672,9 @@ getTableAttrs(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, " @@ -5737,13 +5740,11 @@ getTableAttrs(TableInfo *tblinfo, int numTables) tbinfo->attlen = (int *) malloc(ntups * sizeof(int)); tbinfo->attalign = (char *) malloc(ntups * sizeof(char)); tbinfo->attislocal = (bool *) malloc(ntups * sizeof(bool)); - tbinfo->notnull = (bool *) malloc(ntups * sizeof(bool)); - tbinfo->attrdefs = (AttrDefInfo **) malloc(ntups * sizeof(AttrDefInfo *)); tbinfo->attoptions = (char **) malloc(ntups * sizeof(char *)); tbinfo->attcollation = (Oid *) malloc(ntups * sizeof(Oid)); - tbinfo->inhAttrs = (bool *) malloc(ntups * sizeof(bool)); - tbinfo->inhAttrDef = (bool *) malloc(ntups * sizeof(bool)); + tbinfo->notnull = (bool *) malloc(ntups * sizeof(bool)); tbinfo->inhNotNull = (bool *) malloc(ntups * sizeof(bool)); + tbinfo->attrdefs = (AttrDefInfo **) malloc(ntups * sizeof(AttrDefInfo *)); hasdefaults = false; for (j = 0; j < ntups; j++) @@ -5771,8 +5772,6 @@ getTableAttrs(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; } @@ -5836,12 +5835,28 @@ getTableAttrs(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 = strdup(PQgetvalue(res, j, 3)); attrdefs[j].dobj.name = strdup(tbinfo->dobj.name); @@ -5852,9 +5867,8 @@ getTableAttrs(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) { @@ -5863,19 +5877,27 @@ getTableAttrs(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); @@ -6024,6 +6046,28 @@ getTableAttrs(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: @@ -12133,38 +12177,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 */ @@ -12428,16 +12474,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)); @@ -12451,7 +12517,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]) { @@ -12549,18 +12615,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]), diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h index c95614b16f..1539685587 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -275,17 +275,9 @@ typedef struct _tableInfo bool *attislocal; /* true if attr has local definition */ char **attoptions; /* per-attribute options */ Oid *attcollation; /* per-attribute collation selection */ - - /* - * 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 */ /* @@ -298,7 +290,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(TableInfo tblinfo[], int numTables); extern ProcLangInfo *getProcLangs(int *numProcLangs); extern CastInfo *getCasts(int *numCasts); extern void getTableAttrs(TableInfo *tbinfo, int numTables); +extern bool shouldPrintColumn(TableInfo *tbinfo, int colno); extern TSParserInfo *getTSParsers(int *numTSParsers); extern TSDictInfo *getTSDictionaries(int *numTSDicts); extern TSTemplateInfo *getTSTemplates(int *numTSTemplates); diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c index 963f734dce..72ea2f1226 100644 --- a/src/bin/pg_dump/pg_dump_sort.c +++ b/src/bin/pg_dump/pg_dump_sort.c @@ -177,6 +177,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); -- 2.40.0