Re: ALTER TABLE with ADD COLUMN and ADD PRIMARY KEY USING INDEX throws spurious "column contains null values"
| От | Tom Lane |
|---|---|
| Тема | Re: ALTER TABLE with ADD COLUMN and ADD PRIMARY KEY USING INDEX throws spurious "column contains null values" |
| Дата | |
| Msg-id | 22853.1555881688@sss.pgh.pa.us обсуждение исходный текст |
| Ответ на | Re: ALTER TABLE with ADD COLUMN and ADD PRIMARY KEY USING INDEXthrows spurious "column contains null values" (Robert Haas <robertmhaas@gmail.com>) |
| Список | pgsql-hackers |
Robert Haas <robertmhaas@gmail.com> writes: > On Wed, Apr 17, 2019 at 11:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> * I'm not sure whether we want to try to back-patch this, or how >> far it should go. The misbehavior has been there a long time >> (at least back to 8.4, I didn't check further); so the lack of >> previous reports means people seldom try to do it. That may >> indicate that it's not worth taking any risks of new bugs to >> squash this one. (Also, I suspect that it might take a lot of >> work to port this to before v10, because there are comments >> suggesting that this code worked a good bit differently before.) >> I do think we should shove this into v12 though. > Shoving it into v12 but not back-patching seems like a reasonable > compromise, although I have not reviewed the patch or tried to figure > out how risky it is. Here's a less-WIP patch for that. I fixed up some more stuff: >> * I initially thought that index_check_primary_key could be simplified >> to the point where it *only* throws an error for missing NOT NULL. >> This soon proved to be wrong, because the comments for the function >> are lies, or at least obsolete: there are multiple scenarios in which >> a CREATE TABLE with a PRIMARY KEY option does need this function to >> perform ALTER TABLE SET NOT NULL. I decided that a cleaner way to handle this was to make the parser generate required ALTER TABLE SET NOT NULL operations in all cases, not just the ALTER TABLE case. This gets rid of the former confused situation wherein transformIndexConstraint forced primary-key columns NOT NULL in some situations and abdicated responsibility in others. >> * We need to fix the order of operations in ALTER TABLE phase 2 so that >> any AT_SetNotNull subcommands happen after the AT_PASS_ADD_COL pass >> (else the column might not be there yet) and before AT_PASS_ADD_INDEX >> (else index_check_primary_key will complain). I did this by putting >> AT_SetNotNull into the AT_PASS_COL_ATTRS pass and moving that to after >> AT_PASS_ADD_COL; it had been before AT_PASS_ADD_COL, but that seems at >> best random and at worst broken. (AT_SetIdentity is the only existing >> subcommand using AT_PASS_COL_ATTRS, and it sure seems like it'd make more >> sense to run it after AT_PASS_ADD_COL, so that it can work on a column >> being added in the same ALTER. Am I missing something?) Sure enough, AT_SetIdentity is broken for the case where the column was just created in the same ALTER command, as per test case added below. Admittedly, that's a fairly unlikely thing to do, but it should work; so the current ordering of these passes is wrong. BTW, now that we have an AT_PASS_COL_ATTRS pass, it's a bit tempting to shove other stuff that's in the nature of change-a-column-attribute into it; there are several AT_ subcommands of that sort that are currently in AT_PASS_MISC. I didn't do that here though. Also, this fixes the issue complained of in https://postgr.es/m/16115.1555874162@sss.pgh.pa.us Barring objections I'll commit this tomorrow or so. regards, tom lane diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index e9399be..331f905 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -185,13 +185,14 @@ relationHasPrimaryKey(Relation rel) * * We check for a pre-existing primary key, and that all columns of the index * are simple column references (not expressions), and that all those - * columns are marked NOT NULL. If they aren't (which can only happen during - * ALTER TABLE ADD CONSTRAINT, since the parser forces such columns to be - * created NOT NULL during CREATE TABLE), do an ALTER SET NOT NULL to mark - * them so --- or fail if they are not in fact nonnull. + * columns are marked NOT NULL. If not, fail. * - * As of PG v10, the SET NOT NULL is applied to child tables as well, so - * that the behavior is like a manual SET NOT NULL. + * We used to automatically change unmarked columns to NOT NULL here by doing + * our own local ALTER TABLE command. But that doesn't work well if we're + * executing one subcommand of an ALTER TABLE: the operations may not get + * performed in the right order overall. Now we expect that the parser + * inserted any required ALTER TABLE SET NOT NULL operations before trying + * to create a primary-key index. * * Caller had better have at least ShareLock on the table, else the not-null * checking isn't trustworthy. @@ -202,12 +203,11 @@ index_check_primary_key(Relation heapRel, bool is_alter_table, IndexStmt *stmt) { - List *cmds; int i; /* - * If ALTER TABLE and CREATE TABLE .. PARTITION OF, check that there isn't - * already a PRIMARY KEY. In CREATE TABLE for an ordinary relations, we + * If ALTER TABLE or CREATE TABLE .. PARTITION OF, check that there isn't + * already a PRIMARY KEY. In CREATE TABLE for an ordinary relation, we * have faith that the parser rejected multiple pkey clauses; and CREATE * INDEX doesn't have a way to say PRIMARY KEY, so it's no problem either. */ @@ -222,9 +222,9 @@ index_check_primary_key(Relation heapRel, /* * Check that all of the attributes in a primary key are marked as not - * null, otherwise attempt to ALTER TABLE .. SET NOT NULL + * null. (We don't really expect to see that; it'd mean the parser messed + * up. But it seems wise to check anyway.) */ - cmds = NIL; for (i = 0; i < indexInfo->ii_NumIndexKeyAttrs; i++) { AttrNumber attnum = indexInfo->ii_IndexAttrNumbers[i]; @@ -249,30 +249,13 @@ index_check_primary_key(Relation heapRel, attform = (Form_pg_attribute) GETSTRUCT(atttuple); if (!attform->attnotnull) - { - /* Add a subcommand to make this one NOT NULL */ - AlterTableCmd *cmd = makeNode(AlterTableCmd); - - cmd->subtype = AT_SetNotNull; - cmd->name = pstrdup(NameStr(attform->attname)); - cmds = lappend(cmds, cmd); - } + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("primary key column \"%s\" is not marked NOT NULL", + NameStr(attform->attname)))); ReleaseSysCache(atttuple); } - - /* - * XXX: possible future improvement: when being called from ALTER TABLE, - * it would be more efficient to merge this with the outer ALTER TABLE, so - * as to avoid two scans. But that seems to complicate DefineIndex's API - * unduly. - */ - if (cmds) - { - EventTriggerAlterTableStart((Node *) stmt); - AlterTableInternal(RelationGetRelid(heapRel), cmds, true); - EventTriggerAlterTableEnd(); - } } /* diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index d48a947..aa7328e 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -142,9 +142,9 @@ static List *on_commits = NIL; #define AT_PASS_ALTER_TYPE 1 /* ALTER COLUMN TYPE */ #define AT_PASS_OLD_INDEX 2 /* re-add existing indexes */ #define AT_PASS_OLD_CONSTR 3 /* re-add existing constraints */ -#define AT_PASS_COL_ATTRS 4 /* set other column attributes */ /* We could support a RENAME COLUMN pass here, but not currently used */ -#define AT_PASS_ADD_COL 5 /* ADD COLUMN */ +#define AT_PASS_ADD_COL 4 /* ADD COLUMN */ +#define AT_PASS_COL_ATTRS 5 /* set other column attributes */ #define AT_PASS_ADD_INDEX 6 /* ADD indexes */ #define AT_PASS_ADD_CONSTR 7 /* ADD constraints, defaults */ #define AT_PASS_MISC 8 /* other stuff */ @@ -370,9 +370,13 @@ static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid); static void add_column_collation_dependency(Oid relid, int32 attnum, Oid collid); static void ATPrepDropNotNull(Relation rel, bool recurse, bool recursing); static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode); -static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing); +static void ATPrepSetNotNull(List **wqueue, Relation rel, + AlterTableCmd *cmd, bool recurse, bool recursing, + LOCKMODE lockmode); static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel, const char *colName, LOCKMODE lockmode); +static void ATExecCheckNotNull(AlteredTableInfo *tab, Relation rel, + const char *colName, LOCKMODE lockmode); static bool NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr); static bool ConstraintImpliedByRelConstraint(Relation scanrel, List *partConstraint, List *existedConstraints); @@ -1068,7 +1072,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId, RelationGetDescr(parent), gettext_noop("could not convert row type")); idxstmt = - generateClonedIndexStmt(NULL, RelationGetRelid(rel), idxRel, + generateClonedIndexStmt(NULL, idxRel, attmap, RelationGetDescr(rel)->natts, &constraintOid); DefineIndex(RelationGetRelid(rel), @@ -3765,6 +3769,15 @@ AlterTableGetLockLevel(List *cmds) cmd_lockmode = AccessExclusiveLock; break; + case AT_CheckNotNull: + + /* + * This only examines the table's schema; but lock must be + * strong enough to prevent concurrent DROP NOT NULL. + */ + cmd_lockmode = AccessShareLock; + break; + default: /* oops */ elog(ERROR, "unrecognized alter table type: %d", (int) cmd->subtype); @@ -3889,15 +3902,19 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE); ATPrepDropNotNull(rel, recurse, recursing); ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode); - /* No command-specific prep needed */ pass = AT_PASS_DROP; break; case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */ ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE); - ATPrepSetNotNull(rel, recurse, recursing); + /* Need command-specific recursion decision */ + ATPrepSetNotNull(wqueue, rel, cmd, recurse, recursing, lockmode); + pass = AT_PASS_COL_ATTRS; + break; + case AT_CheckNotNull: /* check column is already marked NOT NULL */ + ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE); ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode); /* No command-specific prep needed */ - pass = AT_PASS_ADD_CONSTR; + pass = AT_PASS_COL_ATTRS; break; case AT_SetStatistics: /* ALTER COLUMN SET STATISTICS */ ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode); @@ -4214,6 +4231,9 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel, case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */ address = ATExecSetNotNull(tab, rel, cmd->name, lockmode); break; + case AT_CheckNotNull: /* check column is already marked NOT NULL */ + ATExecCheckNotNull(tab, rel, cmd->name, lockmode); + break; case AT_SetStatistics: /* ALTER COLUMN SET STATISTICS */ address = ATExecSetStatistics(rel, cmd->name, cmd->num, cmd->def, lockmode); break; @@ -5966,9 +5986,6 @@ add_column_collation_dependency(Oid relid, int32 attnum, Oid collid) /* * ALTER TABLE ALTER COLUMN DROP NOT NULL - * - * Return the address of the modified column. If the column was already - * nullable, InvalidObjectAddress is returned. */ static void @@ -5990,6 +6007,11 @@ ATPrepDropNotNull(Relation rel, bool recurse, bool recursing) errhint("Do not specify the ONLY keyword."))); } } + +/* + * Return the address of the modified column. If the column was already + * nullable, InvalidObjectAddress is returned. + */ static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode) { @@ -6116,23 +6138,33 @@ ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode) */ static void -ATPrepSetNotNull(Relation rel, bool recurse, bool recursing) +ATPrepSetNotNull(List **wqueue, Relation rel, + AlterTableCmd *cmd, bool recurse, bool recursing, + LOCKMODE lockmode) { /* - * If the parent is a partitioned table, like check constraints, NOT NULL - * constraints must be added to the child tables. Complain if requested - * otherwise and partitions exist. + * If we're already recursing, there's nothing to do; the topmost + * invocation of ATSimpleRecursion already visited all children. */ - if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + if (recursing) + return; + + /* + * If we have ALTER TABLE ONLY ... SET NOT NULL on a partitioned table, + * apply ALTER TABLE ... CHECK NOT NULL to every child. Otherwise, use + * normal recursion logic. + */ + if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE && + !recurse) { - PartitionDesc partdesc = RelationGetPartitionDesc(rel); + AlterTableCmd *newcmd = makeNode(AlterTableCmd); - if (partdesc && partdesc->nparts > 0 && !recurse && !recursing) - ereport(ERROR, - (errcode(ERRCODE_INVALID_TABLE_DEFINITION), - errmsg("cannot add constraint to only the partitioned table when partitions exist"), - errhint("Do not specify the ONLY keyword."))); + newcmd->subtype = AT_CheckNotNull; + newcmd->name = pstrdup(cmd->name); + ATSimpleRecursion(wqueue, rel, newcmd, true, lockmode); } + else + ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode); } /* @@ -6208,6 +6240,46 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel, } /* + * ALTER TABLE ALTER COLUMN CHECK NOT NULL + * + * This doesn't exist in the grammar, but we generate AT_CheckNotNull + * commands against the partitions of a partitioned table if the user + * writes ALTER TABLE ONLY ... SET NOT NULL on the partitioned table, + * or tries to create a primary key on it (which internally creates + * AT_SetNotNull on the partitioned table). Such a command doesn't + * allow us to actually modify any partition, but we want to let it + * go through if the partitions are already properly marked. + * + * In future, this might need to adjust the child table's state, likely + * by incrementing an inheritance count for the attnotnull constraint. + * For now we need only check for the presence of the flag. + */ +static void +ATExecCheckNotNull(AlteredTableInfo *tab, Relation rel, + const char *colName, LOCKMODE lockmode) +{ + HeapTuple tuple; + + tuple = SearchSysCacheAttName(RelationGetRelid(rel), colName); + + if (!HeapTupleIsValid(tuple)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_COLUMN), + errmsg("column \"%s\" of relation \"%s\" does not exist", + colName, RelationGetRelationName(rel)))); + + if (!((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull) + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("constraint must be added to child tables too"), + errdetail("Column \"%s\" of relation \"%s\" is not already NOT NULL.", + colName, RelationGetRelationName(rel)), + errhint("Do not specify the ONLY keyword."))); + + ReleaseSysCache(tuple); +} + +/* * NotNullImpliedByRelConstraints * Does rel's existing constraints imply NOT NULL for the given attribute? */ @@ -11269,6 +11341,16 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd, NIL, con->conname); } + else if (cmd->subtype == AT_SetNotNull) + { + /* + * The parser will create AT_SetNotNull subcommands for + * columns of PRIMARY KEY indexes/constraints, but we need + * not do anything with them here, because the columns' + * NOT NULL marks will already have been propagated into + * the new table definition. + */ + } else elog(ERROR, "unexpected statement subtype: %d", (int) cmd->subtype); @@ -15649,7 +15731,7 @@ AttachPartitionEnsureIndexes(Relation rel, Relation attachrel) IndexStmt *stmt; Oid constraintOid; - stmt = generateClonedIndexStmt(NULL, RelationGetRelid(attachrel), + stmt = generateClonedIndexStmt(NULL, idxRel, attmap, RelationGetDescr(rel)->natts, &constraintOid); diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index 674f4b9..2399690 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -293,8 +293,10 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString) } /* - * transformIndexConstraints wants cxt.alist to contain only index - * statements, so transfer anything we already have into save_alist. + * Transfer anything we already have in cxt.alist into save_alist, to keep + * it separate from the output of transformIndexConstraints. (This may + * not be necessary anymore, but we'll keep doing it to preserve the + * historical order of execution of the alist commands.) */ save_alist = cxt.alist; cxt.alist = NIL; @@ -1196,9 +1198,10 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla parent_index = index_open(parent_index_oid, AccessShareLock); /* Build CREATE INDEX statement to recreate the parent_index */ - index_stmt = generateClonedIndexStmt(cxt->relation, InvalidOid, + index_stmt = generateClonedIndexStmt(cxt->relation, parent_index, - attmap, tupleDesc->natts, NULL); + attmap, tupleDesc->natts, + NULL); /* Copy comment on index, if requested */ if (table_like_clause->options & CREATE_TABLE_LIKE_COMMENTS) @@ -1311,13 +1314,26 @@ transformOfType(CreateStmtContext *cxt, TypeName *ofTypename) /* * Generate an IndexStmt node using information from an already existing index - * "source_idx", for the rel identified either by heapRel or heapRelid. + * "source_idx". * - * Attribute numbers should be adjusted according to attmap. + * heapRel is stored into the IndexStmt's relation field, but we don't use it + * otherwise; some callers pass NULL, if they don't need it to be valid. + * (The target relation might not exist yet, so we mustn't try to access it.) + * + * Attribute numbers in expression Vars are adjusted according to attmap. + * + * If constraintOid isn't NULL, we store the OID of any constraint associated + * with the index there. + * + * Unlike transformIndexConstraint, we don't make any effort to force primary + * key columns to be NOT NULL. The larger cloning process this is part of + * should have cloned their NOT NULL status separately (and DefineIndex will + * complain if that fails to happen). */ IndexStmt * -generateClonedIndexStmt(RangeVar *heapRel, Oid heapRelid, Relation source_idx, - const AttrNumber *attmap, int attmap_length, Oid *constraintOid) +generateClonedIndexStmt(RangeVar *heapRel, Relation source_idx, + const AttrNumber *attmap, int attmap_length, + Oid *constraintOid) { Oid source_relid = RelationGetRelid(source_idx); HeapTuple ht_idxrel; @@ -1337,8 +1353,8 @@ generateClonedIndexStmt(RangeVar *heapRel, Oid heapRelid, Relation source_idx, Datum datum; bool isnull; - Assert((heapRel == NULL && OidIsValid(heapRelid)) || - (heapRel != NULL && !OidIsValid(heapRelid))); + if (constraintOid) + *constraintOid = InvalidOid; /* * Fetch pg_class tuple of source index. We can't use the copy in the @@ -1821,6 +1837,7 @@ transformIndexConstraints(CreateStmtContext *cxt) { IndexStmt *index; List *indexlist = NIL; + List *finalindexlist = NIL; ListCell *lc; /* @@ -1869,11 +1886,10 @@ transformIndexConstraints(CreateStmtContext *cxt) * XXX in ALTER TABLE case, it'd be nice to look for duplicate * pre-existing indexes, too. */ - Assert(cxt->alist == NIL); if (cxt->pkey != NULL) { /* Make sure we keep the PKEY index in preference to others... */ - cxt->alist = list_make1(cxt->pkey); + finalindexlist = list_make1(cxt->pkey); } foreach(lc, indexlist) @@ -1883,11 +1899,11 @@ transformIndexConstraints(CreateStmtContext *cxt) index = lfirst(lc); - /* if it's pkey, it's already in cxt->alist */ + /* if it's pkey, it's already in finalindexlist */ if (index == cxt->pkey) continue; - foreach(k, cxt->alist) + foreach(k, finalindexlist) { IndexStmt *priorindex = lfirst(k); @@ -1915,19 +1931,32 @@ transformIndexConstraints(CreateStmtContext *cxt) } if (keep) - cxt->alist = lappend(cxt->alist, index); + finalindexlist = lappend(finalindexlist, index); } + + /* + * Now append all the IndexStmts to cxt->alist. If we generated an ALTER + * TABLE SET NOT NULL statement to support a primary key, it's already in + * cxt->alist. + */ + cxt->alist = list_concat(cxt->alist, finalindexlist); } /* * transformIndexConstraint * Transform one UNIQUE, PRIMARY KEY, or EXCLUDE constraint for * transformIndexConstraints. + * + * We return an IndexStmt. For a PRIMARY KEY constraint, we additionally + * produce NOT NULL constraints, either by marking ColumnDefs in cxt->columns + * as is_not_null or by adding an ALTER TABLE SET NOT NULL command to + * cxt->alist. */ static IndexStmt * transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt) { IndexStmt *index; + List *notnullcmds = NIL; ListCell *lc; index = makeNode(IndexStmt); @@ -2170,9 +2199,7 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt) * For UNIQUE and PRIMARY KEY, we just have a list of column names. * * Make sure referenced keys exist. If we are making a PRIMARY KEY index, - * also make sure they are NOT NULL, if possible. (Although we could leave - * it to DefineIndex to mark the columns NOT NULL, it's more efficient to - * get it right the first time.) + * also make sure they are NOT NULL. */ else { @@ -2180,11 +2207,12 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt) { char *key = strVal(lfirst(lc)); bool found = false; + bool forced_not_null = false; ColumnDef *column = NULL; ListCell *columns; IndexElem *iparam; - /* Make sure referenced column exist. */ + /* Make sure referenced column exists. */ foreach(columns, cxt->columns) { column = castNode(ColumnDef, lfirst(columns)); @@ -2196,9 +2224,18 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt) } if (found) { - /* found column in the new table; force it to be NOT NULL */ - if (constraint->contype == CONSTR_PRIMARY) + /* + * column is defined in the new table. For PRIMARY KEY, we + * can apply the NOT NULL constraint cheaply here ... unless + * the column is marked is_from_type, in which case marking it + * here would be ineffective (see MergeAttributes). + */ + if (constraint->contype == CONSTR_PRIMARY && + !column->is_from_type) + { column->is_not_null = true; + forced_not_null = true; + } } else if (SystemAttributeByName(key) != NULL) { @@ -2242,10 +2279,11 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt) found = true; /* - * We currently have no easy way to force an - * inherited column to be NOT NULL at creation, if - * its parent wasn't so already. We leave it to - * DefineIndex to fix things up in this case. + * It's tempting to set forced_not_null if the + * parent column is already NOT NULL, but that + * seems unsafe because the column's NOT NULL + * marking might disappear between now and + * execution. Do the runtime check to be safe. */ break; } @@ -2259,8 +2297,7 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt) /* * In the ALTER TABLE case, don't complain about index keys not * created in the command; they may well exist already. - * DefineIndex will complain about them if not, and will also take - * care of marking them NOT NULL. + * DefineIndex will complain about them if not. */ if (!found && !cxt->isalter) ereport(ERROR, @@ -2299,10 +2336,29 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt) iparam->ordering = SORTBY_DEFAULT; iparam->nulls_ordering = SORTBY_NULLS_DEFAULT; index->indexParams = lappend(index->indexParams, iparam); + + /* + * For a primary-key column, also create an item for ALTER TABLE + * SET NOT NULL if we couldn't ensure it via is_not_null above. + */ + if (constraint->contype == CONSTR_PRIMARY && !forced_not_null) + { + AlterTableCmd *notnullcmd = makeNode(AlterTableCmd); + + notnullcmd->subtype = AT_SetNotNull; + notnullcmd->name = pstrdup(key); + notnullcmds = lappend(notnullcmds, notnullcmd); + } } } - /* Add included columns to index definition */ + /* + * Add included columns to index definition. This is much like the + * simple-column-name-list code above, except that we don't worry about + * NOT NULL marking; included columns in a primary key should not be + * forced NOT NULL. We don't complain about duplicate columns, either, + * though maybe we should? + */ foreach(lc, constraint->including) { char *key = strVal(lfirst(lc)); @@ -2327,8 +2383,7 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt) { /* * column will be a system column in the new table, so accept - * it. System columns can't ever be null, so no need to worry - * about PRIMARY/NOT NULL constraint. + * it. */ found = true; } @@ -2363,13 +2418,6 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt) if (strcmp(key, inhname) == 0) { found = true; - - /* - * We currently have no easy way to force an - * inherited column to be NOT NULL at creation, if - * its parent wasn't so already. We leave it to - * DefineIndex to fix things up in this case. - */ break; } } @@ -2383,8 +2431,7 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt) /* * In the ALTER TABLE case, don't complain about index keys not * created in the command; they may well exist already. DefineIndex - * will complain about them if not, and will also take care of marking - * them NOT NULL. + * will complain about them if not. */ if (!found && !cxt->isalter) ereport(ERROR, @@ -2402,6 +2449,22 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt) index->indexIncludingParams = lappend(index->indexIncludingParams, iparam); } + /* + * If we found anything that requires run-time SET NOT NULL, build a full + * ALTER TABLE command for that and add it to cxt->alist. + */ + if (notnullcmds) + { + AlterTableStmt *alterstmt = makeNode(AlterTableStmt); + + alterstmt->relation = copyObject(cxt->relation); + alterstmt->cmds = notnullcmds; + alterstmt->relkind = OBJECT_TABLE; + alterstmt->missing_ok = false; + + cxt->alist = lappend(cxt->alist, alterstmt); + } + return index; } @@ -3220,9 +3283,8 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt, } /* - * transformIndexConstraints wants cxt.alist to contain only index - * statements, so transfer anything we already have into save_alist - * immediately. + * Transfer anything we already have in cxt.alist into save_alist, to keep + * it separate from the output of transformIndexConstraints. */ save_alist = cxt.alist; cxt.alist = NIL; @@ -3240,13 +3302,31 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt, */ foreach(l, cxt.alist) { - IndexStmt *idxstmt = lfirst_node(IndexStmt, l); + Node *istmt = (Node *) lfirst(l); - idxstmt = transformIndexStmt(relid, idxstmt, queryString); - newcmd = makeNode(AlterTableCmd); - newcmd->subtype = OidIsValid(idxstmt->indexOid) ? AT_AddIndexConstraint : AT_AddIndex; - newcmd->def = (Node *) idxstmt; - newcmds = lappend(newcmds, newcmd); + /* + * We assume here that cxt.alist contains only IndexStmts and possibly + * ALTER TABLE SET NOT NULL statements generated from primary key + * constraints. We absorb the subcommands of the latter directly. + */ + if (IsA(istmt, IndexStmt)) + { + IndexStmt *idxstmt = (IndexStmt *) istmt; + + idxstmt = transformIndexStmt(relid, idxstmt, queryString); + newcmd = makeNode(AlterTableCmd); + newcmd->subtype = OidIsValid(idxstmt->indexOid) ? AT_AddIndexConstraint : AT_AddIndex; + newcmd->def = (Node *) idxstmt; + newcmds = lappend(newcmds, newcmd); + } + else if (IsA(istmt, AlterTableStmt)) + { + AlterTableStmt *alterstmt = (AlterTableStmt *) istmt; + + newcmds = list_concat(newcmds, alterstmt->cmds); + } + else + elog(ERROR, "unexpected stmt type %d", (int) nodeTag(istmt)); } cxt.alist = NIL; diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 94c0b7a..462237d 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -1764,6 +1764,7 @@ typedef enum AlterTableType AT_ColumnDefault, /* alter column default */ AT_DropNotNull, /* alter column drop not null */ AT_SetNotNull, /* alter column set not null */ + AT_CheckNotNull, /* check column is already marked not null */ AT_SetStatistics, /* alter column set statistics */ AT_SetOptions, /* alter column set ( options ) */ AT_ResetOptions, /* alter column reset ( options ) */ diff --git a/src/include/parser/parse_utilcmd.h b/src/include/parser/parse_utilcmd.h index 09a99d9..6928aef 100644 --- a/src/include/parser/parse_utilcmd.h +++ b/src/include/parser/parse_utilcmd.h @@ -27,7 +27,7 @@ extern void transformRuleStmt(RuleStmt *stmt, const char *queryString, extern List *transformCreateSchemaStmt(CreateSchemaStmt *stmt); extern PartitionBoundSpec *transformPartitionBound(ParseState *pstate, Relation parent, PartitionBoundSpec *spec); -extern IndexStmt *generateClonedIndexStmt(RangeVar *heapRel, Oid heapOid, +extern IndexStmt *generateClonedIndexStmt(RangeVar *heapRel, Relation source_idx, const AttrNumber *attmap, int attmap_length, Oid *constraintOid); diff --git a/src/test/modules/test_ddl_deparse/expected/alter_table.out b/src/test/modules/test_ddl_deparse/expected/alter_table.out index 7da847d..141060f 100644 --- a/src/test/modules/test_ddl_deparse/expected/alter_table.out +++ b/src/test/modules/test_ddl_deparse/expected/alter_table.out @@ -23,8 +23,7 @@ NOTICE: DDL test: type simple, tag CREATE TABLE CREATE TABLE part1 PARTITION OF part FOR VALUES FROM (1) to (100); NOTICE: DDL test: type simple, tag CREATE TABLE ALTER TABLE part ADD PRIMARY KEY (a); -NOTICE: DDL test: type alter table, tag CREATE INDEX +NOTICE: DDL test: type alter table, tag ALTER TABLE NOTICE: subcommand: SET NOT NULL NOTICE: subcommand: SET NOT NULL -NOTICE: DDL test: type alter table, tag ALTER TABLE NOTICE: subcommand: ADD INDEX diff --git a/src/test/modules/test_ddl_deparse/expected/create_table.out b/src/test/modules/test_ddl_deparse/expected/create_table.out index 2d7dfd5..523c996 100644 --- a/src/test/modules/test_ddl_deparse/expected/create_table.out +++ b/src/test/modules/test_ddl_deparse/expected/create_table.out @@ -85,6 +85,8 @@ CREATE TABLE employees OF employee_type ( salary WITH OPTIONS DEFAULT 1000 ); NOTICE: DDL test: type simple, tag CREATE TABLE +NOTICE: DDL test: type alter table, tag ALTER TABLE +NOTICE: subcommand: SET NOT NULL NOTICE: DDL test: type simple, tag CREATE INDEX -- Inheritance CREATE TABLE person ( diff --git a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c index 2fe0c24..7f77f19 100644 --- a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c +++ b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c @@ -117,6 +117,9 @@ get_altertable_subcmdtypes(PG_FUNCTION_ARGS) case AT_SetNotNull: strtype = "SET NOT NULL"; break; + case AT_CheckNotNull: + strtype = "CHECK NOT NULL"; + break; case AT_SetStatistics: strtype = "SET STATS"; break; diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 2a26aa3..3e9d717 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -978,7 +978,7 @@ drop table atacc1; create table atacc1 ( test int ); -- add a primary key constraint (fails) alter table atacc1 add constraint atacc_test1 primary key (test1); -ERROR: column "test1" named in key does not exist +ERROR: column "test1" of relation "atacc1" does not exist drop table atacc1; -- adding a new column as primary key to a non-empty table. -- should fail unless the column has a non-null default value. @@ -990,6 +990,13 @@ ERROR: column "test2" contains null values -- now add a primary key column with a default (succeeds). alter table atacc1 add column test2 int default 0 primary key; drop table atacc1; +-- this combination used to have order-of-execution problems (bug #15580) +create table atacc1 (a int); +insert into atacc1 values(1); +alter table atacc1 + add column b float8 not null default random(), + add primary key(a); +drop table atacc1; -- something a little more complicated create table atacc1 ( test int, test2 int); -- add a primary key constraint @@ -1404,9 +1411,9 @@ ERROR: column "a" does not exist alter table atacc1 rename "........pg.dropped.1........" to x; ERROR: column "........pg.dropped.1........" does not exist alter table atacc1 add primary key(a); -ERROR: column "a" named in key does not exist +ERROR: column "a" of relation "atacc1" does not exist alter table atacc1 add primary key("........pg.dropped.1........"); -ERROR: column "........pg.dropped.1........" named in key does not exist +ERROR: column "........pg.dropped.1........" of relation "atacc1" does not exist alter table atacc1 add unique(a); ERROR: column "a" named in key does not exist alter table atacc1 add unique("........pg.dropped.1........"); @@ -3751,7 +3758,8 @@ ERROR: cannot alter inherited column "b" -- cannot add/drop NOT NULL or check constraints to *only* the parent, when -- partitions exist ALTER TABLE ONLY list_parted2 ALTER b SET NOT NULL; -ERROR: cannot add constraint to only the partitioned table when partitions exist +ERROR: constraint must be added to child tables too +DETAIL: Column "b" of relation "part_2" is not already NOT NULL. HINT: Do not specify the ONLY keyword. ALTER TABLE ONLY list_parted2 ADD CONSTRAINT check_b CHECK (b <> 'zz'); ERROR: constraint must be added to child tables too diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out index f8f3ae8..2286519 100644 --- a/src/test/regress/expected/identity.out +++ b/src/test/regress/expected/identity.out @@ -290,6 +290,18 @@ SELECT seqtypid::regtype FROM pg_sequence WHERE seqrelid = 'itest3_a_seq'::regcl ALTER TABLE itest3 ALTER COLUMN a TYPE text; -- error ERROR: identity column type must be smallint, integer, or bigint +-- kinda silly to change property in the same command, but it should work +ALTER TABLE itest3 + ADD COLUMN c int GENERATED BY DEFAULT AS IDENTITY, + ALTER COLUMN c SET GENERATED ALWAYS; +\d itest3 + Table "public.itest3" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+---------------------------------- + a | integer | | not null | generated by default as identity + b | text | | | + c | integer | | not null | generated always as identity + -- ALTER COLUMN ... SET CREATE TABLE itest6 (a int GENERATED ALWAYS AS IDENTITY, b text); INSERT INTO itest6 DEFAULT VALUES; diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out index e9ac715..c143df5 100644 --- a/src/test/regress/expected/indexing.out +++ b/src/test/regress/expected/indexing.out @@ -1099,6 +1099,21 @@ select indrelid::regclass, indexrelid::regclass, inhparent::regclass, indisvalid (2 rows) drop table idxpart; +-- Related to the above scenario: ADD PRIMARY KEY on the parent mustn't +-- automatically propagate NOT NULL to child columns. +create table idxpart (a int) partition by range (a); +create table idxpart0 (like idxpart); +alter table idxpart0 add unique (a); +alter table idxpart attach partition idxpart0 default; +alter table only idxpart add primary key (a); -- fail, no NOT NULL constraint +ERROR: constraint must be added to child tables too +DETAIL: Column "a" of relation "idxpart0" is not already NOT NULL. +HINT: Do not specify the ONLY keyword. +alter table idxpart0 alter column a set not null; +alter table only idxpart add primary key (a); -- now it works +alter table idxpart0 alter column a drop not null; -- fail, pkey needs it +ERROR: column "a" is marked NOT NULL in parent table +drop table idxpart; -- if a partition has a unique index without a constraint, does not attach -- automatically; creates a new index instead. create table idxpart (a int, b int) partition by range (a); diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 5bda7fe..5e3d6ec 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -749,6 +749,14 @@ alter table atacc1 add column test2 int primary key; alter table atacc1 add column test2 int default 0 primary key; drop table atacc1; +-- this combination used to have order-of-execution problems (bug #15580) +create table atacc1 (a int); +insert into atacc1 values(1); +alter table atacc1 + add column b float8 not null default random(), + add primary key(a); +drop table atacc1; + -- something a little more complicated create table atacc1 ( test int, test2 int); -- add a primary key constraint diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql index 43c2a59..8dcfdf3 100644 --- a/src/test/regress/sql/identity.sql +++ b/src/test/regress/sql/identity.sql @@ -174,6 +174,12 @@ SELECT seqtypid::regtype FROM pg_sequence WHERE seqrelid = 'itest3_a_seq'::regcl ALTER TABLE itest3 ALTER COLUMN a TYPE text; -- error +-- kinda silly to change property in the same command, but it should work +ALTER TABLE itest3 + ADD COLUMN c int GENERATED BY DEFAULT AS IDENTITY, + ALTER COLUMN c SET GENERATED ALWAYS; +\d itest3 + -- ALTER COLUMN ... SET diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql index 7d46e03..cc3d0ab 100644 --- a/src/test/regress/sql/indexing.sql +++ b/src/test/regress/sql/indexing.sql @@ -578,6 +578,18 @@ select indrelid::regclass, indexrelid::regclass, inhparent::regclass, indisvalid order by indexrelid::regclass::text collate "C"; drop table idxpart; +-- Related to the above scenario: ADD PRIMARY KEY on the parent mustn't +-- automatically propagate NOT NULL to child columns. +create table idxpart (a int) partition by range (a); +create table idxpart0 (like idxpart); +alter table idxpart0 add unique (a); +alter table idxpart attach partition idxpart0 default; +alter table only idxpart add primary key (a); -- fail, no NOT NULL constraint +alter table idxpart0 alter column a set not null; +alter table only idxpart add primary key (a); -- now it works +alter table idxpart0 alter column a drop not null; -- fail, pkey needs it +drop table idxpart; + -- if a partition has a unique index without a constraint, does not attach -- automatically; creates a new index instead. create table idxpart (a int, b int) partition by range (a);
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Tom LaneДата:
Сообщение: Re: Odd behavior of partitioned ALTER TABLE ONLY ... ADD PRIMARY KEY