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);