starting to review the Extend NOT NULL representation to pg_constraint patch
Reference: https://commitfest.postgresql.org/action/patch_view?id=312
The patch from http://archives.postgresql.org/message-id/CA2E4C4762EAE28D68404E75@amenophis
does not apply cleanly to the current git master:
$ patch -p1 < extend_not_null.patch
patching file src/backend/catalog/heap.c
patching file q
Hunk #3 succeeded at 290 (offset 1 line).
Hunk #4 succeeded at 351 (offset 1 line).
Hunk #5 succeeded at 530 (offset 1 line).
Hunk #6 FAILED at 2709.
Hunk #7 succeeded at 2957 (offset 18 lines).
Hunk #8 succeeded at 4227 (offset 68 lines).
Hunk #9 succeeded at 4574 (offset 68 lines).
Hunk #10 succeeded at 4584 (offset 68 lines).
Hunk #11 succeeded at 4673 (offset 68 lines).
Hunk #12 succeeded at 6298 (offset 72 lines).
Hunk #13 succeeded at 6312 (offset 72 lines).
Hunk #14 succeeded at 6385 (offset 72 lines).
Hunk #15 succeeded at 7098 (offset 89 lines).
Hunk #16 succeeded at 7860 (offset 89 lines).
Hunk #17 succeeded at 7947 (offset 89 lines).
Hunk #18 succeeded at 8027 (offset 89 lines).
Hunk #19 succeeded at 8075 (offset 89 lines).
Hunk #20 succeeded at 8118 (offset 89 lines).
Hunk #21 succeeded at 8146 (offset 89 lines).
Hunk #22 succeeded at 8163 (offset 89 lines).
Hunk #23 succeeded at 8246 (offset 89 lines).
Hunk #24 succeeded at 8260 (offset 89 lines).
Hunk #25 succeeded at 8310 (offset 89 lines).
Hunk #26 succeeded at 8325 (offset 89 lines).
Hunk #27 succeeded at 8333 (offset 89 lines).
Hunk #28 succeeded at 8387 (offset 89 lines).
Hunk #29 succeeded at 8417 (offset 89 lines).
1 out of 29 hunks FAILED -- saving rejects to file
src/backend/commands/tablecmds.c.rej
patching file src/backend/parser/parse_utilcmd.c
patching file src/backend/port/pg_latch.c
patching file src/backend/utils/adt/ruleutils.c
patching file src/include/catalog/heap.h
patching file src/include/catalog/pg_constraint.h
patching file src/include/nodes/parsenodes.h
Hunk #1 succeeded at 1117 (offset 1 line).
patching file src/test/regress/expected/alter_table.out
patching file src/test/regress/expected/cluster.out
$ cat src/backend/commands/tablecmds.c.rej
*** src/backend/commands/tablecmds.c
--- src/backend/commands/tablecmds.c
*************** ATPrepCmd(List **wqueue, Relation rel, A
*** 2709,2715 ****
break;
case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
ATSimplePermissions(rel, false);
! ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
/* No command-specific prep needed */
pass = AT_PASS_DROP;
break;
--- 2752,2761 ----
break;
case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
ATSimplePermissions(rel, false);
!
! if (recurse)
! cmd->subtype = AT_DropNotNullRecurse;
!
/* No command-specific prep needed */
pass = AT_PASS_DROP;
break;
--On 29. September 2010 23:05:11 -0400 Andrew Geery
<andrew.geery@gmail.com> wrote:
Reference: https://commitfest.postgresql.org/action/patch_view?id=312
The patch from
http://archives.postgresql.org/message-id/CA2E4C4762EAE28D68404E75@amenop
his does not apply cleanly to the current git master:
Yeah, there where some changes in the meantime to the master which generate
some merge failures...will provide a new version along with other fixes
soon. Are you going to update the commitfest page?
Thanks
Bernd
Ok -- I've updated the commitfest page linking in this review and
changing the status to waiting on a new patch from you.
Thanks
Andrew
Show quoted text
On Thu, Sep 30, 2010 at 4:12 AM, Bernd Helmle <mailings@oopsware.de> wrote:
--On 29. September 2010 23:05:11 -0400 Andrew Geery <andrew.geery@gmail.com>
wrote:Reference: https://commitfest.postgresql.org/action/patch_view?id=312
The patch from
http://archives.postgresql.org/message-id/CA2E4C4762EAE28D68404E75@amenop
his does not apply cleanly to the current git master:Yeah, there where some changes in the meantime to the master which generate
some merge failures...will provide a new version along with other fixes
soon. Are you going to update the commitfest page?Thanks
Bernd
--On 30. September 2010 10:12:48 +0200 Bernd Helmle <mailings@oopsware.de>
wrote:
Yeah, there where some changes in the meantime to the master which
generate some merge failures...will provide a new version along with
other fixes soon
Here's a new patch that addresses all DDL commands around NOT NULL
constraints and maintain and follow inheritance information correctly now
(but it lags documentation updates). I hope i haven't introduced nasty bugs
and broke something badly, some deeper testing is welcome.
--
Thanks
Bernd
Attachments:
notnull_constraint.patchapplication/octet-stream; name=notnull_constraint.patchDownload
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index dcc53e1..c976e46 100644
*** a/src/backend/catalog/heap.c
--- b/src/backend/catalog/heap.c
*************** StoreAttrDefault(Relation rel, AttrNumbe
*** 1714,1719 ****
--- 1714,1759 ----
}
/*
+ * Stores a NOT NULL constraint of a column into pg_constraint.
+ */
+ void
+ StoreColumnNotNullConstraint(Relation rel, CookedConstraint *cooked)
+ {
+
+ /*
+ * Store the constraint. Reflect conislocal and coninhcount to
+ * match the same values as the attached column.
+ */
+ CreateConstraintEntry(cooked->name,
+ RelationGetNamespace(rel),
+ CONSTRAINT_NOTNULL,
+ false,
+ false,
+ RelationGetRelid(rel),
+ &(cooked->attnum),
+ 1,
+ InvalidOid,
+ InvalidOid,
+ InvalidOid,
+ NULL,
+ InvalidOid,
+ InvalidOid,
+ InvalidOid,
+ 0,
+ ' ',
+ ' ',
+ ' ',
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ cooked->is_local,
+ cooked->inhcount
+ );
+
+ }
+
+ /*
* Store a check-constraint expression for the given relation.
*
* Caller is responsible for updating the count of constraints
*************** StoreConstraints(Relation rel, List *coo
*** 1837,1842 ****
--- 1877,1885 ----
switch (con->contype)
{
+ case CONSTR_NOTNULL:
+ StoreColumnNotNullConstraint(rel, con);
+ break;
case CONSTR_DEFAULT:
StoreAttrDefault(rel, con->attnum, con->expr);
break;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 403e55a..228258c 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*************** typedef struct NewColumnValue
*** 175,180 ****
--- 175,192 ----
} NewColumnValue;
/*
+ * Struct describing the constraint information
+ * to de-inherit. Currently CHECK and NOT NULL constraints
+ * are carried by ATExecDropInherit() within these struct.
+ */
+ typedef struct DeinheritConstraintInfo
+ {
+ char contype; /* constraint type */
+ AttrNumber attnum; /* if NOT NULL constraint, the attnum */
+ char *conname; /* if CHECK constraint, the constraint name */
+ } DeinheritConstraintInfo;
+
+ /*
* Error-reporting support for RemoveRelations
*/
struct dropmsgstrings
*************** static List *MergeAttributes(List *schem
*** 227,234 ****
List **supOids, List **supconstr, int *supOidCount);
static bool MergeCheckConstraint(List *constraints, char *name, Node *expr);
static bool change_varattnos_walker(Node *node, const AttrNumber *newattno);
! static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel);
! static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel);
static void StoreCatalogInheritance(Oid relationId, List *supers);
static void StoreCatalogInheritance1(Oid relationId, Oid parentOid,
int16 seqNumber, Relation inhRelation);
--- 239,248 ----
List **supOids, List **supconstr, int *supOidCount);
static bool MergeCheckConstraint(List *constraints, char *name, Node *expr);
static bool change_varattnos_walker(Node *node, const AttrNumber *newattno);
! static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel,
! List **child_attnums);
! static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel,
! List *child_attnums);
static void StoreCatalogInheritance(Oid relationId, List *supers);
static void StoreCatalogInheritance1(Oid relationId, Oid parentOid,
int16 seqNumber, Relation inhRelation);
*************** static void ATExecAddColumn(AlteredTable
*** 276,285 ****
ColumnDef *colDef, bool isOid, LOCKMODE lockmode);
static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid);
static void ATPrepAddOids(List **wqueue, Relation rel, bool recurse,
! AlterTableCmd *cmd, LOCKMODE lockmode);
! static void ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode);
! static void ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
! const char *colName, LOCKMODE lockmode);
static void ATExecColumnDefault(Relation rel, const char *colName,
Node *newDefault, LOCKMODE lockmode);
static void ATPrepSetStatistics(Relation rel, const char *colName,
--- 290,304 ----
ColumnDef *colDef, bool isOid, LOCKMODE lockmode);
static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid);
static void ATPrepAddOids(List **wqueue, Relation rel, bool recurse,
! AlterTableCmd *cmd, LOCKMODE lockmode);
! static void ATExecDropNotNull(Relation rel, const char *colName, DropBehavior behavior,
! bool recurse, bool recursing, LOCKMODE lockmode);
! static void ATExecSetNotNull(List **wqueue, AlteredTableInfo *tab, Relation rel,
! const char *colName, bool recurse, bool recursing,
! LOCKMODE lockmode);
! static void ATExecSetNotNullInternal(Relation rel, Relation attr_rel,
! HeapTuple atttup, bool *is_new_constraint,
! bool is_local);
static void ATExecColumnDefault(Relation rel, const char *colName,
Node *newDefault, LOCKMODE lockmode);
static void ATPrepSetStatistics(Relation rel, const char *colName,
*************** static void ATExecDropInherit(Relation r
*** 336,342 ****
static void copy_relation_data(SMgrRelation rel, SMgrRelation dst,
ForkNumber forkNum, bool istemp);
static const char *storage_name(char c);
!
/* ----------------------------------------------------------------
* DefineRelation
--- 355,368 ----
static void copy_relation_data(SMgrRelation rel, SMgrRelation dst,
ForkNumber forkNum, bool istemp);
static const char *storage_name(char c);
! static bool ATExecDropNotNullInternal(Relation rel, HeapTuple constrtup,
! DropBehavior behavior,
! bool recurse, bool recursing);
! static bool
! CheckNotNullOnAttributeName(Relation rel, const char *colname,
! AttrNumber *attnum);
! static void
! DropNotNullOnAttributeNum(Relation rel, AttrNumber attnum, bool lock);
/* ----------------------------------------------------------------
* DefineRelation
*************** DefineRelation(CreateStmt *stmt, char re
*** 508,513 ****
--- 534,560 ----
attnum++;
+ if (colDef->is_not_null) {
+ /*
+ * Adjust NOT NULL constraint of this column
+ * to hold new attnum and inheritance information
+ *
+ * XXX: Currently the constraint is cheated into cookedDefaults
+ * list, maybe we need to invent a different machinerie/naming.
+ */
+ CookedConstraint *cooked;
+
+ cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint));
+ cooked->contype = CONSTR_NOTNULL;
+ cooked->name = ChooseConstraintName(relname, NameStr(descriptor->attrs[attnum - 1]->attname),
+ "not_null", namespaceId, NIL);
+ cooked->attnum = attnum;
+ cooked->expr = NULL;
+ cooked->is_local = colDef->is_local;
+ cooked->inhcount = colDef->inhcount;
+ cookedDefaults = lappend(cookedDefaults, cooked);
+ }
+
if (colDef->raw_default != NULL)
{
RawColumnDefault *rawEnt;
*************** ATPrepCmd(List **wqueue, Relation rel, A
*** 2682,2696 ****
/* No command-specific prep needed */
pass = cmd->def ? AT_PASS_ADD_CONSTR : AT_PASS_DROP;
break;
! case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
ATSimplePermissions(rel, false, false);
! 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, false, false);
! ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
/* No command-specific prep needed */
pass = AT_PASS_ADD_CONSTR;
break;
--- 2729,2748 ----
/* No command-specific prep needed */
pass = cmd->def ? AT_PASS_ADD_CONSTR : AT_PASS_DROP;
break;
! case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
ATSimplePermissions(rel, false, false);
!
! if (recurse)
! cmd->subtype = AT_DropNotNullRecurse;
/* No command-specific prep needed */
pass = AT_PASS_DROP;
break;
case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */
ATSimplePermissions(rel, false, false);
!
! if (recurse)
! cmd->subtype = AT_SetNotNullRecurse;
!
/* No command-specific prep needed */
pass = AT_PASS_ADD_CONSTR;
break;
*************** ATExecCmd(List **wqueue, AlteredTableInf
*** 2914,2923 ****
ATExecColumnDefault(rel, cmd->name, cmd->def, lockmode);
break;
case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
! ATExecDropNotNull(rel, cmd->name, lockmode);
break;
case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */
! ATExecSetNotNull(tab, rel, cmd->name, lockmode);
break;
case AT_SetStatistics: /* ALTER COLUMN SET STATISTICS */
ATExecSetStatistics(rel, cmd->name, cmd->def, lockmode);
--- 2966,2983 ----
ATExecColumnDefault(rel, cmd->name, cmd->def, lockmode);
break;
case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
! ATExecDropNotNull(rel, cmd->name, cmd->behavior,
! false, false, lockmode);
! break;
! case AT_DropNotNullRecurse: /* ALTER COLUMN DROP NOT NULL with recursion */
! ATExecDropNotNull(rel, cmd->name, cmd->behavior,
! true, false, lockmode);
break;
case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */
! ATExecSetNotNull(wqueue, tab, rel, cmd->name, false, false, lockmode);
! break;
! case AT_SetNotNullRecurse: /* ALTER COLUMN SET NOT NULL recursing */
! ATExecSetNotNull(wqueue, tab, rel, cmd->name, true, false, lockmode);
break;
case AT_SetStatistics: /* ALTER COLUMN SET STATISTICS */
ATExecSetStatistics(rel, cmd->name, cmd->def, lockmode);
*************** ATRewriteTable(AlteredTableInfo *tab, Oi
*** 3441,3448 ****
if (heap_attisnull(tuple, attn + 1))
ereport(ERROR,
(errcode(ERRCODE_NOT_NULL_VIOLATION),
! errmsg("column \"%s\" contains null values",
! NameStr(newTupDesc->attrs[attn]->attname))));
}
foreach(l, tab->constraints)
--- 3501,3509 ----
if (heap_attisnull(tuple, attn + 1))
ereport(ERROR,
(errcode(ERRCODE_NOT_NULL_VIOLATION),
! errmsg("column \"%s\" of relation \"%s\" contains null values",
! NameStr(newTupDesc->attrs[attn]->attname),
! RelationGetRelationName(oldrel))));
}
foreach(l, tab->constraints)
*************** ATPrepAddOids(List **wqueue, Relation re
*** 4179,4225 ****
* ALTER TABLE ALTER COLUMN DROP NOT NULL
*/
static void
! ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
{
! HeapTuple tuple;
AttrNumber attnum;
! Relation attr_rel;
! List *indexoidlist;
! ListCell *indexoidscan;
/*
! * lookup the attribute
*/
! attr_rel = heap_open(AttributeRelationId, RowExclusiveLock);
! tuple = SearchSysCacheCopyAttName(RelationGetRelid(rel), colName);
! if (!HeapTupleIsValid(tuple))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" of relation \"%s\" does not exist",
! colName, RelationGetRelationName(rel))));
! attnum = ((Form_pg_attribute) GETSTRUCT(tuple))->attnum;
/* Prevent them from altering a system attribute */
! if (attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg("cannot alter system column \"%s\"",
! colName)));
/*
! * Check that the attribute is not in a primary key
*/
/* Loop over all indexes on the relation */
indexoidlist = RelationGetIndexList(rel);
!
foreach(indexoidscan, indexoidlist)
{
! Oid indexoid = lfirst_oid(indexoidscan);
! HeapTuple indexTuple;
Form_pg_index indexStruct;
int i;
--- 4240,4585 ----
* ALTER TABLE ALTER COLUMN DROP NOT NULL
*/
static void
! ATExecDropNotNull(Relation rel, const char *colName, DropBehavior behavior,
! bool recurse, bool recursing, LOCKMODE lockmode)
{
! List *children;
! ListCell *child;
! HeapTuple constr_tuple;
AttrNumber attnum;
! Relation constr_rel;
! SysScanDesc scan;
! ScanKeyData key;
! bool found;
!
! if (recursing)
! ATSimplePermissions(rel, false, false);
/*
! * Lookup the attribute. This also checks for a dropped column
! * or any attempts to alter a system column. Returns false in case
! * no further work needs to be done (e.g. no NOT NULL present).
*/
! if (!CheckNotNullOnAttributeName(rel, colName, &attnum))
! return;
! /*
! * Scan through our constraint records, looking for the matching
! * NOT NULL constraint.
! */
! constr_rel = heap_open(ConstraintRelationId, RowExclusiveLock);
! ScanKeyInit(&key,
! Anum_pg_constraint_conrelid,
! BTEqualStrategyNumber, F_OIDEQ,
! ObjectIdGetDatum(RelationGetRelid(rel)));
! scan = systable_beginscan(constr_rel, ConstraintRelidIndexId,
! true, SnapshotNow, 1, &key);
!
! while(HeapTupleIsValid(constr_tuple = systable_getnext(scan)))
! {
! Form_pg_constraint constrStruct;
! Datum arrayp;
! Datum *keyvals;
! int nelems;
! bool isnull;
! constrStruct = (Form_pg_constraint) GETSTRUCT(constr_tuple);
!
! if (constrStruct->contype != CONSTRAINT_NOTNULL)
! continue;
!
! /*
! * Check wether this constraint is attached to the given column.
! * ATExecSetNotNull() ensures that only one NOT NULL constraint tuple
! * lives per attribute.
! */
! arrayp = SysCacheGetAttr(CONSTROID, constr_tuple, Anum_pg_constraint_conkey, &isnull);
!
! /* should not happen */
! Assert(!isnull);
!
! deconstruct_array(DatumGetArrayTypeP(arrayp), INT2OID, 2, true,
! 's', &keyvals, NULL, &nelems);
!
! /* Is this NOT NULL constraint attached to the current tuple? */
! if (DatumGetInt16(keyvals[0]) != attnum)
! continue;
!
! /*
! * Check if the attribute is inherited from another relation or
! * part of a primary key.
! * If true, error out, otherwise perform the deletion of the constraint.
! *
! * Note: ATExecDropNotNullInternal() performs a CCI, if required.
! */
! if ((found = ATExecDropNotNullInternal(rel,
! constr_tuple,
! behavior,
! recurse, recursing)))
! {
! /*
! * Okay, actually remove the NOT NULL from pg_attribute.
! * CheckNotNullOnAttributeName() already holds a lock on pg_attribute, so
! * override an additional lock here.
! */
! DropNotNullOnAttributeNum(rel, attnum, false);
! break;
! }
! }
!
! systable_endscan(scan);
!
! if (found)
! {
! /*
! * Propagate to children as appropriate.
! */
! children = find_inheritance_children(RelationGetRelid(rel),
! AccessExclusiveLock);
! }
! else
! {
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_OBJECT),
! errmsg("column \"%s\" of relation \"%s\" has no NOT NULL constraint",
! colName,
! RelationGetRelationName(rel))));
! }
!
! foreach(child, children)
! {
! Oid childrelid = lfirst_oid(child);
! Relation childrel;
!
! childrel = heap_open(childrelid, NoLock);
! CheckTableNotInUse(childrel, "ALTER TABLE");
!
! ScanKeyInit(&key,
! Anum_pg_constraint_conrelid,
! BTEqualStrategyNumber, F_OIDEQ,
! ObjectIdGetDatum(childrelid));
! scan = systable_beginscan(constr_rel, ConstraintRelidIndexId,
! true, SnapshotNow, 1, &key);
!
! while (HeapTupleIsValid(constr_tuple = systable_getnext(scan)))
! {
! HeapTuple copy_tuple;
! Form_pg_constraint constrStruct;
! Datum arrayp;
! Datum *keyvals;
! int nelems;
! bool isnull;
!
! constrStruct = (Form_pg_constraint) GETSTRUCT(constr_tuple);
!
! if (constrStruct->contype != CONSTRAINT_NOTNULL)
! continue;
!
! /*
! * Check wether this constraint is attached to the given column.
! * ATExecSetNotNull() ensures that only one NOT NULL constraint tuple
! * lives per attribute.
! */
! arrayp = SysCacheGetAttr(CONSTROID, constr_tuple, Anum_pg_constraint_conkey, &isnull);
!
! /* should not happen */
! Assert(!isnull);
!
! deconstruct_array(DatumGetArrayTypeP(arrayp), INT2OID, 2, true,
! 's', &keyvals, NULL, &nelems);
!
! /* Is this NOT NULL constraint attached to the current tuple? */
! if (DatumGetInt16(keyvals[0]) != attnum)
! continue;
!
! found = true;
!
! if (constrStruct->coninhcount <= 0) /* shouldn't happen */
! elog(ERROR, "relation %u has non-inherited NOT NULL constraint \"%s\"",
! childrelid, NameStr(constrStruct->conname));
!
! copy_tuple = heap_copytuple(constr_tuple);
! constrStruct = (Form_pg_constraint) GETSTRUCT(copy_tuple);
!
! if (recurse)
! {
! /*
! * If the child constraint has other definition sources, just
! * decrement its inheritance count; if not, recurse to delete
! * it.
! */
! if (constrStruct->coninhcount == 1 && !constrStruct->conislocal)
! {
! /* Time to delete this child constraint, too */
! ATExecDropNotNull(childrel, colName, behavior,
! true, true, lockmode);
! }
! else
! {
! /* Child constraint must survive my deletion */
! constrStruct->coninhcount--;
! simple_heap_update(constr_rel, ©_tuple->t_self, copy_tuple);
! CatalogUpdateIndexes(constr_rel, copy_tuple);
!
! /* Make update visible */
! CommandCounterIncrement();
! }
! }
! else
! {
! /*
! * If we were told to drop ONLY in this table (no recursion),
! * we need to mark the inheritors' constraints as locally
! * defined rather than inherited.
! */
! constrStruct->coninhcount--;
! constrStruct->conislocal = true;
!
! simple_heap_update(constr_rel, ©_tuple->t_self, copy_tuple);
! CatalogUpdateIndexes(constr_rel, copy_tuple);
!
! /* Make update visible */
! CommandCounterIncrement();
! }
!
! heap_freetuple(copy_tuple);
!
! }
!
! systable_endscan(scan);
! heap_close(childrel, NoLock);
!
! }
!
! heap_close(constr_rel, NoLock);
!
! }
!
! /*
! * Does the leg work on dropping a NOT NULL constraint
! * identified by the given constrtup tuple.
! *
! * Note: The caller is responsible to pass a valid
! * CONSTRAINT_NOTNULL tuple.
! */
! static bool
! ATExecDropNotNullInternal(Relation rel, HeapTuple constrtup,
! DropBehavior behavior,
! bool recurse, bool recursing)
! {
! bool found;
! Form_pg_constraint constrStruct;
! ObjectAddress conobj;
!
! found = false;
!
! if (HeapTupleIsValid(constrtup))
! {
!
! constrStruct = (Form_pg_constraint) GETSTRUCT(constrtup);
!
! /*
! * Be paranoid, caller is responsible to pass a valid HeapTuple.
! */
! Assert(constrStruct->contype == CONSTRAINT_NOTNULL);
!
! /* It is okay to drop this constraint when recursing */
! if ((constrStruct->coninhcount > 0) && (!recursing))
! {
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
! errmsg("cannot drop inherited NOT NULL constraint \"%s\", relation \"%s\"",
! NameStr(constrStruct->conname), RelationGetRelationName(rel))));
! }
!
! /*
! * Time to drop the constraint now.
! */
! conobj.classId = ConstraintRelationId;
! conobj.objectId = HeapTupleGetOid(constrtup);
! conobj.objectSubId = 0;
!
! performDeletion(&conobj, behavior);
!
! /*
! * Make sure changes are visible
! */
! CommandCounterIncrement();
! found = true;
!
! }
!
! return found;
! }
!
! /*
! * Checks wether the given attribute name has a NOT NULL constraint attached
! * which can be dropped.
! *
! * Any attempts to drop a NOT NULL constraint on a system
! * column or non-existing column will throw an error. Returns true
! * in case the caller is allowed to proceed, false if no NOT NULL flag
! * was set on the attribute.
! */
! static bool
! CheckNotNullOnAttributeName(Relation rel, const char *colname, AttrNumber *attnum)
! {
! Relation attr_rel;
! HeapTuple attr_tuple;
! Form_pg_attribute attr_struct;
! bool has_not_null;
! List *indexoidlist;
! ListCell *indexoidscan;
!
! has_not_null = false;
!
! attr_rel = heap_open(AttributeRelationId, RowExclusiveLock);
! attr_tuple = SearchSysCacheCopyAttName(RelationGetRelid(rel), colname);
!
! if (!HeapTupleIsValid(attr_tuple))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" of relation \"%s\" does not exist",
! colname,
! RelationGetRelationName(rel))));
! /* attribute exists */
! attr_struct = (Form_pg_attribute) GETSTRUCT(attr_tuple);
/* Prevent them from altering a system attribute */
! if (attr_struct->attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg("cannot alter system column \"%s\" of relation \"%s\"",
! colname,
! RelationGetRelationName(rel))));
/*
! * Check for dropped attributes
*/
+ if (attr_struct->attisdropped)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("attempt to drop NOT NULL constraint on dropped column \"%s\" of relation \"%s\"",
+ colname,
+ RelationGetRelationName(rel))));
+ }
+
+ if (attnum)
+ *attnum = attr_struct->attnum;
+ /*
+ * Check that the attribute is not in a primary key
+ */
+
/* Loop over all indexes on the relation */
indexoidlist = RelationGetIndexList(rel);
!
foreach(indexoidscan, indexoidlist)
{
! Oid indexoid = lfirst_oid(indexoidscan);
! HeapTuple indexTuple;
Form_pg_index indexStruct;
int i;
*************** ATExecDropNotNull(Relation rel, const ch
*** 4227,4233 ****
if (!HeapTupleIsValid(indexTuple))
elog(ERROR, "cache lookup failed for index %u", indexoid);
indexStruct = (Form_pg_index) GETSTRUCT(indexTuple);
!
/* If the index is not a primary key, skip the check */
if (indexStruct->indisprimary)
{
--- 4587,4593 ----
if (!HeapTupleIsValid(indexTuple))
elog(ERROR, "cache lookup failed for index %u", indexoid);
indexStruct = (Form_pg_index) GETSTRUCT(indexTuple);
!
/* If the index is not a primary key, skip the check */
if (indexStruct->indisprimary)
{
*************** ATExecDropNotNull(Relation rel, const ch
*** 4237,4281 ****
*/
for (i = 0; i < indexStruct->indnatts; i++)
{
! if (indexStruct->indkey.values[i] == attnum)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("column \"%s\" is in a primary key",
! colName)));
}
}
!
ReleaseSysCache(indexTuple);
}
! list_free(indexoidlist);
!
! /*
! * Okay, actually perform the catalog change ... if needed
*/
! if (((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull)
! {
! ((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull = FALSE;
! simple_heap_update(attr_rel, &tuple->t_self, tuple);
! /* keep the system catalog indexes current */
! CatalogUpdateIndexes(attr_rel, tuple);
! }
- heap_close(attr_rel, RowExclusiveLock);
}
/*
* ALTER TABLE ALTER COLUMN SET NOT NULL
*/
static void
! ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
! const char *colName, LOCKMODE lockmode)
{
HeapTuple tuple;
AttrNumber attnum;
Relation attr_rel;
/*
* lookup the attribute
--- 4597,4681 ----
*/
for (i = 0; i < indexStruct->indnatts; i++)
{
! if (indexStruct->indkey.values[i] == attr_struct->attnum)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("column \"%s\" is in a primary key",
! colname)));
}
}
!
ReleaseSysCache(indexTuple);
}
! /*
! * In case no NOT NULL constraint was set on the
! * column, give up and tell the caller we haven't done
! * anything.
*/
! has_not_null = ((!attr_struct->attnotnull) ? false : true);
! heap_freetuple(attr_tuple);
! heap_close(attr_rel, NoLock);
! return has_not_null;
! }
! /*
! * Drops a NOT NULL constraint from the given attribute number of the
! * specified relation.
! */
! static void
! DropNotNullOnAttributeNum(Relation rel, AttrNumber attnum, bool lock)
! {
! Relation attr_rel;
! HeapTuple attr_tuple;
! Form_pg_attribute attr_struct;
!
! if (lock)
! attr_rel = heap_open(AttributeRelationId, RowExclusiveLock);
! else
! attr_rel = heap_open(AttributeRelationId, NoLock);
!
! attr_tuple = SearchSysCacheCopy(ATTNUM,
! ObjectIdGetDatum(RelationGetRelid(rel)),
! Int16GetDatum(attnum),
! 0, 0);
!
! attr_struct = (Form_pg_attribute) GETSTRUCT(attr_tuple);
!
! /* not expected, so be paranoid */
! Assert(attr_struct->attnotnull);
!
! ((Form_pg_attribute) GETSTRUCT(attr_tuple))->attnotnull = FALSE;
! simple_heap_update(attr_rel, &attr_tuple->t_self, attr_tuple);
!
! /* keep the system catalog indexes current */
! CatalogUpdateIndexes(attr_rel, attr_tuple);
!
! heap_close(attr_rel, NoLock);
}
/*
* ALTER TABLE ALTER COLUMN SET NOT NULL
*/
static void
! ATExecSetNotNull(List **wqueue, AlteredTableInfo *tab, Relation rel,
! const char *colName, bool recurse, bool recursing,
! LOCKMODE lockmode)
{
HeapTuple tuple;
AttrNumber attnum;
Relation attr_rel;
+ List *children;
+ ListCell *child;
+ bool is_new_constraint;
+
+ if (recursing)
+ ATSimplePermissions(rel, false, false);
+
+ children = find_inheritance_children(RelationGetRelid(rel),
+ AccessExclusiveLock);
/*
* lookup the attribute
*************** ATExecSetNotNull(AlteredTableInfo *tab,
*** 4296,4321 ****
if (attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg("cannot alter system column \"%s\"",
! colName)));
/*
! * Okay, actually perform the catalog change ... if needed
*/
! if (!((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull)
{
! ((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull = TRUE;
! simple_heap_update(attr_rel, &tuple->t_self, tuple);
! /* keep the system catalog indexes current */
! CatalogUpdateIndexes(attr_rel, tuple);
! /* Tell Phase 3 it needs to test the constraint */
! tab->new_notnull = true;
}
! heap_close(attr_rel, RowExclusiveLock);
}
/*
--- 4696,4907 ----
if (attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg("cannot alter system column \"%s\" of relation \"%s\"",
! colName, RelationGetRelationName(rel))));
/*
! * Treat it an error if there's an attempt to add
! * a NOT NULL constraint to a table but not to
! * existing child tables (thus, ALTER TABLE ONLY
! * was specified to a table inherited from others).
! * Maybe we should treat this as a no op, but it seems
! * better to inform the user that he's doing something
! * wrong.
*/
! if (!recurse)
! {
! if (children)
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
! errmsg("NOT NULL constraint must be added to child tables too")));
! }
!
! /*
! * Okay, actually perform the catalog changes ...
! *
! * Setting attnotnull to TRUE requires to create a
! * constraint tuple in pg_constraint as well. We do this
! * to record wether this constraint was added locally or
! * inherited by some other attribute. We also record the
! * attribute number to determine which column this
! * constraint belongs to. We need to take care wether
! * the constraint was added locally to a child table or
! * we're currently already recursing through an inheritance
! * tree.
! */
! ATExecSetNotNullInternal(rel, attr_rel, tuple, &is_new_constraint, !recursing);
!
! /* Tell Phase 3 it needs to test the constraint */
! tab->new_notnull = true;
!
! heap_freetuple(tuple);
! heap_close(attr_rel, RowExclusiveLock);
!
! /*
! * Loop through all child relations
! */
! foreach(child, children)
{
! Oid childrelid = lfirst_oid(child);
! Relation childrel;
! bool is_new_constraint;
! AlteredTableInfo *childtab;
! attr_rel = heap_open(AttributeRelationId, RowExclusiveLock);
! childrel = heap_open(childrelid, NoLock);
! CheckTableNotInUse(childrel, "ALTER TABLE");
! /* get work queue entry for child relation */
! childtab = ATGetQueueEntry(wqueue, childrel);
! /*
! * Look for inherited column
! */
! tuple = SearchSysCacheCopyAttName(childrelid, colName);
!
! /* shouldn't happen */
! Assert(HeapTupleIsValid(tuple));
!
! /* perform the actual work */
! ATExecSetNotNull(wqueue, childtab, childrel,
! colName, recurse, true, lockmode);
!
! heap_freetuple(tuple);
! heap_close(attr_rel, NoLock);
! heap_close(childrel, NoLock);
}
+ }
! /*
! * Internal function to ATExecSetNotNull()
! *
! * Takes a relation and the pg_attribute relation and tuple of the
! * target column to create a NOT NULL constraint.
! * The caller is responsible to pass a valid HeapTuple.
! */
! static void
! ATExecSetNotNullInternal(Relation rel, Relation attr_rel,
! HeapTuple atttup, bool *is_new_constraint, bool is_local)
! {
! Form_pg_attribute attr;
!
! attr = (Form_pg_attribute) GETSTRUCT(atttup);
! *is_new_constraint = FALSE;
!
! if (attr->attnotnull)
! {
! SysScanDesc scan;
! ScanKeyData key;
! HeapTuple constr_tup;
! Relation constr_rel;
! bool found;
!
! /*
! * Column has already a NOT NULL constraint attached,
! * fetch its pg_constraint tuple and adjust the inheritance
! * information according to its pg_attribute tuple.
! */
! constr_rel = heap_open(ConstraintRelationId, RowExclusiveLock);
! ScanKeyInit(&key,
! Anum_pg_constraint_conrelid,
! BTEqualStrategyNumber, F_OIDEQ,
! ObjectIdGetDatum(RelationGetRelid(rel)));
! scan = systable_beginscan(constr_rel, ConstraintRelidIndexId,
! true, SnapshotNow, 1, &key);
!
! while (HeapTupleIsValid(constr_tup = systable_getnext(scan)))
! {
! Form_pg_constraint constr_struct;
! Datum arrayp;
! Datum *keyvals;
! int nelems;
! bool isnull;
!
! constr_struct = (Form_pg_constraint) GETSTRUCT(constr_tup);
!
! /* We need to look for NOT NULL constraint only */
! if (constr_struct->contype != CONSTRAINT_NOTNULL)
! continue;
!
! /* constraint tuple attached to our column? */
! arrayp = SysCacheGetAttr(CONSTROID, constr_tup,
! Anum_pg_constraint_conkey,
! &isnull);
! /* not expected here */
! Assert(!isnull);
!
! deconstruct_array(DatumGetArrayTypeP(arrayp), INT2OID, 2, true,
! 's', &keyvals, NULL, &nelems);
! if (DatumGetInt16(keyvals[0]) != attr->attnum)
! continue;
!
! /* only reached in case we have found the required constraint tuple */
! found = TRUE;
! break;
! }
!
! /* there should be a NOT NULL constraint tuple */
! if (found)
! {
! HeapTuple copy_tuple = heap_copytuple(constr_tup);
! Form_pg_constraint constr = (Form_pg_constraint) GETSTRUCT(copy_tuple);
!
! /*
! * We don't need to update the constraint tuple when
! * the inheritance counter is already up to date
! */
! if (constr->coninhcount != attr->attinhcount)
! {
! constr->coninhcount = is_local ? 0 : attr->attinhcount;
! simple_heap_update(constr_rel, ©_tuple->t_self, copy_tuple);
!
! /*
! * Keep system catalog indexes current and make
! * changes visible
! */
! CatalogUpdateIndexes(constr_rel, copy_tuple);
! CommandCounterIncrement();
! *is_new_constraint = TRUE;
! }
!
! heap_freetuple(copy_tuple);
! }
!
! systable_endscan(scan);
! heap_close(constr_rel, NoLock);
!
! }
! else
! {
! CookedConstraint *cooked;
!
! /*
! * New NOT NULL constraint
! */
! cooked = palloc(sizeof(CookedConstraint));
! cooked->contype = CONSTR_NOTNULL;
! cooked->expr = NULL;
! cooked->name = ChooseConstraintName(RelationGetRelationName(rel),
! NameStr(attr->attname),
! "not_null", RelationGetNamespace(rel),
! NIL);
! cooked->attnum = attr->attnum;
! cooked->is_local = is_local;
! cooked->inhcount = is_local ? 0 : 1;
!
! StoreColumnNotNullConstraint(rel, cooked);
!
! /* Make changes visible */
! CommandCounterIncrement();
!
! attr->attnotnull = TRUE;
! simple_heap_update(attr_rel, &atttup->t_self, atttup);
!
! /* keep the system catalog indexes current */
! CatalogUpdateIndexes(attr_rel, atttup);
!
! *is_new_constraint = TRUE;
! }
}
/*
*************** ATExecDropConstraint(Relation rel, const
*** 5893,5898 ****
--- 6479,6488 ----
con = (Form_pg_constraint) GETSTRUCT(tuple);
+ /* NOT NULL is handled by ALTER TABLE ... DROP/SET NOT NULL */
+ if (con->contype == CONSTRAINT_NOTNULL)
+ continue;
+
if (strcmp(NameStr(con->conname), constrName) != 0)
continue;
*************** ATExecDropConstraint(Relation rel, const
*** 5903,5909 ****
errmsg("cannot drop inherited constraint \"%s\" of relation \"%s\"",
constrName, RelationGetRelationName(rel))));
! /* Right now only CHECK constraints can be inherited */
if (con->contype == CONSTRAINT_CHECK)
is_check_constraint = true;
--- 6493,6501 ----
errmsg("cannot drop inherited constraint \"%s\" of relation \"%s\"",
constrName, RelationGetRelationName(rel))));
! /* This is a CHECK constraint, remember that we found one
! * and proceed to drop it
! */
if (con->contype == CONSTRAINT_CHECK)
is_check_constraint = true;
*************** ATExecDropConstraint(Relation rel, const
*** 5974,5980 ****
con = (Form_pg_constraint) GETSTRUCT(tuple);
! /* Right now only CHECK constraints can be inherited */
if (con->contype != CONSTRAINT_CHECK)
continue;
--- 6566,6575 ----
con = (Form_pg_constraint) GETSTRUCT(tuple);
! /* Besides CHECK constraint we support inheritance of
! * NOT NULL constraints, too. Ignore them here, since they
! * are handled by ALTER TABLE...DROP/SET NOT NULL
! */
if (con->contype != CONSTRAINT_CHECK)
continue;
*************** ATPostAlterTypeParse(char *cmd, List **w
*** 6701,6706 ****
--- 7296,7302 ----
tab->subcmds[AT_PASS_OLD_INDEX] =
lappend(tab->subcmds[AT_PASS_OLD_INDEX], cmd);
break;
+ case AT_SetNotNull:
case AT_AddConstraint:
tab->subcmds[AT_PASS_OLD_CONSTR] =
lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
*************** ATExecAddInherit(Relation child_rel, Ran
*** 7462,7467 ****
--- 8058,8064 ----
HeapTuple inheritsTuple;
int32 inhseqno;
List *children;
+ List *child_attnums;
/*
* AccessShareLock on the parent is what's obtained during normal CREATE
*************** ATExecAddInherit(Relation child_rel, Ran
*** 7548,7557 ****
RelationGetRelationName(parent_rel))));
/* Match up the columns and bump attinhcount as needed */
! MergeAttributesIntoExisting(child_rel, parent_rel);
/* Match up the constraints and bump coninhcount as needed */
! MergeConstraintsIntoExisting(child_rel, parent_rel);
/*
* OK, it looks valid. Make the catalog entries that show inheritance.
--- 8145,8155 ----
RelationGetRelationName(parent_rel))));
/* Match up the columns and bump attinhcount as needed */
! child_attnums = NIL;
! MergeAttributesIntoExisting(child_rel, parent_rel, &child_attnums);
/* Match up the constraints and bump coninhcount as needed */
! MergeConstraintsIntoExisting(child_rel, parent_rel, child_attnums);
/*
* OK, it looks valid. Make the catalog entries that show inheritance.
*************** constraints_equivalent(HeapTuple a, Heap
*** 7627,7633 ****
* the child must be as well. Defaults are not compared, however.
*/
static void
! MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel)
{
Relation attrrel;
AttrNumber parent_attno;
--- 8225,8232 ----
* the child must be as well. Defaults are not compared, however.
*/
static void
! MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel,
! List **attnums_list)
{
Relation attrrel;
AttrNumber parent_attno;
*************** MergeAttributesIntoExisting(Relation chi
*** 7674,7679 ****
--- 8273,8283 ----
attributeName)));
/*
+ * Record the child's inherited attnum in attnums_list.
+ */
+ *attnums_list = lappend_int(*attnums_list, childatt->attnum);
+
+ /*
* OK, bump the child column's inheritance count. (If we fail
* later on, this change will just roll back.)
*/
*************** MergeAttributesIntoExisting(Relation chi
*** 7712,7718 ****
* a problem though. Even 100 constraints ought not be the end of the world.
*/
static void
! MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel)
{
Relation catalog_relation;
TupleDesc tuple_desc;
--- 8316,8323 ----
* a problem though. Even 100 constraints ought not be the end of the world.
*/
static void
! MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel,
! List *child_attnums)
{
Relation catalog_relation;
TupleDesc tuple_desc;
*************** MergeConstraintsIntoExisting(Relation ch
*** 7739,7745 ****
HeapTuple child_tuple;
bool found = false;
! if (parent_con->contype != CONSTRAINT_CHECK)
continue;
/* Search for a child constraint matching this one */
--- 8344,8351 ----
HeapTuple child_tuple;
bool found = false;
! if (parent_con->contype != CONSTRAINT_CHECK
! && parent_con->contype != CONSTRAINT_NOTNULL)
continue;
/* Search for a child constraint matching this one */
*************** MergeConstraintsIntoExisting(Relation ch
*** 7755,7773 ****
Form_pg_constraint child_con = (Form_pg_constraint) GETSTRUCT(child_tuple);
HeapTuple child_copy;
! if (child_con->contype != CONSTRAINT_CHECK)
! continue;
! if (strcmp(NameStr(parent_con->conname),
! NameStr(child_con->conname)) != 0)
! continue;
! if (!constraints_equivalent(parent_tuple, child_tuple, tuple_desc))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("child table \"%s\" has different definition for check constraint \"%s\"",
! RelationGetRelationName(child_rel),
! NameStr(parent_con->conname))));
/*
* OK, bump the child constraint's inheritance count. (If we fail
--- 8361,8407 ----
Form_pg_constraint child_con = (Form_pg_constraint) GETSTRUCT(child_tuple);
HeapTuple child_copy;
! switch(child_con->contype)
! {
! case CONSTRAINT_CHECK:
! {
! if (strcmp(NameStr(parent_con->conname),
! NameStr(child_con->conname)) != 0)
! continue;
!
! if (!constraints_equivalent(parent_tuple, child_tuple, tuple_desc))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("child table \"%s\" has different definition for check constraint \"%s\"",
! RelationGetRelationName(child_rel),
! NameStr(parent_con->conname))));
! break;
! }
! case CONSTRAINT_NOTNULL:
! {
! Datum arrayp;
! Datum *vals;
! int nelems;
! bool isnull;
! if (child_attnums == NIL)
! continue;
! arrayp = SysCacheGetAttr(CONSTROID, child_tuple,
! Anum_pg_constraint_conkey, &isnull);
! /* should not happen */
! Assert(!isnull);
! deconstruct_array(DatumGetArrayTypeP(arrayp), INT2OID, 2, true,
! 's', &vals, NULL, &nelems);
! Assert(nelems > 0);
!
! if (!list_member_int(child_attnums, DatumGetInt16(vals[0])))
! continue;
! break;
! }
! default:
! continue;
! }
/*
* OK, bump the child constraint's inheritance count. (If we fail
*************** MergeConstraintsIntoExisting(Relation ch
*** 7810,7817 ****
* surprise. But at least we'll never surprise by dropping columns someone
* isn't expecting to be dropped which would actually mean data loss.
*
! * coninhcount and conislocal for inherited constraints are adjusted in
! * exactly the same way.
*/
static void
ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode)
--- 8444,8451 ----
* surprise. But at least we'll never surprise by dropping columns someone
* isn't expecting to be dropped which would actually mean data loss.
*
! * coninhcount and conislocal for inherited CHECK and NOT NULL constraints
! * are adjusted in exactly the same way.
*/
static void
ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode)
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7824,7830 ****
attributeTuple,
constraintTuple,
depTuple;
! List *connames;
bool found = false;
/*
--- 8458,8464 ----
attributeTuple,
constraintTuple,
depTuple;
! List *constraints;
bool found = false;
/*
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7874,7879 ****
--- 8508,8515 ----
RelationGetRelationName(parent_rel),
RelationGetRelationName(rel))));
+ constraints = NIL;
+
/*
* Search through child columns looking for ones matching parent rel
*/
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7887,7892 ****
--- 8523,8529 ----
while (HeapTupleIsValid(attributeTuple = systable_getnext(scan)))
{
Form_pg_attribute att = (Form_pg_attribute) GETSTRUCT(attributeTuple);
+ HeapTuple parentattr_tuple;
/* Ignore if dropped or not inherited */
if (att->attisdropped)
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7894,7922 ****
if (att->attinhcount <= 0)
continue;
! if (SearchSysCacheExistsAttName(RelationGetRelid(parent_rel),
! NameStr(att->attname)))
{
/* Decrement inhcount and possibly set islocal to true */
HeapTuple copyTuple = heap_copytuple(attributeTuple);
Form_pg_attribute copy_att = (Form_pg_attribute) GETSTRUCT(copyTuple);
!
copy_att->attinhcount--;
if (copy_att->attinhcount == 0)
copy_att->attislocal = true;
simple_heap_update(catalogRelation, ©Tuple->t_self, copyTuple);
CatalogUpdateIndexes(catalogRelation, copyTuple);
heap_freetuple(copyTuple);
}
}
systable_endscan(scan);
heap_close(catalogRelation, RowExclusiveLock);
/*
! * Likewise, find inherited check constraints and disinherit them. To do
! * this, we first need a list of the names of the parent's check
! * constraints. (We cheat a bit by only checking for name matches,
* assuming that the expressions will match.)
*/
catalogRelation = heap_open(ConstraintRelationId, RowExclusiveLock);
--- 8531,8580 ----
if (att->attinhcount <= 0)
continue;
! parentattr_tuple = SearchSysCacheCopyAttName(RelationGetRelid(parent_rel),
! NameStr(att->attname));
!
! if (HeapTupleIsValid(parentattr_tuple))
{
/* Decrement inhcount and possibly set islocal to true */
HeapTuple copyTuple = heap_copytuple(attributeTuple);
Form_pg_attribute copy_att = (Form_pg_attribute) GETSTRUCT(copyTuple);
! Form_pg_attribute parent_att = (Form_pg_attribute) GETSTRUCT(parentattr_tuple);
!
copy_att->attinhcount--;
if (copy_att->attinhcount == 0)
copy_att->attislocal = true;
+
+ /*
+ * If attnotnull is set, record the inherited
+ * not null constraint. We save its attnum to deinherit
+ * its representation in pg_constraint below, but only when
+ * its ancestor has a NOT NULL constraint, too.
+ */
+ if (copy_att->attnotnull
+ && parent_att->attnotnull)
+ {
+ DeinheritConstraintInfo *inhInfo
+ = (DeinheritConstraintInfo *)palloc(sizeof(DeinheritConstraintInfo));
+ inhInfo->contype = CONSTRAINT_NOTNULL;
+ inhInfo->conname = NULL;
+ inhInfo->attnum = copy_att->attnum;
+ constraints = lappend(constraints, inhInfo);
+ }
simple_heap_update(catalogRelation, ©Tuple->t_self, copyTuple);
CatalogUpdateIndexes(catalogRelation, copyTuple);
heap_freetuple(copyTuple);
+ heap_freetuple(parentattr_tuple);
}
}
systable_endscan(scan);
heap_close(catalogRelation, RowExclusiveLock);
/*
! * Likewise, find inherited check and NOT NULL constraints and disinherit
! * them. To do this, we first need a list of the names of the parent's check
! * and NOT NULL constraints. (We cheat a bit by only checking for name matches,
* assuming that the expressions will match.)
*/
catalogRelation = heap_open(ConstraintRelationId, RowExclusiveLock);
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7927,7940 ****
scan = systable_beginscan(catalogRelation, ConstraintRelidIndexId,
true, SnapshotNow, 1, key);
- connames = NIL;
-
while (HeapTupleIsValid(constraintTuple = systable_getnext(scan)))
{
Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(constraintTuple);
if (con->contype == CONSTRAINT_CHECK)
! connames = lappend(connames, pstrdup(NameStr(con->conname)));
}
systable_endscan(scan);
--- 8585,8603 ----
scan = systable_beginscan(catalogRelation, ConstraintRelidIndexId,
true, SnapshotNow, 1, key);
while (HeapTupleIsValid(constraintTuple = systable_getnext(scan)))
{
Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(constraintTuple);
if (con->contype == CONSTRAINT_CHECK)
! {
! DeinheritConstraintInfo *inhInfo
! = (DeinheritConstraintInfo *)palloc(sizeof(DeinheritConstraintInfo));
! inhInfo->contype = CONSTRAINT_CHECK;
! inhInfo->conname = pstrdup(NameStr(con->conname));
! inhInfo->attnum = 0;
! constraints = lappend(constraints, inhInfo);
! }
}
systable_endscan(scan);
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7952,7969 ****
Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(constraintTuple);
bool match;
ListCell *lc;
! if (con->contype != CONSTRAINT_CHECK)
continue;
match = false;
! foreach(lc, connames)
{
! if (strcmp(NameStr(con->conname), (char *) lfirst(lc)) == 0)
{
match = true;
break;
}
}
if (match)
--- 8615,8663 ----
Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(constraintTuple);
bool match;
ListCell *lc;
+ Datum arrayp;
+ Datum *keyvals;
+ int nelems;
+ bool isnull;
! if (con->contype != CONSTRAINT_CHECK
! && con->contype != CONSTRAINT_NOTNULL)
continue;
+ /*
+ * We scan through all constraints of the current child relation,
+ * checking for CONSTRAINT_CHECK and CONSTRAINT_NOTNULL occurrences.
+ *
+ * CHECK constraints are assumed to be named equally to in the
+ * relation's ancestor, while NOT NULL constraints always are attached
+ * to a specific column. Check for attnum and adjust the inheritance
+ * information accordingly.
+ */
+
match = false;
! foreach(lc, constraints)
{
! DeinheritConstraintInfo *inhInfo = (DeinheritConstraintInfo *) lfirst(lc);
!
! if ((inhInfo->contype == CONSTRAINT_CHECK)
! && (strcmp(NameStr(con->conname), inhInfo->conname) == 0))
{
match = true;
break;
}
+ else if (inhInfo->contype == CONSTRAINT_NOTNULL)
+ {
+ arrayp = SysCacheGetAttr(CONSTROID, constraintTuple,
+ Anum_pg_constraint_conkey, &isnull);
+
+ deconstruct_array(DatumGetArrayTypeP(arrayp), INT2OID, 2, true,
+ 's', &keyvals, NULL, &nelems);
+ if (keyvals[0] == inhInfo->attnum)
+ {
+ match = true;
+ break;
+ }
+ }
}
if (match)
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 37ca331..b4d8d3d 100644
*** a/src/backend/parser/parse_utilcmd.c
--- b/src/backend/parser/parse_utilcmd.c
*************** transformColumnDefinition(ParseState *ps
*** 466,471 ****
--- 466,472 ----
parser_errposition(pstate,
constraint->location)));
column->is_not_null = TRUE;
+ cxt->ckconstraints = lappend(cxt->ckconstraints, constraint);
saw_nullable = true;
break;
diff --git a/src/backend/port/pg_latch.c b/src/backend/port/pg_latch.c
index ...002f2f4 .
*** a/src/backend/port/pg_latch.c
--- b/src/backend/port/pg_latch.c
***************
*** 0 ****
--- 1 ----
+ ../../../src/backend/port/unix_latch.c
\ No newline at end of file
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 578b9ce..22498d2 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
*************** pg_get_constraintdef_worker(Oid constrai
*** 1071,1083 ****
if (fullCommand && OidIsValid(conForm->conrelid))
{
! appendStringInfo(&buf, "ALTER TABLE ONLY %s ADD CONSTRAINT %s ",
! generate_relation_name(conForm->conrelid, NIL),
! quote_identifier(NameStr(conForm->conname)));
}
switch (conForm->contype)
{
case CONSTRAINT_FOREIGN:
{
Datum val;
--- 1071,1112 ----
if (fullCommand && OidIsValid(conForm->conrelid))
{
! /* XXX: TODO, not sure how to handle this right now? */
! if (conForm->contype == CONSTRAINT_NOTNULL)
! {
! appendStringInfo(&buf, "ALTER TABLE ONLY %s ALTER COLUMN ",
! generate_relation_name(conForm->conrelid, NIL));
! }
! else
! {
! appendStringInfo(&buf, "ALTER TABLE ONLY %s ADD CONSTRAINT %s ",
! generate_relation_name(conForm->conrelid, NIL),
! quote_identifier(NameStr(conForm->conname)));
! }
}
switch (conForm->contype)
{
+ case CONSTRAINT_NOTNULL:
+ {
+ Datum val;
+ bool isnull;
+
+ /* XXX: TODO, not sure how to handle this right now? */
+
+ /* Fetch referenced column OID */
+ val = SysCacheGetAttr(CONSTROID, tup,
+ Anum_pg_constraint_conkey, &isnull);
+
+ /* Should not happen */
+ if (isnull)
+ elog(ERROR, "null conkey for constraint %u",
+ constraintId);
+
+ decompile_column_index_array(val, conForm->conrelid, &buf);
+ appendStringInfo(&buf, " SET NOT NULL");
+ break;
+ }
case CONSTRAINT_FOREIGN:
{
Datum val;
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index 7795bda..4f55a57 100644
*** a/src/include/catalog/heap.h
--- b/src/include/catalog/heap.h
*************** extern List *AddRelationNewConstraints(R
*** 91,97 ****
bool is_local);
extern void StoreAttrDefault(Relation rel, AttrNumber attnum, Node *expr);
!
extern Node *cookDefault(ParseState *pstate,
Node *raw_default,
Oid atttypid,
--- 91,97 ----
bool is_local);
extern void StoreAttrDefault(Relation rel, AttrNumber attnum, Node *expr);
! extern void StoreColumnNotNullConstraint(Relation rel, CookedConstraint *cooked);
extern Node *cookDefault(ParseState *pstate,
Node *raw_default,
Oid atttypid,
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index 49e7c31..a0abc65 100644
*** a/src/include/catalog/pg_constraint.h
--- b/src/include/catalog/pg_constraint.h
*************** typedef FormData_pg_constraint *Form_pg_
*** 175,180 ****
--- 175,181 ----
/* Valid values for contype */
#define CONSTRAINT_CHECK 'c'
+ #define CONSTRAINT_NOTNULL 'n'
#define CONSTRAINT_FOREIGN 'f'
#define CONSTRAINT_PRIMARY 'p'
#define CONSTRAINT_UNIQUE 'u'
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index b2f0fef..ab2d3ed 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
*************** typedef enum AlterTableType
*** 1117,1123 ****
--- 1117,1125 ----
AT_AddColumnToView, /* implicitly via CREATE OR REPLACE VIEW */
AT_ColumnDefault, /* alter column default */
AT_DropNotNull, /* alter column drop not null */
+ AT_DropNotNullRecurse, /* internal to commands/tablecmds.c */
AT_SetNotNull, /* alter column set not null */
+ AT_SetNotNullRecurse, /* internal to commands/tablecmds.c */
AT_SetStatistics, /* alter column set statistics */
AT_SetOptions, /* alter column set ( options ) */
AT_ResetOptions, /* alter column reset ( options ) */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index ab19a8e..f1585e1 100644
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
*************** create table atacc1 ( test int );
*** 503,509 ****
insert into atacc1 (test) values (NULL);
-- add a primary key (fails)
alter table atacc1 add constraint atacc_test1 primary key (test);
! ERROR: column "test" contains null values
insert into atacc1 (test) values (3);
drop table atacc1;
-- let's do one where the primary key constraint fails
--- 503,509 ----
insert into atacc1 (test) values (NULL);
-- add a primary key (fails)
alter table atacc1 add constraint atacc_test1 primary key (test);
! ERROR: column "test" of relation "atacc1" contains null values
insert into atacc1 (test) values (3);
drop table atacc1;
-- let's do one where the primary key constraint fails
*************** insert into atacc1 (test) values (0);
*** 520,526 ****
-- add a primary key column without a default (fails).
alter table atacc1 add column test2 int primary key;
NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index "atacc1_pkey" for table "atacc1"
! 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;
NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index "atacc1_pkey" for table "atacc1"
--- 520,526 ----
-- add a primary key column without a default (fails).
alter table atacc1 add column test2 int primary key;
NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index "atacc1_pkey" for table "atacc1"
! ERROR: column "test2" of relation "atacc1" contains null values
-- now add a primary key column with a default (succeeds).
alter table atacc1 add column test2 int default 0 primary key;
NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index "atacc1_pkey" for table "atacc1"
*************** alter table atacc1 drop constraint "atac
*** 583,589 ****
alter table atacc1 alter column test drop not null;
insert into atacc1 values (null);
alter table atacc1 alter test set not null;
! ERROR: column "test" contains null values
delete from atacc1;
alter table atacc1 alter test set not null;
-- try altering a non-existent column, should fail
--- 583,589 ----
alter table atacc1 alter column test drop not null;
insert into atacc1 values (null);
alter table atacc1 alter test set not null;
! ERROR: column "test" of relation "atacc1" contains null values
delete from atacc1;
alter table atacc1 alter test set not null;
-- try altering a non-existent column, should fail
*************** alter table atacc1 alter bar drop not nu
*** 593,601 ****
ERROR: column "bar" of relation "atacc1" does not exist
-- try altering the oid column, should fail
alter table atacc1 alter oid set not null;
! ERROR: cannot alter system column "oid"
alter table atacc1 alter oid drop not null;
! ERROR: cannot alter system column "oid"
-- try creating a view and altering that, should fail
create view myview as select * from atacc1;
alter table myview alter column test drop not null;
--- 593,601 ----
ERROR: column "bar" of relation "atacc1" does not exist
-- try altering the oid column, should fail
alter table atacc1 alter oid set not null;
! ERROR: cannot alter system column "oid" of relation "atacc1"
alter table atacc1 alter oid drop not null;
! ERROR: cannot alter system column "oid" of relation "atacc1"
-- try creating a view and altering that, should fail
create view myview as select * from atacc1;
alter table myview alter column test drop not null;
*************** alter table parent alter a drop not null
*** 616,628 ****
insert into parent values (NULL);
insert into child (a, b) values (NULL, 'foo');
alter table only parent alter a set not null;
! ERROR: column "a" contains null values
alter table child alter a set not null;
! ERROR: column "a" contains null values
delete from parent;
alter table only parent alter a set not null;
insert into parent values (NULL);
- ERROR: null value in column "a" violates not-null constraint
alter table child alter a set not null;
insert into child (a, b) values (NULL, 'foo');
ERROR: null value in column "a" violates not-null constraint
--- 616,628 ----
insert into parent values (NULL);
insert into child (a, b) values (NULL, 'foo');
alter table only parent alter a set not null;
! ERROR: NOT NULL constraint must be added to child tables too
alter table child alter a set not null;
! ERROR: column "a" of relation "child" contains null values
delete from parent;
alter table only parent alter a set not null;
+ ERROR: NOT NULL constraint must be added to child tables too
insert into parent values (NULL);
alter table child alter a set not null;
insert into child (a, b) values (NULL, 'foo');
ERROR: null value in column "a" violates not-null constraint
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index 96bd816..6d0e3e1 100644
*** a/src/test/regress/expected/cluster.out
--- b/src/test/regress/expected/cluster.out
*************** ERROR: insert or update on table "clstr
*** 251,261 ****
DETAIL: Key (b)=(1111) is not present in table "clstr_tst_s".
SELECT conname FROM pg_constraint WHERE conrelid = 'clstr_tst'::regclass
ORDER BY 1;
! conname
! ----------------
clstr_tst_con
clstr_tst_pkey
! (2 rows)
SELECT relname, relkind,
EXISTS(SELECT 1 FROM pg_class WHERE oid = c.reltoastrelid) AS hastoast
--- 251,262 ----
DETAIL: Key (b)=(1111) is not present in table "clstr_tst_s".
SELECT conname FROM pg_constraint WHERE conrelid = 'clstr_tst'::regclass
ORDER BY 1;
! conname
! ----------------------
! clstr_tst_a_not_null
clstr_tst_con
clstr_tst_pkey
! (3 rows)
SELECT relname, relkind,
EXISTS(SELECT 1 FROM pg_class WHERE oid = c.reltoastrelid) AS hastoast
On Tue, Oct 5, 2010 at 4:57 PM, Bernd Helmle <mailings@oopsware.de> wrote:
--On 30. September 2010 10:12:48 +0200 Bernd Helmle <mailings@oopsware.de>
wrote:Yeah, there where some changes in the meantime to the master which
generate some merge failures...will provide a new version along with
other fixes soonHere's a new patch that addresses all DDL commands around NOT NULL
constraints and maintain and follow inheritance information correctly now
(but it lags documentation updates). I hope i haven't introduced nasty bugs
and broke something badly, some deeper testing is welcome.
This appears to be waiting on further review from Andrew Geery.
Andrew, will you be posting a new review soon?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
I'll post it sometime tomorrow...
Thanks
Andrew
Show quoted text
On Wed, Oct 13, 2010 at 9:25 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Oct 5, 2010 at 4:57 PM, Bernd Helmle <mailings@oopsware.de> wrote:
--On 30. September 2010 10:12:48 +0200 Bernd Helmle <mailings@oopsware.de>
wrote:Yeah, there where some changes in the meantime to the master which
generate some merge failures...will provide a new version along with
other fixes soonHere's a new patch that addresses all DDL commands around NOT NULL
constraints and maintain and follow inheritance information correctly now
(but it lags documentation updates). I hope i haven't introduced nasty bugs
and broke something badly, some deeper testing is welcome.This appears to be waiting on further review from Andrew Geery.
Andrew, will you be posting a new review soon?--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Below is my review of the latest iteration of the "Extend NOT NULL
Representation to pg_constraint" patch found here:
http://archives.postgresql.org/message-id/E57A252DFD60C1FCA91731BD@amenophis
Thanks
Andrew
------------------------------------------------------------------------------------------------
Basic questions
============
Is the patch in context diff format? Yes
Does it apply cleanly to the current git master? Yes
patching file src/backend/catalog/heap.c
patching file src/backend/commands/tablecmds.c
patching file src/backend/parser/parse_utilcmd.c
patching file src/backend/port/pg_latch.c
patching file src/backend/utils/adt/ruleutils.c
Hunk #1 succeeded at 1076 (offset 5 lines).
patching file src/include/catalog/heap.h
patching file src/include/catalog/pg_constraint.h
patching file src/include/nodes/parsenodes.h
patching file src/test/regress/expected/alter_table.out
patching file src/test/regress/expected/cluster.out
However, one of the modified files in the patch is
/src/backend/port/pg_latch.c. There are no functional changes in this
file, but it does add a line to the top of the file that breaks the
build:
diff --git a/src/backend/port/pg_latch.c b/src/backend/port/pg_latch.c
index ...002f2f4 .
*** a/src/backend/port/pg_latch.c
--- b/src/backend/port/pg_latch.c
***************
*** 0 ****
--- 1 ----
+ ../../../src/backend/port/unix_latch.c
\ No newline at end of fileRemoving the line allows the build to complete successfully.
Overview
======
The impetus for this patch is to prevent a child table from dropping
an inherited not null constraint. Not only does dropping an inherited
not null constraint violate the spirit of table inheritance, but it
also breaks pg_dump (the dropped constraint on the child table is not
in the dump, so any null values in the child data will be disallowed).
To fix this problem, the patch adds a new constraint type for not null
constraints in the pg_constraint catalog, while continuing to maintain
the attnotnull info in pg_attribute.
Problem
======
In 9.0 and before, not null constraints are tracked in the
pg_attribute.attnotnull. The problem is that there is nothing in this
catalog that indicates whether the not null constraint is inherited.
However, the pg_constraint catalog does have columns for tracking
whether a constraint is local to the relation or inherited (conislocal
and coninhcount), so it makes sense to add a new constraint type
(contype=’n’) for not null constraints which enables the db to
disallow dropping inherited not null constraints. Not null
constraints are given the name (conname)
<table_name>_<column_name>_not_null. (Note that this also opens up
the possibility (if, for example, the alter table syntax was changed)
for giving the not null constraint an arbitrary name.)
Here’s a simple example of the problem:
create table foo_parent ( id int not null );
create table foo_child () inherits ( foo_parent );
alter table foo_child alter column id drop not null;
insert into foo_child values ( null );
In 9.0 and before, the db cannot detect that the “alter table ...
alter column ... drop not null” should not be allowed because there is
no information in the pg_attribute catalog to specify that the
relation is inherited.
In this example, with the patch, the pg_constraint catalog has two
additional rows, foo_parent_id_not_null (conislocal=t, coninhcount=0)
and foo_child_id_not_null (conislocal=f, coninhcount=1) and the db can
now detect that the “alter table ... alter column ... drop not null”
statement should be disallowed because the not null constraint on
foo_child is inherited. The db reports the following error for this
statement:
cannot drop inherited NOT NULL constraint "foo_child_id_not_null",
relation "foo_child"
[perhaps to make this more consistent with the error message produced
when trying to drop, for example, an inherited check constraint,
change the comma to the word “of”]
Basic tests
========
I performed the following basic SQL tests with the patch:
* create table with a column with a not null constraint -- check that
the not null constraint is recorded in the pg_constraint table
* create table with no not null column constraint; alter table to add
it -- check that the column not null constraint is recorded in the
pg_constraint table
* create parent with a not null column constraint; create child table
that inherits from the parent -- check that both have a not null
column constraint in pg_constraint and that the child’s not null
constraint inherits from the parent’s
* create parent table with no not null column constraint; create child
table that inherits from the parent; alter the parent table to add a
not null column constraint -- check that both the parent and the child
have a not null column constraint in pg_constraint
* create parent table with no not null column constraint; create child
table that inherits from the parent; alter the child table to add a
not null column constraint -- check that there is only a not null
column constraint for the child table in pg_constraint
* create parent table with a not null column constraint; create a
child table with a no not null column constraint that does not inherit
from the parent; alter the child table to inherit from the parent --
verify that the db disallows this
* create parent table with a not null column constraint; create a
child table with a matching not null constraint that does not inherit
from the parent; alter the child table to inherit from the parent --
verify that this is allowed and that the child’s
pg_constraint.conihcount = 1
* create parent with a not null column constraint; create child table
that inherits from the parent; drop the not null constraint from the
child table -- verify that the db does not allow this
* create parent with a not null column constraint; create child that
inherits from the parent; alter the child table to not inherit from
the parent -- verify that the child’s pg_constraint values are
conislocal=t and coninhcount=0
* drop the not null constraints in the scenarios above and verify that
the corresponding rows are removed from pg_constraint
Error messages changed
===================
The following error messages have been changed/added:
Was: column %s contains null values
Patched: column %s of relation %s contains null values
Was: cannot alter system column %s
Patched: cannot alter system column %s of relation %s
New error message:
NOT NULL constraint must be added to child tables too
Code
====
I didn’t have much time to look at the code. The only thing I’ll
mention is that there are a couple of XXX TODO items that should be
cleared up.
Documentation
===========
Since this patch actually makes inheritance behave in a more expected
way, nothing needs to be changed in the inheritance documentation.
However, at the very least, the documentation dealing with the
pg_catalog [http://www.postgresql.org/docs/9.0/interactive/catalog-pg-constraint.html]
needs to be updated to deal with the new constraint type.
Tests
====
I did a sanity make clean && make && make check before applying the
patch and all the tests passed. After applying the patch and doing
make clean && make && make check, I got a number of failures of the
form “FAILED (test process exited with exit code 2)”. The exact
number of failures varies by run, so I’m wondering if I didn’t do
something wrong...
The first failure I get is in the inherit tests (tail of
/src/test/regress/results/inherit.out):
alter table a alter column aa type integer using bit_length(aa);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
Connection to server was lost
This test consistently breaks in this location and breaks for both
make check and make installcheck.
However, when I just pipe the /src/test/regress/sql/inherit.sql file
through psql, the connection does not close unexpectedly because the
error, “function bit_length(integer) does not exist”, is given for the
statement in question. I’m not sure why there is a discrepancy here
or why the test passes before the patch but not after the patch...
Discussion
========
Below are excerpts from the lists about the problem and patch.
* The problem, with example, was very succinctly described in this
message by Bernd Helmle:
http://archives.postgresql.org/pgsql-hackers/2009-11/msg00146.php
* The underlying problem was described by Tom Lane here:
[http://archives.postgresql.org/pgsql-hackers/2009-11/msg00149.php]:
“The ALTER should be rejected, but it is not, because we don't have
enough infrastructure to notice that the constraint is inherited and
logically can't be dropped. I think the consensus was that the way to
fix this (along with some other problems) is to start representing NOT
NULL constraints in pg_constraint, turning attnotnull into just a bit
of denormalization for performance.”
* The basic patch proposal is described here by Bernd Helme:
http://archives.postgresql.org/pgsql-hackers/2009-11/msg00536.php
My first idea is to just introduce a special contype in pg_constraint
representing a NOT NULL constraint on a column, which holds all
required information to do the mentioned maintenance stuff on them and
to keep most of the current infrastructure. Utility commands need to
track all changes in pg_constraint and keep pg_attribute.attnotnull up
to date.
* Follow-up discussion here from Tom Lane, agreeing that a special
constraint is better than handling not null as a generic check:
http://archives.postgresql.org/pgsql-hackers/2009-11/msg00556.php
Testing attnotnull is significantly cheaper than enforcing a generic
constraint expression, and NOT NULL is a sufficiently common case to
be worth worrying about optimizing it. Furthermore, removing
attnotnull would break an unknown but probably not-negligible amount
of client-side code that cares whether columns are known not null (I
think both JDBC and ODBC track that).
* Detailed description of the patch from Bernd Helme:
http://archives.postgresql.org/message-id/57F9D81FFD86336C6F92B2CD@amenophis
The patch creates a new CONSTRAINT_NOTNULL contype and assigns all
required information for the NOT NULL constraint to it. Currently the
constraint records the attribute number it belongs to and manages the
inheritance properties. Passes regression tests with some adjustments
to pg_constraint output.
The patch as it stands employs a dedicated code path for
ATExecDropNotNull(), thus duplicates the behavior of
ATExecDropConstraint(). I'm not really satisfied with this, but i did
it this way to prevent some heavy conditional rearrangement
inATExecDropConstraint(). Maybe its worth to move the code to adjust
constraint inheritance properties into a separate function.
Show quoted text
On Wed, Oct 13, 2010 at 9:41 PM, Andrew Geery <andrew.geery@gmail.com> wrote:
I'll post it sometime tomorrow...
Thanks
AndrewOn Wed, Oct 13, 2010 at 9:25 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Oct 5, 2010 at 4:57 PM, Bernd Helmle <mailings@oopsware.de> wrote:
--On 30. September 2010 10:12:48 +0200 Bernd Helmle <mailings@oopsware.de>
wrote:Yeah, there where some changes in the meantime to the master which
generate some merge failures...will provide a new version along with
other fixes soonHere's a new patch that addresses all DDL commands around NOT NULL
constraints and maintain and follow inheritance information correctly now
(but it lags documentation updates). I hope i haven't introduced nasty bugs
and broke something badly, some deeper testing is welcome.This appears to be waiting on further review from Andrew Geery.
Andrew, will you be posting a new review soon?--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Oct 14, 2010 at 10:02 AM, Andrew Geery <andrew.geery@gmail.com> wrote:
I didn’t have much time to look at the code. The only thing I’ll
mention is that there are a couple of XXX TODO items that should be
cleared up.
[...]
Since this patch actually makes inheritance behave in a more expected
way, nothing needs to be changed in the inheritance documentation.
However, at the very least, the documentation dealing with the
pg_catalog [http://www.postgresql.org/docs/9.0/interactive/catalog-pg-constraint.html]
needs to be updated to deal with the new constraint type.
Thanks for catching these problems.
I did a sanity make clean && make && make check before applying the
patch and all the tests passed. After applying the patch and doing
make clean && make && make check, I got a number of failures of the
form “FAILED (test process exited with exit code 2)”. The exact
number of failures varies by run, so I’m wondering if I didn’t do
something wrong...
That indicates that PostgreSQL is crashing. So I think this patch is
definitely not ready for prime time yet, and needs some debugging. In
view of the fact that we are out of time for this CommitFest, I'm
going to mark this Returned with Feedback in the CommitFest
application. Hopefully it will be resubmitted for the next CommitFest
after further refinement, because I think this is a good and useful
improvement.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--On 14. Oktober 2010 11:42:27 -0400 Robert Haas <robertmhaas@gmail.com>
wrote:
I did a sanity make clean && make && make check before applying the
patch and all the tests passed. After applying the patch and doing
make clean && make && make check, I got a number of failures of the
form “FAILED (test process exited with exit code 2)”. The exact
number of failures varies by run, so I’m wondering if I didn’t do
something wrong...That indicates that PostgreSQL is crashing. So I think this patch is
definitely not ready for prime time yet, and needs some debugging. In
view of the fact that we are out of time for this CommitFest, I'm
going to mark this Returned with Feedback in the CommitFest
application. Hopefully it will be resubmitted for the next CommitFest
after further refinement, because I think this is a good and useful
improvement.
Yeah, its crashing but it doesn't do it here on my MacBook.... (passing the
regression test is one of my top prio when submitting a patch). Must be
some broken pointer or similar oversight which is triggered on Andrew's
box. Andrew, which OS and architecture have you tested on?
--
Thanks
Bernd
On 14 October 2010 16:42, Robert Haas <robertmhaas@gmail.com> wrote:
In view of the fact that we are out of time for this CommitFest, ...
When is the official end of this commitfest?
I remember talk at the start, that the end would be postponed (by a
week?) due to time spent on the git migration. Is that still the case?
Regards,
Dean
--On 14. Oktober 2010 10:02:12 -0400 Andrew Geery <andrew.geery@gmail.com>
wrote:
The first failure I get is in the inherit tests (tail of
/src/test/regress/results/inherit.out):alter table a alter column aa type integer using bit_length(aa);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
Connection to server was lostThis test consistently breaks in this location and breaks for both
make check and make installcheck.
Okay, can reproduce it on a Linux box here, will be back with a fixed
version.
--
Thanks
Bernd
On 14 October 2010 17:40, Bernd Helmle <mailings@oopsware.de> wrote:
--On 14. Oktober 2010 11:42:27 -0400 Robert Haas <robertmhaas@gmail.com>
wrote:I did a sanity make clean && make && make check before applying the
patch and all the tests passed. After applying the patch and doing
make clean && make && make check, I got a number of failures of the
form “FAILED (test process exited with exit code 2)”. The exact
number of failures varies by run, so I’m wondering if I didn’t do
something wrong...That indicates that PostgreSQL is crashing. So I think this patch is
definitely not ready for prime time yet, and needs some debugging. In
view of the fact that we are out of time for this CommitFest, I'm
going to mark this Returned with Feedback in the CommitFest
application. Hopefully it will be resubmitted for the next CommitFest
after further refinement, because I think this is a good and useful
improvement.Yeah, its crashing but it doesn't do it here on my MacBook.... (passing the
regression test is one of my top prio when submitting a patch). Must be some
broken pointer or similar oversight which is triggered on Andrew's box.
Andrew, which OS and architecture have you tested on?
Yeah, it crashes for me too (opensuse 11.2 64-bit) but only in an
optimised build.
There are a couple of compiler warnings:
tablecmds.c: In function 'ATExecSetNotNull':
tablecmds.c:4747: warning: unused variable 'is_new_constraint'
tablecmds.c: In function 'ATExecDropNotNull':
tablecmds.c:4332: warning: 'found' may be used uninitialized in this function
tablecmds.c:4246: warning: 'children' may be used uninitialized in this function
The last 2 look serious enough to cause problems, but fixing them
didn't cure the crash.
Digging deeper, I get a segfault running src/test/regress/sql/alter_table.sql:
Program received signal SIGSEGV, Segmentation fault.
ATExecSetNotNullInternal (is_local=1 '\001',
is_new_constraint=<value optimized out>, atttup=<value optimized out>,
attr_rel=<value optimized out>, rel=<value optimized out>)
at tablecmds.c:4847
4847 Form_pg_constraint constr =
(Form_pg_constraint) GETSTRUCT(copy_tuple);
Looking in that function, there is a similar "found" variable that
isn't being initialised (which my compiler didn't warn about).
Initialising that to false, sems to fix the problem and all the
regression tests then pass.
Regards,
Dean
Looking in that function, there is a similar "found" variable that
isn't being initialised (which my compiler didn't warn about).
Initialising that to false, sems to fix the problem and all the
regression tests then pass.
Excellent. Please send an updated patch.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
--On 14. Oktober 2010 19:16:56 +0100 Dean Rasheed
<dean.a.rasheed@gmail.com> wrote:
Program received signal SIGSEGV, Segmentation fault.
ATExecSetNotNullInternal (is_local=1 '\001',
is_new_constraint=<value optimized out>, atttup=<value optimized out>,
attr_rel=<value optimized out>, rel=<value optimized out>)
at tablecmds.c:4847
4847 Form_pg_constraint constr =
(Form_pg_constraint) GETSTRUCT(copy_tuple);Looking in that function, there is a similar "found" variable that
isn't being initialised (which my compiler didn't warn about).
Initialising that to false, sems to fix the problem and all the
regression tests then pass.
Yepp, that was it. I had a CFLAGS='-O0' in my dev build from a former
debugging cycle and forgot about it (which reminds me to do a
maintainer-clean more often between coding). This is also the reason i
haven't seen the compiler warnings and the crash in the regression tests.
Shame on me, but i think i have learned the lesson ;)
--
Thanks
Bernd
On 14 October 2010 20:28, Alvaro Herrera <alvherre@commandprompt.com> wrote:
Looking in that function, there is a similar "found" variable that
isn't being initialised (which my compiler didn't warn about).
Initialising that to false, sems to fix the problem and all the
regression tests then pass.Excellent. Please send an updated patch.
OK, here it is.
I've not cured this compiler warning (in fact I'm not sure what it's
complaining about, because the variable *is* used):
tablecmds.c: In function 'ATExecSetNotNull':
tablecmds.c:4747: warning: unused variable 'is_new_constraint'
Regards,
Dean
Attachments:
notnull_constraint2.patchapplication/octet-stream; name=notnull_constraint2.patchDownload
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index dcc53e1..c976e46 100644
*** a/src/backend/catalog/heap.c
--- b/src/backend/catalog/heap.c
*************** StoreAttrDefault(Relation rel, AttrNumbe
*** 1714,1719 ****
--- 1714,1759 ----
}
/*
+ * Stores a NOT NULL constraint of a column into pg_constraint.
+ */
+ void
+ StoreColumnNotNullConstraint(Relation rel, CookedConstraint *cooked)
+ {
+
+ /*
+ * Store the constraint. Reflect conislocal and coninhcount to
+ * match the same values as the attached column.
+ */
+ CreateConstraintEntry(cooked->name,
+ RelationGetNamespace(rel),
+ CONSTRAINT_NOTNULL,
+ false,
+ false,
+ RelationGetRelid(rel),
+ &(cooked->attnum),
+ 1,
+ InvalidOid,
+ InvalidOid,
+ InvalidOid,
+ NULL,
+ InvalidOid,
+ InvalidOid,
+ InvalidOid,
+ 0,
+ ' ',
+ ' ',
+ ' ',
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ cooked->is_local,
+ cooked->inhcount
+ );
+
+ }
+
+ /*
* Store a check-constraint expression for the given relation.
*
* Caller is responsible for updating the count of constraints
*************** StoreConstraints(Relation rel, List *coo
*** 1837,1842 ****
--- 1877,1885 ----
switch (con->contype)
{
+ case CONSTR_NOTNULL:
+ StoreColumnNotNullConstraint(rel, con);
+ break;
case CONSTR_DEFAULT:
StoreAttrDefault(rel, con->attnum, con->expr);
break;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c009711..88d574b 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*************** typedef struct NewColumnValue
*** 175,180 ****
--- 175,192 ----
} NewColumnValue;
/*
+ * Struct describing the constraint information
+ * to de-inherit. Currently CHECK and NOT NULL constraints
+ * are carried by ATExecDropInherit() within these struct.
+ */
+ typedef struct DeinheritConstraintInfo
+ {
+ char contype; /* constraint type */
+ AttrNumber attnum; /* if NOT NULL constraint, the attnum */
+ char *conname; /* if CHECK constraint, the constraint name */
+ } DeinheritConstraintInfo;
+
+ /*
* Error-reporting support for RemoveRelations
*/
struct dropmsgstrings
*************** static List *MergeAttributes(List *schem
*** 227,234 ****
List **supOids, List **supconstr, int *supOidCount);
static bool MergeCheckConstraint(List *constraints, char *name, Node *expr);
static bool change_varattnos_walker(Node *node, const AttrNumber *newattno);
! static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel);
! static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel);
static void StoreCatalogInheritance(Oid relationId, List *supers);
static void StoreCatalogInheritance1(Oid relationId, Oid parentOid,
int16 seqNumber, Relation inhRelation);
--- 239,248 ----
List **supOids, List **supconstr, int *supOidCount);
static bool MergeCheckConstraint(List *constraints, char *name, Node *expr);
static bool change_varattnos_walker(Node *node, const AttrNumber *newattno);
! static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel,
! List **child_attnums);
! static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel,
! List *child_attnums);
static void StoreCatalogInheritance(Oid relationId, List *supers);
static void StoreCatalogInheritance1(Oid relationId, Oid parentOid,
int16 seqNumber, Relation inhRelation);
*************** static void ATExecAddColumn(AlteredTable
*** 276,285 ****
ColumnDef *colDef, bool isOid, LOCKMODE lockmode);
static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid);
static void ATPrepAddOids(List **wqueue, Relation rel, bool recurse,
! AlterTableCmd *cmd, LOCKMODE lockmode);
! static void ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode);
! static void ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
! const char *colName, LOCKMODE lockmode);
static void ATExecColumnDefault(Relation rel, const char *colName,
Node *newDefault, LOCKMODE lockmode);
static void ATPrepSetStatistics(Relation rel, const char *colName,
--- 290,304 ----
ColumnDef *colDef, bool isOid, LOCKMODE lockmode);
static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid);
static void ATPrepAddOids(List **wqueue, Relation rel, bool recurse,
! AlterTableCmd *cmd, LOCKMODE lockmode);
! static void ATExecDropNotNull(Relation rel, const char *colName, DropBehavior behavior,
! bool recurse, bool recursing, LOCKMODE lockmode);
! static void ATExecSetNotNull(List **wqueue, AlteredTableInfo *tab, Relation rel,
! const char *colName, bool recurse, bool recursing,
! LOCKMODE lockmode);
! static void ATExecSetNotNullInternal(Relation rel, Relation attr_rel,
! HeapTuple atttup, bool *is_new_constraint,
! bool is_local);
static void ATExecColumnDefault(Relation rel, const char *colName,
Node *newDefault, LOCKMODE lockmode);
static void ATPrepSetStatistics(Relation rel, const char *colName,
*************** static void ATExecDropInherit(Relation r
*** 336,342 ****
static void copy_relation_data(SMgrRelation rel, SMgrRelation dst,
ForkNumber forkNum, bool istemp);
static const char *storage_name(char c);
!
/* ----------------------------------------------------------------
* DefineRelation
--- 355,368 ----
static void copy_relation_data(SMgrRelation rel, SMgrRelation dst,
ForkNumber forkNum, bool istemp);
static const char *storage_name(char c);
! static bool ATExecDropNotNullInternal(Relation rel, HeapTuple constrtup,
! DropBehavior behavior,
! bool recurse, bool recursing);
! static bool
! CheckNotNullOnAttributeName(Relation rel, const char *colname,
! AttrNumber *attnum);
! static void
! DropNotNullOnAttributeNum(Relation rel, AttrNumber attnum, bool lock);
/* ----------------------------------------------------------------
* DefineRelation
*************** DefineRelation(CreateStmt *stmt, char re
*** 508,513 ****
--- 534,560 ----
attnum++;
+ if (colDef->is_not_null) {
+ /*
+ * Adjust NOT NULL constraint of this column
+ * to hold new attnum and inheritance information
+ *
+ * XXX: Currently the constraint is cheated into cookedDefaults
+ * list, maybe we need to invent a different machinerie/naming.
+ */
+ CookedConstraint *cooked;
+
+ cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint));
+ cooked->contype = CONSTR_NOTNULL;
+ cooked->name = ChooseConstraintName(relname, NameStr(descriptor->attrs[attnum - 1]->attname),
+ "not_null", namespaceId, NIL);
+ cooked->attnum = attnum;
+ cooked->expr = NULL;
+ cooked->is_local = colDef->is_local;
+ cooked->inhcount = colDef->inhcount;
+ cookedDefaults = lappend(cookedDefaults, cooked);
+ }
+
if (colDef->raw_default != NULL)
{
RawColumnDefault *rawEnt;
*************** ATPrepCmd(List **wqueue, Relation rel, A
*** 2682,2696 ****
/* No command-specific prep needed */
pass = cmd->def ? AT_PASS_ADD_CONSTR : AT_PASS_DROP;
break;
! case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
ATSimplePermissions(rel, false, false);
! 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, false, false);
! ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
/* No command-specific prep needed */
pass = AT_PASS_ADD_CONSTR;
break;
--- 2729,2748 ----
/* No command-specific prep needed */
pass = cmd->def ? AT_PASS_ADD_CONSTR : AT_PASS_DROP;
break;
! case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
ATSimplePermissions(rel, false, false);
!
! if (recurse)
! cmd->subtype = AT_DropNotNullRecurse;
/* No command-specific prep needed */
pass = AT_PASS_DROP;
break;
case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */
ATSimplePermissions(rel, false, false);
!
! if (recurse)
! cmd->subtype = AT_SetNotNullRecurse;
!
/* No command-specific prep needed */
pass = AT_PASS_ADD_CONSTR;
break;
*************** ATExecCmd(List **wqueue, AlteredTableInf
*** 2914,2923 ****
ATExecColumnDefault(rel, cmd->name, cmd->def, lockmode);
break;
case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
! ATExecDropNotNull(rel, cmd->name, lockmode);
break;
case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */
! ATExecSetNotNull(tab, rel, cmd->name, lockmode);
break;
case AT_SetStatistics: /* ALTER COLUMN SET STATISTICS */
ATExecSetStatistics(rel, cmd->name, cmd->def, lockmode);
--- 2966,2983 ----
ATExecColumnDefault(rel, cmd->name, cmd->def, lockmode);
break;
case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
! ATExecDropNotNull(rel, cmd->name, cmd->behavior,
! false, false, lockmode);
! break;
! case AT_DropNotNullRecurse: /* ALTER COLUMN DROP NOT NULL with recursion */
! ATExecDropNotNull(rel, cmd->name, cmd->behavior,
! true, false, lockmode);
break;
case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */
! ATExecSetNotNull(wqueue, tab, rel, cmd->name, false, false, lockmode);
! break;
! case AT_SetNotNullRecurse: /* ALTER COLUMN SET NOT NULL recursing */
! ATExecSetNotNull(wqueue, tab, rel, cmd->name, true, false, lockmode);
break;
case AT_SetStatistics: /* ALTER COLUMN SET STATISTICS */
ATExecSetStatistics(rel, cmd->name, cmd->def, lockmode);
*************** ATRewriteTable(AlteredTableInfo *tab, Oi
*** 3441,3448 ****
if (heap_attisnull(tuple, attn + 1))
ereport(ERROR,
(errcode(ERRCODE_NOT_NULL_VIOLATION),
! errmsg("column \"%s\" contains null values",
! NameStr(newTupDesc->attrs[attn]->attname))));
}
foreach(l, tab->constraints)
--- 3501,3509 ----
if (heap_attisnull(tuple, attn + 1))
ereport(ERROR,
(errcode(ERRCODE_NOT_NULL_VIOLATION),
! errmsg("column \"%s\" of relation \"%s\" contains null values",
! NameStr(newTupDesc->attrs[attn]->attname),
! RelationGetRelationName(oldrel))));
}
foreach(l, tab->constraints)
*************** ATPrepAddOids(List **wqueue, Relation re
*** 4179,4225 ****
* ALTER TABLE ALTER COLUMN DROP NOT NULL
*/
static void
! ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
{
! HeapTuple tuple;
AttrNumber attnum;
! Relation attr_rel;
! List *indexoidlist;
! ListCell *indexoidscan;
/*
! * lookup the attribute
*/
! attr_rel = heap_open(AttributeRelationId, RowExclusiveLock);
! tuple = SearchSysCacheCopyAttName(RelationGetRelid(rel), colName);
! if (!HeapTupleIsValid(tuple))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" of relation \"%s\" does not exist",
! colName, RelationGetRelationName(rel))));
! attnum = ((Form_pg_attribute) GETSTRUCT(tuple))->attnum;
/* Prevent them from altering a system attribute */
! if (attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg("cannot alter system column \"%s\"",
! colName)));
/*
! * Check that the attribute is not in a primary key
*/
/* Loop over all indexes on the relation */
indexoidlist = RelationGetIndexList(rel);
!
foreach(indexoidscan, indexoidlist)
{
! Oid indexoid = lfirst_oid(indexoidscan);
! HeapTuple indexTuple;
Form_pg_index indexStruct;
int i;
--- 4240,4585 ----
* ALTER TABLE ALTER COLUMN DROP NOT NULL
*/
static void
! ATExecDropNotNull(Relation rel, const char *colName, DropBehavior behavior,
! bool recurse, bool recursing, LOCKMODE lockmode)
{
! List *children = NULL;
! ListCell *child;
! HeapTuple constr_tuple;
AttrNumber attnum;
! Relation constr_rel;
! SysScanDesc scan;
! ScanKeyData key;
! bool found = false;
!
! if (recursing)
! ATSimplePermissions(rel, false, false);
/*
! * Lookup the attribute. This also checks for a dropped column
! * or any attempts to alter a system column. Returns false in case
! * no further work needs to be done (e.g. no NOT NULL present).
*/
! if (!CheckNotNullOnAttributeName(rel, colName, &attnum))
! return;
! /*
! * Scan through our constraint records, looking for the matching
! * NOT NULL constraint.
! */
! constr_rel = heap_open(ConstraintRelationId, RowExclusiveLock);
! ScanKeyInit(&key,
! Anum_pg_constraint_conrelid,
! BTEqualStrategyNumber, F_OIDEQ,
! ObjectIdGetDatum(RelationGetRelid(rel)));
! scan = systable_beginscan(constr_rel, ConstraintRelidIndexId,
! true, SnapshotNow, 1, &key);
!
! while(HeapTupleIsValid(constr_tuple = systable_getnext(scan)))
! {
! Form_pg_constraint constrStruct;
! Datum arrayp;
! Datum *keyvals;
! int nelems;
! bool isnull;
! constrStruct = (Form_pg_constraint) GETSTRUCT(constr_tuple);
!
! if (constrStruct->contype != CONSTRAINT_NOTNULL)
! continue;
!
! /*
! * Check wether this constraint is attached to the given column.
! * ATExecSetNotNull() ensures that only one NOT NULL constraint tuple
! * lives per attribute.
! */
! arrayp = SysCacheGetAttr(CONSTROID, constr_tuple, Anum_pg_constraint_conkey, &isnull);
!
! /* should not happen */
! Assert(!isnull);
!
! deconstruct_array(DatumGetArrayTypeP(arrayp), INT2OID, 2, true,
! 's', &keyvals, NULL, &nelems);
!
! /* Is this NOT NULL constraint attached to the current tuple? */
! if (DatumGetInt16(keyvals[0]) != attnum)
! continue;
!
! /*
! * Check if the attribute is inherited from another relation or
! * part of a primary key.
! * If true, error out, otherwise perform the deletion of the constraint.
! *
! * Note: ATExecDropNotNullInternal() performs a CCI, if required.
! */
! if ((found = ATExecDropNotNullInternal(rel,
! constr_tuple,
! behavior,
! recurse, recursing)))
! {
! /*
! * Okay, actually remove the NOT NULL from pg_attribute.
! * CheckNotNullOnAttributeName() already holds a lock on pg_attribute, so
! * override an additional lock here.
! */
! DropNotNullOnAttributeNum(rel, attnum, false);
! break;
! }
! }
!
! systable_endscan(scan);
!
! if (found)
! {
! /*
! * Propagate to children as appropriate.
! */
! children = find_inheritance_children(RelationGetRelid(rel),
! AccessExclusiveLock);
! }
! else
! {
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_OBJECT),
! errmsg("column \"%s\" of relation \"%s\" has no NOT NULL constraint",
! colName,
! RelationGetRelationName(rel))));
! }
!
! foreach(child, children)
! {
! Oid childrelid = lfirst_oid(child);
! Relation childrel;
!
! childrel = heap_open(childrelid, NoLock);
! CheckTableNotInUse(childrel, "ALTER TABLE");
!
! ScanKeyInit(&key,
! Anum_pg_constraint_conrelid,
! BTEqualStrategyNumber, F_OIDEQ,
! ObjectIdGetDatum(childrelid));
! scan = systable_beginscan(constr_rel, ConstraintRelidIndexId,
! true, SnapshotNow, 1, &key);
!
! while (HeapTupleIsValid(constr_tuple = systable_getnext(scan)))
! {
! HeapTuple copy_tuple;
! Form_pg_constraint constrStruct;
! Datum arrayp;
! Datum *keyvals;
! int nelems;
! bool isnull;
!
! constrStruct = (Form_pg_constraint) GETSTRUCT(constr_tuple);
!
! if (constrStruct->contype != CONSTRAINT_NOTNULL)
! continue;
!
! /*
! * Check wether this constraint is attached to the given column.
! * ATExecSetNotNull() ensures that only one NOT NULL constraint tuple
! * lives per attribute.
! */
! arrayp = SysCacheGetAttr(CONSTROID, constr_tuple, Anum_pg_constraint_conkey, &isnull);
!
! /* should not happen */
! Assert(!isnull);
!
! deconstruct_array(DatumGetArrayTypeP(arrayp), INT2OID, 2, true,
! 's', &keyvals, NULL, &nelems);
!
! /* Is this NOT NULL constraint attached to the current tuple? */
! if (DatumGetInt16(keyvals[0]) != attnum)
! continue;
!
! found = true;
!
! if (constrStruct->coninhcount <= 0) /* shouldn't happen */
! elog(ERROR, "relation %u has non-inherited NOT NULL constraint \"%s\"",
! childrelid, NameStr(constrStruct->conname));
!
! copy_tuple = heap_copytuple(constr_tuple);
! constrStruct = (Form_pg_constraint) GETSTRUCT(copy_tuple);
!
! if (recurse)
! {
! /*
! * If the child constraint has other definition sources, just
! * decrement its inheritance count; if not, recurse to delete
! * it.
! */
! if (constrStruct->coninhcount == 1 && !constrStruct->conislocal)
! {
! /* Time to delete this child constraint, too */
! ATExecDropNotNull(childrel, colName, behavior,
! true, true, lockmode);
! }
! else
! {
! /* Child constraint must survive my deletion */
! constrStruct->coninhcount--;
! simple_heap_update(constr_rel, ©_tuple->t_self, copy_tuple);
! CatalogUpdateIndexes(constr_rel, copy_tuple);
!
! /* Make update visible */
! CommandCounterIncrement();
! }
! }
! else
! {
! /*
! * If we were told to drop ONLY in this table (no recursion),
! * we need to mark the inheritors' constraints as locally
! * defined rather than inherited.
! */
! constrStruct->coninhcount--;
! constrStruct->conislocal = true;
!
! simple_heap_update(constr_rel, ©_tuple->t_self, copy_tuple);
! CatalogUpdateIndexes(constr_rel, copy_tuple);
!
! /* Make update visible */
! CommandCounterIncrement();
! }
!
! heap_freetuple(copy_tuple);
!
! }
!
! systable_endscan(scan);
! heap_close(childrel, NoLock);
!
! }
!
! heap_close(constr_rel, NoLock);
!
! }
!
! /*
! * Does the leg work on dropping a NOT NULL constraint
! * identified by the given constrtup tuple.
! *
! * Note: The caller is responsible to pass a valid
! * CONSTRAINT_NOTNULL tuple.
! */
! static bool
! ATExecDropNotNullInternal(Relation rel, HeapTuple constrtup,
! DropBehavior behavior,
! bool recurse, bool recursing)
! {
! bool found;
! Form_pg_constraint constrStruct;
! ObjectAddress conobj;
!
! found = false;
!
! if (HeapTupleIsValid(constrtup))
! {
!
! constrStruct = (Form_pg_constraint) GETSTRUCT(constrtup);
!
! /*
! * Be paranoid, caller is responsible to pass a valid HeapTuple.
! */
! Assert(constrStruct->contype == CONSTRAINT_NOTNULL);
!
! /* It is okay to drop this constraint when recursing */
! if ((constrStruct->coninhcount > 0) && (!recursing))
! {
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
! errmsg("cannot drop inherited NOT NULL constraint \"%s\", relation \"%s\"",
! NameStr(constrStruct->conname), RelationGetRelationName(rel))));
! }
!
! /*
! * Time to drop the constraint now.
! */
! conobj.classId = ConstraintRelationId;
! conobj.objectId = HeapTupleGetOid(constrtup);
! conobj.objectSubId = 0;
!
! performDeletion(&conobj, behavior);
!
! /*
! * Make sure changes are visible
! */
! CommandCounterIncrement();
! found = true;
!
! }
!
! return found;
! }
!
! /*
! * Checks wether the given attribute name has a NOT NULL constraint attached
! * which can be dropped.
! *
! * Any attempts to drop a NOT NULL constraint on a system
! * column or non-existing column will throw an error. Returns true
! * in case the caller is allowed to proceed, false if no NOT NULL flag
! * was set on the attribute.
! */
! static bool
! CheckNotNullOnAttributeName(Relation rel, const char *colname, AttrNumber *attnum)
! {
! Relation attr_rel;
! HeapTuple attr_tuple;
! Form_pg_attribute attr_struct;
! bool has_not_null;
! List *indexoidlist;
! ListCell *indexoidscan;
!
! has_not_null = false;
!
! attr_rel = heap_open(AttributeRelationId, RowExclusiveLock);
! attr_tuple = SearchSysCacheCopyAttName(RelationGetRelid(rel), colname);
!
! if (!HeapTupleIsValid(attr_tuple))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" of relation \"%s\" does not exist",
! colname,
! RelationGetRelationName(rel))));
! /* attribute exists */
! attr_struct = (Form_pg_attribute) GETSTRUCT(attr_tuple);
/* Prevent them from altering a system attribute */
! if (attr_struct->attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg("cannot alter system column \"%s\" of relation \"%s\"",
! colname,
! RelationGetRelationName(rel))));
/*
! * Check for dropped attributes
*/
+ if (attr_struct->attisdropped)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("attempt to drop NOT NULL constraint on dropped column \"%s\" of relation \"%s\"",
+ colname,
+ RelationGetRelationName(rel))));
+ }
+
+ if (attnum)
+ *attnum = attr_struct->attnum;
+ /*
+ * Check that the attribute is not in a primary key
+ */
+
/* Loop over all indexes on the relation */
indexoidlist = RelationGetIndexList(rel);
!
foreach(indexoidscan, indexoidlist)
{
! Oid indexoid = lfirst_oid(indexoidscan);
! HeapTuple indexTuple;
Form_pg_index indexStruct;
int i;
*************** ATExecDropNotNull(Relation rel, const ch
*** 4227,4233 ****
if (!HeapTupleIsValid(indexTuple))
elog(ERROR, "cache lookup failed for index %u", indexoid);
indexStruct = (Form_pg_index) GETSTRUCT(indexTuple);
!
/* If the index is not a primary key, skip the check */
if (indexStruct->indisprimary)
{
--- 4587,4593 ----
if (!HeapTupleIsValid(indexTuple))
elog(ERROR, "cache lookup failed for index %u", indexoid);
indexStruct = (Form_pg_index) GETSTRUCT(indexTuple);
!
/* If the index is not a primary key, skip the check */
if (indexStruct->indisprimary)
{
*************** ATExecDropNotNull(Relation rel, const ch
*** 4237,4281 ****
*/
for (i = 0; i < indexStruct->indnatts; i++)
{
! if (indexStruct->indkey.values[i] == attnum)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("column \"%s\" is in a primary key",
! colName)));
}
}
!
ReleaseSysCache(indexTuple);
}
! list_free(indexoidlist);
!
! /*
! * Okay, actually perform the catalog change ... if needed
*/
! if (((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull)
! {
! ((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull = FALSE;
! simple_heap_update(attr_rel, &tuple->t_self, tuple);
! /* keep the system catalog indexes current */
! CatalogUpdateIndexes(attr_rel, tuple);
! }
- heap_close(attr_rel, RowExclusiveLock);
}
/*
* ALTER TABLE ALTER COLUMN SET NOT NULL
*/
static void
! ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
! const char *colName, LOCKMODE lockmode)
{
HeapTuple tuple;
AttrNumber attnum;
Relation attr_rel;
/*
* lookup the attribute
--- 4597,4681 ----
*/
for (i = 0; i < indexStruct->indnatts; i++)
{
! if (indexStruct->indkey.values[i] == attr_struct->attnum)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("column \"%s\" is in a primary key",
! colname)));
}
}
!
ReleaseSysCache(indexTuple);
}
! /*
! * In case no NOT NULL constraint was set on the
! * column, give up and tell the caller we haven't done
! * anything.
*/
! has_not_null = ((!attr_struct->attnotnull) ? false : true);
! heap_freetuple(attr_tuple);
! heap_close(attr_rel, NoLock);
! return has_not_null;
! }
! /*
! * Drops a NOT NULL constraint from the given attribute number of the
! * specified relation.
! */
! static void
! DropNotNullOnAttributeNum(Relation rel, AttrNumber attnum, bool lock)
! {
! Relation attr_rel;
! HeapTuple attr_tuple;
! Form_pg_attribute attr_struct;
!
! if (lock)
! attr_rel = heap_open(AttributeRelationId, RowExclusiveLock);
! else
! attr_rel = heap_open(AttributeRelationId, NoLock);
!
! attr_tuple = SearchSysCacheCopy(ATTNUM,
! ObjectIdGetDatum(RelationGetRelid(rel)),
! Int16GetDatum(attnum),
! 0, 0);
!
! attr_struct = (Form_pg_attribute) GETSTRUCT(attr_tuple);
!
! /* not expected, so be paranoid */
! Assert(attr_struct->attnotnull);
!
! ((Form_pg_attribute) GETSTRUCT(attr_tuple))->attnotnull = FALSE;
! simple_heap_update(attr_rel, &attr_tuple->t_self, attr_tuple);
!
! /* keep the system catalog indexes current */
! CatalogUpdateIndexes(attr_rel, attr_tuple);
!
! heap_close(attr_rel, NoLock);
}
/*
* ALTER TABLE ALTER COLUMN SET NOT NULL
*/
static void
! ATExecSetNotNull(List **wqueue, AlteredTableInfo *tab, Relation rel,
! const char *colName, bool recurse, bool recursing,
! LOCKMODE lockmode)
{
HeapTuple tuple;
AttrNumber attnum;
Relation attr_rel;
+ List *children;
+ ListCell *child;
+ bool is_new_constraint;
+
+ if (recursing)
+ ATSimplePermissions(rel, false, false);
+
+ children = find_inheritance_children(RelationGetRelid(rel),
+ AccessExclusiveLock);
/*
* lookup the attribute
*************** ATExecSetNotNull(AlteredTableInfo *tab,
*** 4296,4321 ****
if (attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg("cannot alter system column \"%s\"",
! colName)));
/*
! * Okay, actually perform the catalog change ... if needed
*/
! if (!((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull)
{
! ((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull = TRUE;
! simple_heap_update(attr_rel, &tuple->t_self, tuple);
! /* keep the system catalog indexes current */
! CatalogUpdateIndexes(attr_rel, tuple);
! /* Tell Phase 3 it needs to test the constraint */
! tab->new_notnull = true;
}
! heap_close(attr_rel, RowExclusiveLock);
}
/*
--- 4696,4907 ----
if (attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg("cannot alter system column \"%s\" of relation \"%s\"",
! colName, RelationGetRelationName(rel))));
/*
! * Treat it an error if there's an attempt to add
! * a NOT NULL constraint to a table but not to
! * existing child tables (thus, ALTER TABLE ONLY
! * was specified to a table inherited from others).
! * Maybe we should treat this as a no op, but it seems
! * better to inform the user that he's doing something
! * wrong.
*/
! if (!recurse)
! {
! if (children)
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
! errmsg("NOT NULL constraint must be added to child tables too")));
! }
!
! /*
! * Okay, actually perform the catalog changes ...
! *
! * Setting attnotnull to TRUE requires to create a
! * constraint tuple in pg_constraint as well. We do this
! * to record wether this constraint was added locally or
! * inherited by some other attribute. We also record the
! * attribute number to determine which column this
! * constraint belongs to. We need to take care wether
! * the constraint was added locally to a child table or
! * we're currently already recursing through an inheritance
! * tree.
! */
! ATExecSetNotNullInternal(rel, attr_rel, tuple, &is_new_constraint, !recursing);
!
! /* Tell Phase 3 it needs to test the constraint */
! tab->new_notnull = true;
!
! heap_freetuple(tuple);
! heap_close(attr_rel, RowExclusiveLock);
!
! /*
! * Loop through all child relations
! */
! foreach(child, children)
{
! Oid childrelid = lfirst_oid(child);
! Relation childrel;
! bool is_new_constraint;
! AlteredTableInfo *childtab;
! attr_rel = heap_open(AttributeRelationId, RowExclusiveLock);
! childrel = heap_open(childrelid, NoLock);
! CheckTableNotInUse(childrel, "ALTER TABLE");
! /* get work queue entry for child relation */
! childtab = ATGetQueueEntry(wqueue, childrel);
! /*
! * Look for inherited column
! */
! tuple = SearchSysCacheCopyAttName(childrelid, colName);
!
! /* shouldn't happen */
! Assert(HeapTupleIsValid(tuple));
!
! /* perform the actual work */
! ATExecSetNotNull(wqueue, childtab, childrel,
! colName, recurse, true, lockmode);
!
! heap_freetuple(tuple);
! heap_close(attr_rel, NoLock);
! heap_close(childrel, NoLock);
}
+ }
! /*
! * Internal function to ATExecSetNotNull()
! *
! * Takes a relation and the pg_attribute relation and tuple of the
! * target column to create a NOT NULL constraint.
! * The caller is responsible to pass a valid HeapTuple.
! */
! static void
! ATExecSetNotNullInternal(Relation rel, Relation attr_rel,
! HeapTuple atttup, bool *is_new_constraint, bool is_local)
! {
! Form_pg_attribute attr;
!
! attr = (Form_pg_attribute) GETSTRUCT(atttup);
! *is_new_constraint = FALSE;
!
! if (attr->attnotnull)
! {
! SysScanDesc scan;
! ScanKeyData key;
! HeapTuple constr_tup;
! Relation constr_rel;
! bool found = false;
!
! /*
! * Column has already a NOT NULL constraint attached,
! * fetch its pg_constraint tuple and adjust the inheritance
! * information according to its pg_attribute tuple.
! */
! constr_rel = heap_open(ConstraintRelationId, RowExclusiveLock);
! ScanKeyInit(&key,
! Anum_pg_constraint_conrelid,
! BTEqualStrategyNumber, F_OIDEQ,
! ObjectIdGetDatum(RelationGetRelid(rel)));
! scan = systable_beginscan(constr_rel, ConstraintRelidIndexId,
! true, SnapshotNow, 1, &key);
!
! while (HeapTupleIsValid(constr_tup = systable_getnext(scan)))
! {
! Form_pg_constraint constr_struct;
! Datum arrayp;
! Datum *keyvals;
! int nelems;
! bool isnull;
!
! constr_struct = (Form_pg_constraint) GETSTRUCT(constr_tup);
!
! /* We need to look for NOT NULL constraint only */
! if (constr_struct->contype != CONSTRAINT_NOTNULL)
! continue;
!
! /* constraint tuple attached to our column? */
! arrayp = SysCacheGetAttr(CONSTROID, constr_tup,
! Anum_pg_constraint_conkey,
! &isnull);
! /* not expected here */
! Assert(!isnull);
!
! deconstruct_array(DatumGetArrayTypeP(arrayp), INT2OID, 2, true,
! 's', &keyvals, NULL, &nelems);
! if (DatumGetInt16(keyvals[0]) != attr->attnum)
! continue;
!
! /* only reached in case we have found the required constraint tuple */
! found = TRUE;
! break;
! }
!
! /* there should be a NOT NULL constraint tuple */
! if (found)
! {
! HeapTuple copy_tuple = heap_copytuple(constr_tup);
! Form_pg_constraint constr = (Form_pg_constraint) GETSTRUCT(copy_tuple);
!
! /*
! * We don't need to update the constraint tuple when
! * the inheritance counter is already up to date
! */
! if (constr->coninhcount != attr->attinhcount)
! {
! constr->coninhcount = is_local ? 0 : attr->attinhcount;
! simple_heap_update(constr_rel, ©_tuple->t_self, copy_tuple);
!
! /*
! * Keep system catalog indexes current and make
! * changes visible
! */
! CatalogUpdateIndexes(constr_rel, copy_tuple);
! CommandCounterIncrement();
! *is_new_constraint = TRUE;
! }
!
! heap_freetuple(copy_tuple);
! }
!
! systable_endscan(scan);
! heap_close(constr_rel, NoLock);
!
! }
! else
! {
! CookedConstraint *cooked;
!
! /*
! * New NOT NULL constraint
! */
! cooked = palloc(sizeof(CookedConstraint));
! cooked->contype = CONSTR_NOTNULL;
! cooked->expr = NULL;
! cooked->name = ChooseConstraintName(RelationGetRelationName(rel),
! NameStr(attr->attname),
! "not_null", RelationGetNamespace(rel),
! NIL);
! cooked->attnum = attr->attnum;
! cooked->is_local = is_local;
! cooked->inhcount = is_local ? 0 : 1;
!
! StoreColumnNotNullConstraint(rel, cooked);
!
! /* Make changes visible */
! CommandCounterIncrement();
!
! attr->attnotnull = TRUE;
! simple_heap_update(attr_rel, &atttup->t_self, atttup);
!
! /* keep the system catalog indexes current */
! CatalogUpdateIndexes(attr_rel, atttup);
!
! *is_new_constraint = TRUE;
! }
}
/*
*************** ATExecDropConstraint(Relation rel, const
*** 5893,5898 ****
--- 6479,6488 ----
con = (Form_pg_constraint) GETSTRUCT(tuple);
+ /* NOT NULL is handled by ALTER TABLE ... DROP/SET NOT NULL */
+ if (con->contype == CONSTRAINT_NOTNULL)
+ continue;
+
if (strcmp(NameStr(con->conname), constrName) != 0)
continue;
*************** ATExecDropConstraint(Relation rel, const
*** 5903,5909 ****
errmsg("cannot drop inherited constraint \"%s\" of relation \"%s\"",
constrName, RelationGetRelationName(rel))));
! /* Right now only CHECK constraints can be inherited */
if (con->contype == CONSTRAINT_CHECK)
is_check_constraint = true;
--- 6493,6501 ----
errmsg("cannot drop inherited constraint \"%s\" of relation \"%s\"",
constrName, RelationGetRelationName(rel))));
! /* This is a CHECK constraint, remember that we found one
! * and proceed to drop it
! */
if (con->contype == CONSTRAINT_CHECK)
is_check_constraint = true;
*************** ATExecDropConstraint(Relation rel, const
*** 5974,5980 ****
con = (Form_pg_constraint) GETSTRUCT(tuple);
! /* Right now only CHECK constraints can be inherited */
if (con->contype != CONSTRAINT_CHECK)
continue;
--- 6566,6575 ----
con = (Form_pg_constraint) GETSTRUCT(tuple);
! /* Besides CHECK constraint we support inheritance of
! * NOT NULL constraints, too. Ignore them here, since they
! * are handled by ALTER TABLE...DROP/SET NOT NULL
! */
if (con->contype != CONSTRAINT_CHECK)
continue;
*************** ATPostAlterTypeParse(char *cmd, List **w
*** 6701,6706 ****
--- 7296,7302 ----
tab->subcmds[AT_PASS_OLD_INDEX] =
lappend(tab->subcmds[AT_PASS_OLD_INDEX], cmd);
break;
+ case AT_SetNotNull:
case AT_AddConstraint:
tab->subcmds[AT_PASS_OLD_CONSTR] =
lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
*************** ATExecAddInherit(Relation child_rel, Ran
*** 7462,7467 ****
--- 8058,8064 ----
HeapTuple inheritsTuple;
int32 inhseqno;
List *children;
+ List *child_attnums;
/*
* AccessShareLock on the parent is what's obtained during normal CREATE
*************** ATExecAddInherit(Relation child_rel, Ran
*** 7548,7557 ****
RelationGetRelationName(parent_rel))));
/* Match up the columns and bump attinhcount as needed */
! MergeAttributesIntoExisting(child_rel, parent_rel);
/* Match up the constraints and bump coninhcount as needed */
! MergeConstraintsIntoExisting(child_rel, parent_rel);
/*
* OK, it looks valid. Make the catalog entries that show inheritance.
--- 8145,8155 ----
RelationGetRelationName(parent_rel))));
/* Match up the columns and bump attinhcount as needed */
! child_attnums = NIL;
! MergeAttributesIntoExisting(child_rel, parent_rel, &child_attnums);
/* Match up the constraints and bump coninhcount as needed */
! MergeConstraintsIntoExisting(child_rel, parent_rel, child_attnums);
/*
* OK, it looks valid. Make the catalog entries that show inheritance.
*************** constraints_equivalent(HeapTuple a, Heap
*** 7627,7633 ****
* the child must be as well. Defaults are not compared, however.
*/
static void
! MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel)
{
Relation attrrel;
AttrNumber parent_attno;
--- 8225,8232 ----
* the child must be as well. Defaults are not compared, however.
*/
static void
! MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel,
! List **attnums_list)
{
Relation attrrel;
AttrNumber parent_attno;
*************** MergeAttributesIntoExisting(Relation chi
*** 7674,7679 ****
--- 8273,8283 ----
attributeName)));
/*
+ * Record the child's inherited attnum in attnums_list.
+ */
+ *attnums_list = lappend_int(*attnums_list, childatt->attnum);
+
+ /*
* OK, bump the child column's inheritance count. (If we fail
* later on, this change will just roll back.)
*/
*************** MergeAttributesIntoExisting(Relation chi
*** 7712,7718 ****
* a problem though. Even 100 constraints ought not be the end of the world.
*/
static void
! MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel)
{
Relation catalog_relation;
TupleDesc tuple_desc;
--- 8316,8323 ----
* a problem though. Even 100 constraints ought not be the end of the world.
*/
static void
! MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel,
! List *child_attnums)
{
Relation catalog_relation;
TupleDesc tuple_desc;
*************** MergeConstraintsIntoExisting(Relation ch
*** 7739,7745 ****
HeapTuple child_tuple;
bool found = false;
! if (parent_con->contype != CONSTRAINT_CHECK)
continue;
/* Search for a child constraint matching this one */
--- 8344,8351 ----
HeapTuple child_tuple;
bool found = false;
! if (parent_con->contype != CONSTRAINT_CHECK
! && parent_con->contype != CONSTRAINT_NOTNULL)
continue;
/* Search for a child constraint matching this one */
*************** MergeConstraintsIntoExisting(Relation ch
*** 7755,7773 ****
Form_pg_constraint child_con = (Form_pg_constraint) GETSTRUCT(child_tuple);
HeapTuple child_copy;
! if (child_con->contype != CONSTRAINT_CHECK)
! continue;
! if (strcmp(NameStr(parent_con->conname),
! NameStr(child_con->conname)) != 0)
! continue;
! if (!constraints_equivalent(parent_tuple, child_tuple, tuple_desc))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("child table \"%s\" has different definition for check constraint \"%s\"",
! RelationGetRelationName(child_rel),
! NameStr(parent_con->conname))));
/*
* OK, bump the child constraint's inheritance count. (If we fail
--- 8361,8407 ----
Form_pg_constraint child_con = (Form_pg_constraint) GETSTRUCT(child_tuple);
HeapTuple child_copy;
! switch(child_con->contype)
! {
! case CONSTRAINT_CHECK:
! {
! if (strcmp(NameStr(parent_con->conname),
! NameStr(child_con->conname)) != 0)
! continue;
!
! if (!constraints_equivalent(parent_tuple, child_tuple, tuple_desc))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("child table \"%s\" has different definition for check constraint \"%s\"",
! RelationGetRelationName(child_rel),
! NameStr(parent_con->conname))));
! break;
! }
! case CONSTRAINT_NOTNULL:
! {
! Datum arrayp;
! Datum *vals;
! int nelems;
! bool isnull;
! if (child_attnums == NIL)
! continue;
! arrayp = SysCacheGetAttr(CONSTROID, child_tuple,
! Anum_pg_constraint_conkey, &isnull);
! /* should not happen */
! Assert(!isnull);
! deconstruct_array(DatumGetArrayTypeP(arrayp), INT2OID, 2, true,
! 's', &vals, NULL, &nelems);
! Assert(nelems > 0);
!
! if (!list_member_int(child_attnums, DatumGetInt16(vals[0])))
! continue;
! break;
! }
! default:
! continue;
! }
/*
* OK, bump the child constraint's inheritance count. (If we fail
*************** MergeConstraintsIntoExisting(Relation ch
*** 7810,7817 ****
* surprise. But at least we'll never surprise by dropping columns someone
* isn't expecting to be dropped which would actually mean data loss.
*
! * coninhcount and conislocal for inherited constraints are adjusted in
! * exactly the same way.
*/
static void
ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode)
--- 8444,8451 ----
* surprise. But at least we'll never surprise by dropping columns someone
* isn't expecting to be dropped which would actually mean data loss.
*
! * coninhcount and conislocal for inherited CHECK and NOT NULL constraints
! * are adjusted in exactly the same way.
*/
static void
ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode)
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7824,7830 ****
attributeTuple,
constraintTuple,
depTuple;
! List *connames;
bool found = false;
/*
--- 8458,8464 ----
attributeTuple,
constraintTuple,
depTuple;
! List *constraints;
bool found = false;
/*
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7874,7879 ****
--- 8508,8515 ----
RelationGetRelationName(parent_rel),
RelationGetRelationName(rel))));
+ constraints = NIL;
+
/*
* Search through child columns looking for ones matching parent rel
*/
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7887,7892 ****
--- 8523,8529 ----
while (HeapTupleIsValid(attributeTuple = systable_getnext(scan)))
{
Form_pg_attribute att = (Form_pg_attribute) GETSTRUCT(attributeTuple);
+ HeapTuple parentattr_tuple;
/* Ignore if dropped or not inherited */
if (att->attisdropped)
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7894,7922 ****
if (att->attinhcount <= 0)
continue;
! if (SearchSysCacheExistsAttName(RelationGetRelid(parent_rel),
! NameStr(att->attname)))
{
/* Decrement inhcount and possibly set islocal to true */
HeapTuple copyTuple = heap_copytuple(attributeTuple);
Form_pg_attribute copy_att = (Form_pg_attribute) GETSTRUCT(copyTuple);
!
copy_att->attinhcount--;
if (copy_att->attinhcount == 0)
copy_att->attislocal = true;
simple_heap_update(catalogRelation, ©Tuple->t_self, copyTuple);
CatalogUpdateIndexes(catalogRelation, copyTuple);
heap_freetuple(copyTuple);
}
}
systable_endscan(scan);
heap_close(catalogRelation, RowExclusiveLock);
/*
! * Likewise, find inherited check constraints and disinherit them. To do
! * this, we first need a list of the names of the parent's check
! * constraints. (We cheat a bit by only checking for name matches,
* assuming that the expressions will match.)
*/
catalogRelation = heap_open(ConstraintRelationId, RowExclusiveLock);
--- 8531,8580 ----
if (att->attinhcount <= 0)
continue;
! parentattr_tuple = SearchSysCacheCopyAttName(RelationGetRelid(parent_rel),
! NameStr(att->attname));
!
! if (HeapTupleIsValid(parentattr_tuple))
{
/* Decrement inhcount and possibly set islocal to true */
HeapTuple copyTuple = heap_copytuple(attributeTuple);
Form_pg_attribute copy_att = (Form_pg_attribute) GETSTRUCT(copyTuple);
! Form_pg_attribute parent_att = (Form_pg_attribute) GETSTRUCT(parentattr_tuple);
!
copy_att->attinhcount--;
if (copy_att->attinhcount == 0)
copy_att->attislocal = true;
+
+ /*
+ * If attnotnull is set, record the inherited
+ * not null constraint. We save its attnum to deinherit
+ * its representation in pg_constraint below, but only when
+ * its ancestor has a NOT NULL constraint, too.
+ */
+ if (copy_att->attnotnull
+ && parent_att->attnotnull)
+ {
+ DeinheritConstraintInfo *inhInfo
+ = (DeinheritConstraintInfo *)palloc(sizeof(DeinheritConstraintInfo));
+ inhInfo->contype = CONSTRAINT_NOTNULL;
+ inhInfo->conname = NULL;
+ inhInfo->attnum = copy_att->attnum;
+ constraints = lappend(constraints, inhInfo);
+ }
simple_heap_update(catalogRelation, ©Tuple->t_self, copyTuple);
CatalogUpdateIndexes(catalogRelation, copyTuple);
heap_freetuple(copyTuple);
+ heap_freetuple(parentattr_tuple);
}
}
systable_endscan(scan);
heap_close(catalogRelation, RowExclusiveLock);
/*
! * Likewise, find inherited check and NOT NULL constraints and disinherit
! * them. To do this, we first need a list of the names of the parent's check
! * and NOT NULL constraints. (We cheat a bit by only checking for name matches,
* assuming that the expressions will match.)
*/
catalogRelation = heap_open(ConstraintRelationId, RowExclusiveLock);
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7927,7940 ****
scan = systable_beginscan(catalogRelation, ConstraintRelidIndexId,
true, SnapshotNow, 1, key);
- connames = NIL;
-
while (HeapTupleIsValid(constraintTuple = systable_getnext(scan)))
{
Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(constraintTuple);
if (con->contype == CONSTRAINT_CHECK)
! connames = lappend(connames, pstrdup(NameStr(con->conname)));
}
systable_endscan(scan);
--- 8585,8603 ----
scan = systable_beginscan(catalogRelation, ConstraintRelidIndexId,
true, SnapshotNow, 1, key);
while (HeapTupleIsValid(constraintTuple = systable_getnext(scan)))
{
Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(constraintTuple);
if (con->contype == CONSTRAINT_CHECK)
! {
! DeinheritConstraintInfo *inhInfo
! = (DeinheritConstraintInfo *)palloc(sizeof(DeinheritConstraintInfo));
! inhInfo->contype = CONSTRAINT_CHECK;
! inhInfo->conname = pstrdup(NameStr(con->conname));
! inhInfo->attnum = 0;
! constraints = lappend(constraints, inhInfo);
! }
}
systable_endscan(scan);
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7952,7969 ****
Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(constraintTuple);
bool match;
ListCell *lc;
! if (con->contype != CONSTRAINT_CHECK)
continue;
match = false;
! foreach(lc, connames)
{
! if (strcmp(NameStr(con->conname), (char *) lfirst(lc)) == 0)
{
match = true;
break;
}
}
if (match)
--- 8615,8663 ----
Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(constraintTuple);
bool match;
ListCell *lc;
+ Datum arrayp;
+ Datum *keyvals;
+ int nelems;
+ bool isnull;
! if (con->contype != CONSTRAINT_CHECK
! && con->contype != CONSTRAINT_NOTNULL)
continue;
+ /*
+ * We scan through all constraints of the current child relation,
+ * checking for CONSTRAINT_CHECK and CONSTRAINT_NOTNULL occurrences.
+ *
+ * CHECK constraints are assumed to be named equally to in the
+ * relation's ancestor, while NOT NULL constraints always are attached
+ * to a specific column. Check for attnum and adjust the inheritance
+ * information accordingly.
+ */
+
match = false;
! foreach(lc, constraints)
{
! DeinheritConstraintInfo *inhInfo = (DeinheritConstraintInfo *) lfirst(lc);
!
! if ((inhInfo->contype == CONSTRAINT_CHECK)
! && (strcmp(NameStr(con->conname), inhInfo->conname) == 0))
{
match = true;
break;
}
+ else if (inhInfo->contype == CONSTRAINT_NOTNULL)
+ {
+ arrayp = SysCacheGetAttr(CONSTROID, constraintTuple,
+ Anum_pg_constraint_conkey, &isnull);
+
+ deconstruct_array(DatumGetArrayTypeP(arrayp), INT2OID, 2, true,
+ 's', &keyvals, NULL, &nelems);
+ if (keyvals[0] == inhInfo->attnum)
+ {
+ match = true;
+ break;
+ }
+ }
}
if (match)
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 37ca331..b4d8d3d 100644
*** a/src/backend/parser/parse_utilcmd.c
--- b/src/backend/parser/parse_utilcmd.c
*************** transformColumnDefinition(ParseState *ps
*** 466,471 ****
--- 466,472 ----
parser_errposition(pstate,
constraint->location)));
column->is_not_null = TRUE;
+ cxt->ckconstraints = lappend(cxt->ckconstraints, constraint);
saw_nullable = true;
break;
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index f1c1d04..7913cf5 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
*************** pg_get_constraintdef_worker(Oid constrai
*** 1076,1088 ****
if (fullCommand && OidIsValid(conForm->conrelid))
{
! appendStringInfo(&buf, "ALTER TABLE ONLY %s ADD CONSTRAINT %s ",
! generate_relation_name(conForm->conrelid, NIL),
! quote_identifier(NameStr(conForm->conname)));
}
switch (conForm->contype)
{
case CONSTRAINT_FOREIGN:
{
Datum val;
--- 1076,1117 ----
if (fullCommand && OidIsValid(conForm->conrelid))
{
! /* XXX: TODO, not sure how to handle this right now? */
! if (conForm->contype == CONSTRAINT_NOTNULL)
! {
! appendStringInfo(&buf, "ALTER TABLE ONLY %s ALTER COLUMN ",
! generate_relation_name(conForm->conrelid, NIL));
! }
! else
! {
! appendStringInfo(&buf, "ALTER TABLE ONLY %s ADD CONSTRAINT %s ",
! generate_relation_name(conForm->conrelid, NIL),
! quote_identifier(NameStr(conForm->conname)));
! }
}
switch (conForm->contype)
{
+ case CONSTRAINT_NOTNULL:
+ {
+ Datum val;
+ bool isnull;
+
+ /* XXX: TODO, not sure how to handle this right now? */
+
+ /* Fetch referenced column OID */
+ val = SysCacheGetAttr(CONSTROID, tup,
+ Anum_pg_constraint_conkey, &isnull);
+
+ /* Should not happen */
+ if (isnull)
+ elog(ERROR, "null conkey for constraint %u",
+ constraintId);
+
+ decompile_column_index_array(val, conForm->conrelid, &buf);
+ appendStringInfo(&buf, " SET NOT NULL");
+ break;
+ }
case CONSTRAINT_FOREIGN:
{
Datum val;
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index 7795bda..4f55a57 100644
*** a/src/include/catalog/heap.h
--- b/src/include/catalog/heap.h
*************** extern List *AddRelationNewConstraints(R
*** 91,97 ****
bool is_local);
extern void StoreAttrDefault(Relation rel, AttrNumber attnum, Node *expr);
!
extern Node *cookDefault(ParseState *pstate,
Node *raw_default,
Oid atttypid,
--- 91,97 ----
bool is_local);
extern void StoreAttrDefault(Relation rel, AttrNumber attnum, Node *expr);
! extern void StoreColumnNotNullConstraint(Relation rel, CookedConstraint *cooked);
extern Node *cookDefault(ParseState *pstate,
Node *raw_default,
Oid atttypid,
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index 49e7c31..a0abc65 100644
*** a/src/include/catalog/pg_constraint.h
--- b/src/include/catalog/pg_constraint.h
*************** typedef FormData_pg_constraint *Form_pg_
*** 175,180 ****
--- 175,181 ----
/* Valid values for contype */
#define CONSTRAINT_CHECK 'c'
+ #define CONSTRAINT_NOTNULL 'n'
#define CONSTRAINT_FOREIGN 'f'
#define CONSTRAINT_PRIMARY 'p'
#define CONSTRAINT_UNIQUE 'u'
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index ca225d0..7e5b81f 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
*************** typedef enum AlterTableType
*** 1117,1123 ****
--- 1117,1125 ----
AT_AddColumnToView, /* implicitly via CREATE OR REPLACE VIEW */
AT_ColumnDefault, /* alter column default */
AT_DropNotNull, /* alter column drop not null */
+ AT_DropNotNullRecurse, /* internal to commands/tablecmds.c */
AT_SetNotNull, /* alter column set not null */
+ AT_SetNotNullRecurse, /* internal to commands/tablecmds.c */
AT_SetStatistics, /* alter column set statistics */
AT_SetOptions, /* alter column set ( options ) */
AT_ResetOptions, /* alter column reset ( options ) */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index ab19a8e..f1585e1 100644
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
*************** create table atacc1 ( test int );
*** 503,509 ****
insert into atacc1 (test) values (NULL);
-- add a primary key (fails)
alter table atacc1 add constraint atacc_test1 primary key (test);
! ERROR: column "test" contains null values
insert into atacc1 (test) values (3);
drop table atacc1;
-- let's do one where the primary key constraint fails
--- 503,509 ----
insert into atacc1 (test) values (NULL);
-- add a primary key (fails)
alter table atacc1 add constraint atacc_test1 primary key (test);
! ERROR: column "test" of relation "atacc1" contains null values
insert into atacc1 (test) values (3);
drop table atacc1;
-- let's do one where the primary key constraint fails
*************** insert into atacc1 (test) values (0);
*** 520,526 ****
-- add a primary key column without a default (fails).
alter table atacc1 add column test2 int primary key;
NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index "atacc1_pkey" for table "atacc1"
! 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;
NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index "atacc1_pkey" for table "atacc1"
--- 520,526 ----
-- add a primary key column without a default (fails).
alter table atacc1 add column test2 int primary key;
NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index "atacc1_pkey" for table "atacc1"
! ERROR: column "test2" of relation "atacc1" contains null values
-- now add a primary key column with a default (succeeds).
alter table atacc1 add column test2 int default 0 primary key;
NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index "atacc1_pkey" for table "atacc1"
*************** alter table atacc1 drop constraint "atac
*** 583,589 ****
alter table atacc1 alter column test drop not null;
insert into atacc1 values (null);
alter table atacc1 alter test set not null;
! ERROR: column "test" contains null values
delete from atacc1;
alter table atacc1 alter test set not null;
-- try altering a non-existent column, should fail
--- 583,589 ----
alter table atacc1 alter column test drop not null;
insert into atacc1 values (null);
alter table atacc1 alter test set not null;
! ERROR: column "test" of relation "atacc1" contains null values
delete from atacc1;
alter table atacc1 alter test set not null;
-- try altering a non-existent column, should fail
*************** alter table atacc1 alter bar drop not nu
*** 593,601 ****
ERROR: column "bar" of relation "atacc1" does not exist
-- try altering the oid column, should fail
alter table atacc1 alter oid set not null;
! ERROR: cannot alter system column "oid"
alter table atacc1 alter oid drop not null;
! ERROR: cannot alter system column "oid"
-- try creating a view and altering that, should fail
create view myview as select * from atacc1;
alter table myview alter column test drop not null;
--- 593,601 ----
ERROR: column "bar" of relation "atacc1" does not exist
-- try altering the oid column, should fail
alter table atacc1 alter oid set not null;
! ERROR: cannot alter system column "oid" of relation "atacc1"
alter table atacc1 alter oid drop not null;
! ERROR: cannot alter system column "oid" of relation "atacc1"
-- try creating a view and altering that, should fail
create view myview as select * from atacc1;
alter table myview alter column test drop not null;
*************** alter table parent alter a drop not null
*** 616,628 ****
insert into parent values (NULL);
insert into child (a, b) values (NULL, 'foo');
alter table only parent alter a set not null;
! ERROR: column "a" contains null values
alter table child alter a set not null;
! ERROR: column "a" contains null values
delete from parent;
alter table only parent alter a set not null;
insert into parent values (NULL);
- ERROR: null value in column "a" violates not-null constraint
alter table child alter a set not null;
insert into child (a, b) values (NULL, 'foo');
ERROR: null value in column "a" violates not-null constraint
--- 616,628 ----
insert into parent values (NULL);
insert into child (a, b) values (NULL, 'foo');
alter table only parent alter a set not null;
! ERROR: NOT NULL constraint must be added to child tables too
alter table child alter a set not null;
! ERROR: column "a" of relation "child" contains null values
delete from parent;
alter table only parent alter a set not null;
+ ERROR: NOT NULL constraint must be added to child tables too
insert into parent values (NULL);
alter table child alter a set not null;
insert into child (a, b) values (NULL, 'foo');
ERROR: null value in column "a" violates not-null constraint
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index 96bd816..6d0e3e1 100644
*** a/src/test/regress/expected/cluster.out
--- b/src/test/regress/expected/cluster.out
*************** ERROR: insert or update on table "clstr
*** 251,261 ****
DETAIL: Key (b)=(1111) is not present in table "clstr_tst_s".
SELECT conname FROM pg_constraint WHERE conrelid = 'clstr_tst'::regclass
ORDER BY 1;
! conname
! ----------------
clstr_tst_con
clstr_tst_pkey
! (2 rows)
SELECT relname, relkind,
EXISTS(SELECT 1 FROM pg_class WHERE oid = c.reltoastrelid) AS hastoast
--- 251,262 ----
DETAIL: Key (b)=(1111) is not present in table "clstr_tst_s".
SELECT conname FROM pg_constraint WHERE conrelid = 'clstr_tst'::regclass
ORDER BY 1;
! conname
! ----------------------
! clstr_tst_a_not_null
clstr_tst_con
clstr_tst_pkey
! (3 rows)
SELECT relname, relkind,
EXISTS(SELECT 1 FROM pg_class WHERE oid = c.reltoastrelid) AS hastoast
--On 14. Oktober 2010 16:28:51 -0300 Alvaro Herrera
<alvherre@commandprompt.com> wrote:
Looking in that function, there is a similar "found" variable that
isn't being initialised (which my compiler didn't warn about).
Initialising that to false, sems to fix the problem and all the
regression tests then pass.Excellent. Please send an updated patch.
Here is an updated version of the patch. It fixes the following issues
Andrew discovered during his review cycle:
* Fix compiler warnings and crash due to uninitialized variables (pretty
much the fix Dean proposed)
* Remove accidentally added pg_latch.c in my own git repos.
I will do further cycles over Andrew's review report.
--
Thanks
Bernd
Attachments:
notnull_constraint.patchapplication/octet-stream; name=notnull_constraint.patchDownload
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index dcc53e1..c976e46 100644
*** a/src/backend/catalog/heap.c
--- b/src/backend/catalog/heap.c
*************** StoreAttrDefault(Relation rel, AttrNumbe
*** 1714,1719 ****
--- 1714,1759 ----
}
/*
+ * Stores a NOT NULL constraint of a column into pg_constraint.
+ */
+ void
+ StoreColumnNotNullConstraint(Relation rel, CookedConstraint *cooked)
+ {
+
+ /*
+ * Store the constraint. Reflect conislocal and coninhcount to
+ * match the same values as the attached column.
+ */
+ CreateConstraintEntry(cooked->name,
+ RelationGetNamespace(rel),
+ CONSTRAINT_NOTNULL,
+ false,
+ false,
+ RelationGetRelid(rel),
+ &(cooked->attnum),
+ 1,
+ InvalidOid,
+ InvalidOid,
+ InvalidOid,
+ NULL,
+ InvalidOid,
+ InvalidOid,
+ InvalidOid,
+ 0,
+ ' ',
+ ' ',
+ ' ',
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ cooked->is_local,
+ cooked->inhcount
+ );
+
+ }
+
+ /*
* Store a check-constraint expression for the given relation.
*
* Caller is responsible for updating the count of constraints
*************** StoreConstraints(Relation rel, List *coo
*** 1837,1842 ****
--- 1877,1885 ----
switch (con->contype)
{
+ case CONSTR_NOTNULL:
+ StoreColumnNotNullConstraint(rel, con);
+ break;
case CONSTR_DEFAULT:
StoreAttrDefault(rel, con->attnum, con->expr);
break;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c009711..171e8b5 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*************** typedef struct NewColumnValue
*** 175,180 ****
--- 175,192 ----
} NewColumnValue;
/*
+ * Struct describing the constraint information
+ * to de-inherit. Currently CHECK and NOT NULL constraints
+ * are carried by ATExecDropInherit() within these struct.
+ */
+ typedef struct DeinheritConstraintInfo
+ {
+ char contype; /* constraint type */
+ AttrNumber attnum; /* if NOT NULL constraint, the attnum */
+ char *conname; /* if CHECK constraint, the constraint name */
+ } DeinheritConstraintInfo;
+
+ /*
* Error-reporting support for RemoveRelations
*/
struct dropmsgstrings
*************** static List *MergeAttributes(List *schem
*** 227,234 ****
List **supOids, List **supconstr, int *supOidCount);
static bool MergeCheckConstraint(List *constraints, char *name, Node *expr);
static bool change_varattnos_walker(Node *node, const AttrNumber *newattno);
! static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel);
! static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel);
static void StoreCatalogInheritance(Oid relationId, List *supers);
static void StoreCatalogInheritance1(Oid relationId, Oid parentOid,
int16 seqNumber, Relation inhRelation);
--- 239,248 ----
List **supOids, List **supconstr, int *supOidCount);
static bool MergeCheckConstraint(List *constraints, char *name, Node *expr);
static bool change_varattnos_walker(Node *node, const AttrNumber *newattno);
! static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel,
! List **child_attnums);
! static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel,
! List *child_attnums);
static void StoreCatalogInheritance(Oid relationId, List *supers);
static void StoreCatalogInheritance1(Oid relationId, Oid parentOid,
int16 seqNumber, Relation inhRelation);
*************** static void ATExecAddColumn(AlteredTable
*** 276,285 ****
ColumnDef *colDef, bool isOid, LOCKMODE lockmode);
static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid);
static void ATPrepAddOids(List **wqueue, Relation rel, bool recurse,
! AlterTableCmd *cmd, LOCKMODE lockmode);
! static void ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode);
! static void ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
! const char *colName, LOCKMODE lockmode);
static void ATExecColumnDefault(Relation rel, const char *colName,
Node *newDefault, LOCKMODE lockmode);
static void ATPrepSetStatistics(Relation rel, const char *colName,
--- 290,304 ----
ColumnDef *colDef, bool isOid, LOCKMODE lockmode);
static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid);
static void ATPrepAddOids(List **wqueue, Relation rel, bool recurse,
! AlterTableCmd *cmd, LOCKMODE lockmode);
! static void ATExecDropNotNull(Relation rel, const char *colName, DropBehavior behavior,
! bool recurse, bool recursing, LOCKMODE lockmode);
! static void ATExecSetNotNull(List **wqueue, AlteredTableInfo *tab, Relation rel,
! const char *colName, bool recurse, bool recursing,
! LOCKMODE lockmode);
! static void ATExecSetNotNullInternal(Relation rel, Relation attr_rel,
! HeapTuple atttup, bool *is_new_constraint,
! bool is_local);
static void ATExecColumnDefault(Relation rel, const char *colName,
Node *newDefault, LOCKMODE lockmode);
static void ATPrepSetStatistics(Relation rel, const char *colName,
*************** static void ATExecDropInherit(Relation r
*** 336,342 ****
static void copy_relation_data(SMgrRelation rel, SMgrRelation dst,
ForkNumber forkNum, bool istemp);
static const char *storage_name(char c);
!
/* ----------------------------------------------------------------
* DefineRelation
--- 355,368 ----
static void copy_relation_data(SMgrRelation rel, SMgrRelation dst,
ForkNumber forkNum, bool istemp);
static const char *storage_name(char c);
! static bool ATExecDropNotNullInternal(Relation rel, HeapTuple constrtup,
! DropBehavior behavior,
! bool recurse, bool recursing);
! static bool
! CheckNotNullOnAttributeName(Relation rel, const char *colname,
! AttrNumber *attnum);
! static void
! DropNotNullOnAttributeNum(Relation rel, AttrNumber attnum, bool lock);
/* ----------------------------------------------------------------
* DefineRelation
*************** DefineRelation(CreateStmt *stmt, char re
*** 508,513 ****
--- 534,560 ----
attnum++;
+ if (colDef->is_not_null) {
+ /*
+ * Adjust NOT NULL constraint of this column
+ * to hold new attnum and inheritance information
+ *
+ * XXX: Currently the constraint is cheated into cookedDefaults
+ * list, maybe we need to invent a different machinerie/naming.
+ */
+ CookedConstraint *cooked;
+
+ cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint));
+ cooked->contype = CONSTR_NOTNULL;
+ cooked->name = ChooseConstraintName(relname, NameStr(descriptor->attrs[attnum - 1]->attname),
+ "not_null", namespaceId, NIL);
+ cooked->attnum = attnum;
+ cooked->expr = NULL;
+ cooked->is_local = colDef->is_local;
+ cooked->inhcount = colDef->inhcount;
+ cookedDefaults = lappend(cookedDefaults, cooked);
+ }
+
if (colDef->raw_default != NULL)
{
RawColumnDefault *rawEnt;
*************** ATPrepCmd(List **wqueue, Relation rel, A
*** 2682,2696 ****
/* No command-specific prep needed */
pass = cmd->def ? AT_PASS_ADD_CONSTR : AT_PASS_DROP;
break;
! case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
ATSimplePermissions(rel, false, false);
! 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, false, false);
! ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
/* No command-specific prep needed */
pass = AT_PASS_ADD_CONSTR;
break;
--- 2729,2748 ----
/* No command-specific prep needed */
pass = cmd->def ? AT_PASS_ADD_CONSTR : AT_PASS_DROP;
break;
! case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
ATSimplePermissions(rel, false, false);
!
! if (recurse)
! cmd->subtype = AT_DropNotNullRecurse;
/* No command-specific prep needed */
pass = AT_PASS_DROP;
break;
case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */
ATSimplePermissions(rel, false, false);
!
! if (recurse)
! cmd->subtype = AT_SetNotNullRecurse;
!
/* No command-specific prep needed */
pass = AT_PASS_ADD_CONSTR;
break;
*************** ATExecCmd(List **wqueue, AlteredTableInf
*** 2914,2923 ****
ATExecColumnDefault(rel, cmd->name, cmd->def, lockmode);
break;
case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
! ATExecDropNotNull(rel, cmd->name, lockmode);
break;
case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */
! ATExecSetNotNull(tab, rel, cmd->name, lockmode);
break;
case AT_SetStatistics: /* ALTER COLUMN SET STATISTICS */
ATExecSetStatistics(rel, cmd->name, cmd->def, lockmode);
--- 2966,2983 ----
ATExecColumnDefault(rel, cmd->name, cmd->def, lockmode);
break;
case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
! ATExecDropNotNull(rel, cmd->name, cmd->behavior,
! false, false, lockmode);
! break;
! case AT_DropNotNullRecurse: /* ALTER COLUMN DROP NOT NULL with recursion */
! ATExecDropNotNull(rel, cmd->name, cmd->behavior,
! true, false, lockmode);
break;
case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */
! ATExecSetNotNull(wqueue, tab, rel, cmd->name, false, false, lockmode);
! break;
! case AT_SetNotNullRecurse: /* ALTER COLUMN SET NOT NULL recursing */
! ATExecSetNotNull(wqueue, tab, rel, cmd->name, true, false, lockmode);
break;
case AT_SetStatistics: /* ALTER COLUMN SET STATISTICS */
ATExecSetStatistics(rel, cmd->name, cmd->def, lockmode);
*************** ATRewriteTable(AlteredTableInfo *tab, Oi
*** 3441,3448 ****
if (heap_attisnull(tuple, attn + 1))
ereport(ERROR,
(errcode(ERRCODE_NOT_NULL_VIOLATION),
! errmsg("column \"%s\" contains null values",
! NameStr(newTupDesc->attrs[attn]->attname))));
}
foreach(l, tab->constraints)
--- 3501,3509 ----
if (heap_attisnull(tuple, attn + 1))
ereport(ERROR,
(errcode(ERRCODE_NOT_NULL_VIOLATION),
! errmsg("column \"%s\" of relation \"%s\" contains null values",
! NameStr(newTupDesc->attrs[attn]->attname),
! RelationGetRelationName(oldrel))));
}
foreach(l, tab->constraints)
*************** ATPrepAddOids(List **wqueue, Relation re
*** 4179,4225 ****
* ALTER TABLE ALTER COLUMN DROP NOT NULL
*/
static void
! ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
{
! HeapTuple tuple;
AttrNumber attnum;
! Relation attr_rel;
! List *indexoidlist;
! ListCell *indexoidscan;
/*
! * lookup the attribute
*/
! attr_rel = heap_open(AttributeRelationId, RowExclusiveLock);
! tuple = SearchSysCacheCopyAttName(RelationGetRelid(rel), colName);
! if (!HeapTupleIsValid(tuple))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" of relation \"%s\" does not exist",
! colName, RelationGetRelationName(rel))));
! attnum = ((Form_pg_attribute) GETSTRUCT(tuple))->attnum;
/* Prevent them from altering a system attribute */
! if (attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg("cannot alter system column \"%s\"",
! colName)));
/*
! * Check that the attribute is not in a primary key
*/
/* Loop over all indexes on the relation */
indexoidlist = RelationGetIndexList(rel);
!
foreach(indexoidscan, indexoidlist)
{
! Oid indexoid = lfirst_oid(indexoidscan);
! HeapTuple indexTuple;
Form_pg_index indexStruct;
int i;
--- 4240,4588 ----
* ALTER TABLE ALTER COLUMN DROP NOT NULL
*/
static void
! ATExecDropNotNull(Relation rel, const char *colName, DropBehavior behavior,
! bool recurse, bool recursing, LOCKMODE lockmode)
{
! List *children;
! ListCell *child;
! HeapTuple constr_tuple;
AttrNumber attnum;
! Relation constr_rel;
! SysScanDesc scan;
! ScanKeyData key;
! bool found;
!
! found = false;
! children = NIL;
!
! if (recursing)
! ATSimplePermissions(rel, false, false);
/*
! * Lookup the attribute. This also checks for a dropped column
! * or any attempts to alter a system column. Returns false in case
! * no further work needs to be done (e.g. no NOT NULL present).
*/
! if (!CheckNotNullOnAttributeName(rel, colName, &attnum))
! return;
! /*
! * Scan through our constraint records, looking for the matching
! * NOT NULL constraint.
! */
! constr_rel = heap_open(ConstraintRelationId, RowExclusiveLock);
! ScanKeyInit(&key,
! Anum_pg_constraint_conrelid,
! BTEqualStrategyNumber, F_OIDEQ,
! ObjectIdGetDatum(RelationGetRelid(rel)));
! scan = systable_beginscan(constr_rel, ConstraintRelidIndexId,
! true, SnapshotNow, 1, &key);
!
! while(HeapTupleIsValid(constr_tuple = systable_getnext(scan)))
! {
! Form_pg_constraint constrStruct;
! Datum arrayp;
! Datum *keyvals;
! int nelems;
! bool isnull;
! constrStruct = (Form_pg_constraint) GETSTRUCT(constr_tuple);
!
! if (constrStruct->contype != CONSTRAINT_NOTNULL)
! continue;
!
! /*
! * Check wether this constraint is attached to the given column.
! * ATExecSetNotNull() ensures that only one NOT NULL constraint tuple
! * lives per attribute.
! */
! arrayp = SysCacheGetAttr(CONSTROID, constr_tuple, Anum_pg_constraint_conkey, &isnull);
!
! /* should not happen */
! Assert(!isnull);
!
! deconstruct_array(DatumGetArrayTypeP(arrayp), INT2OID, 2, true,
! 's', &keyvals, NULL, &nelems);
!
! /* Is this NOT NULL constraint attached to the current tuple? */
! if (DatumGetInt16(keyvals[0]) != attnum)
! continue;
!
! /*
! * Check if the attribute is inherited from another relation or
! * part of a primary key.
! * If true, error out, otherwise perform the deletion of the constraint.
! *
! * Note: ATExecDropNotNullInternal() performs a CCI, if required.
! */
! if ((found = ATExecDropNotNullInternal(rel,
! constr_tuple,
! behavior,
! recurse, recursing)))
! {
! /*
! * Okay, actually remove the NOT NULL from pg_attribute.
! * CheckNotNullOnAttributeName() already holds a lock on pg_attribute, so
! * override an additional lock here.
! */
! DropNotNullOnAttributeNum(rel, attnum, false);
! break;
! }
! }
!
! systable_endscan(scan);
!
! if (found)
! {
! /*
! * Propagate to children as appropriate.
! */
! children = find_inheritance_children(RelationGetRelid(rel),
! AccessExclusiveLock);
! }
! else
! {
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_OBJECT),
! errmsg("column \"%s\" of relation \"%s\" has no NOT NULL constraint",
! colName,
! RelationGetRelationName(rel))));
! }
!
! foreach(child, children)
! {
! Oid childrelid = lfirst_oid(child);
! Relation childrel;
!
! childrel = heap_open(childrelid, NoLock);
! CheckTableNotInUse(childrel, "ALTER TABLE");
!
! ScanKeyInit(&key,
! Anum_pg_constraint_conrelid,
! BTEqualStrategyNumber, F_OIDEQ,
! ObjectIdGetDatum(childrelid));
! scan = systable_beginscan(constr_rel, ConstraintRelidIndexId,
! true, SnapshotNow, 1, &key);
!
! while (HeapTupleIsValid(constr_tuple = systable_getnext(scan)))
! {
! HeapTuple copy_tuple;
! Form_pg_constraint constrStruct;
! Datum arrayp;
! Datum *keyvals;
! int nelems;
! bool isnull;
!
! constrStruct = (Form_pg_constraint) GETSTRUCT(constr_tuple);
!
! if (constrStruct->contype != CONSTRAINT_NOTNULL)
! continue;
!
! /*
! * Check wether this constraint is attached to the given column.
! * ATExecSetNotNull() ensures that only one NOT NULL constraint tuple
! * lives per attribute.
! */
! arrayp = SysCacheGetAttr(CONSTROID, constr_tuple, Anum_pg_constraint_conkey, &isnull);
!
! /* should not happen */
! Assert(!isnull);
!
! deconstruct_array(DatumGetArrayTypeP(arrayp), INT2OID, 2, true,
! 's', &keyvals, NULL, &nelems);
!
! /* Is this NOT NULL constraint attached to the current tuple? */
! if (DatumGetInt16(keyvals[0]) != attnum)
! continue;
!
! found = true;
!
! if (constrStruct->coninhcount <= 0) /* shouldn't happen */
! elog(ERROR, "relation %u has non-inherited NOT NULL constraint \"%s\"",
! childrelid, NameStr(constrStruct->conname));
!
! copy_tuple = heap_copytuple(constr_tuple);
! constrStruct = (Form_pg_constraint) GETSTRUCT(copy_tuple);
!
! if (recurse)
! {
! /*
! * If the child constraint has other definition sources, just
! * decrement its inheritance count; if not, recurse to delete
! * it.
! */
! if (constrStruct->coninhcount == 1 && !constrStruct->conislocal)
! {
! /* Time to delete this child constraint, too */
! ATExecDropNotNull(childrel, colName, behavior,
! true, true, lockmode);
! }
! else
! {
! /* Child constraint must survive my deletion */
! constrStruct->coninhcount--;
! simple_heap_update(constr_rel, ©_tuple->t_self, copy_tuple);
! CatalogUpdateIndexes(constr_rel, copy_tuple);
!
! /* Make update visible */
! CommandCounterIncrement();
! }
! }
! else
! {
! /*
! * If we were told to drop ONLY in this table (no recursion),
! * we need to mark the inheritors' constraints as locally
! * defined rather than inherited.
! */
! constrStruct->coninhcount--;
! constrStruct->conislocal = true;
!
! simple_heap_update(constr_rel, ©_tuple->t_self, copy_tuple);
! CatalogUpdateIndexes(constr_rel, copy_tuple);
!
! /* Make update visible */
! CommandCounterIncrement();
! }
!
! heap_freetuple(copy_tuple);
!
! }
!
! systable_endscan(scan);
! heap_close(childrel, NoLock);
!
! }
!
! heap_close(constr_rel, NoLock);
!
! }
!
! /*
! * Does the leg work on dropping a NOT NULL constraint
! * identified by the given constrtup tuple.
! *
! * Note: The caller is responsible to pass a valid
! * CONSTRAINT_NOTNULL tuple.
! */
! static bool
! ATExecDropNotNullInternal(Relation rel, HeapTuple constrtup,
! DropBehavior behavior,
! bool recurse, bool recursing)
! {
! bool found;
! Form_pg_constraint constrStruct;
! ObjectAddress conobj;
!
! found = false;
!
! if (HeapTupleIsValid(constrtup))
! {
!
! constrStruct = (Form_pg_constraint) GETSTRUCT(constrtup);
!
! /*
! * Be paranoid, caller is responsible to pass a valid HeapTuple.
! */
! Assert(constrStruct->contype == CONSTRAINT_NOTNULL);
!
! /* It is okay to drop this constraint when recursing */
! if ((constrStruct->coninhcount > 0) && (!recursing))
! {
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
! errmsg("cannot drop inherited NOT NULL constraint \"%s\", relation \"%s\"",
! NameStr(constrStruct->conname), RelationGetRelationName(rel))));
! }
!
! /*
! * Time to drop the constraint now.
! */
! conobj.classId = ConstraintRelationId;
! conobj.objectId = HeapTupleGetOid(constrtup);
! conobj.objectSubId = 0;
!
! performDeletion(&conobj, behavior);
!
! /*
! * Make sure changes are visible
! */
! CommandCounterIncrement();
! found = true;
!
! }
!
! return found;
! }
!
! /*
! * Checks wether the given attribute name has a NOT NULL constraint attached
! * which can be dropped.
! *
! * Any attempts to drop a NOT NULL constraint on a system
! * column or non-existing column will throw an error. Returns true
! * in case the caller is allowed to proceed, false if no NOT NULL flag
! * was set on the attribute.
! */
! static bool
! CheckNotNullOnAttributeName(Relation rel, const char *colname, AttrNumber *attnum)
! {
! Relation attr_rel;
! HeapTuple attr_tuple;
! Form_pg_attribute attr_struct;
! bool has_not_null;
! List *indexoidlist;
! ListCell *indexoidscan;
!
! has_not_null = false;
!
! attr_rel = heap_open(AttributeRelationId, RowExclusiveLock);
! attr_tuple = SearchSysCacheCopyAttName(RelationGetRelid(rel), colname);
!
! if (!HeapTupleIsValid(attr_tuple))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" of relation \"%s\" does not exist",
! colname,
! RelationGetRelationName(rel))));
! /* attribute exists */
! attr_struct = (Form_pg_attribute) GETSTRUCT(attr_tuple);
/* Prevent them from altering a system attribute */
! if (attr_struct->attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg("cannot alter system column \"%s\" of relation \"%s\"",
! colname,
! RelationGetRelationName(rel))));
/*
! * Check for dropped attributes
*/
+ if (attr_struct->attisdropped)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("attempt to drop NOT NULL constraint on dropped column \"%s\" of relation \"%s\"",
+ colname,
+ RelationGetRelationName(rel))));
+ }
+ if (attnum)
+ *attnum = attr_struct->attnum;
+
+ /*
+ * Check that the attribute is not in a primary key
+ */
+
/* Loop over all indexes on the relation */
indexoidlist = RelationGetIndexList(rel);
!
foreach(indexoidscan, indexoidlist)
{
! Oid indexoid = lfirst_oid(indexoidscan);
! HeapTuple indexTuple;
Form_pg_index indexStruct;
int i;
*************** ATExecDropNotNull(Relation rel, const ch
*** 4227,4233 ****
if (!HeapTupleIsValid(indexTuple))
elog(ERROR, "cache lookup failed for index %u", indexoid);
indexStruct = (Form_pg_index) GETSTRUCT(indexTuple);
!
/* If the index is not a primary key, skip the check */
if (indexStruct->indisprimary)
{
--- 4590,4596 ----
if (!HeapTupleIsValid(indexTuple))
elog(ERROR, "cache lookup failed for index %u", indexoid);
indexStruct = (Form_pg_index) GETSTRUCT(indexTuple);
!
/* If the index is not a primary key, skip the check */
if (indexStruct->indisprimary)
{
*************** ATExecDropNotNull(Relation rel, const ch
*** 4237,4281 ****
*/
for (i = 0; i < indexStruct->indnatts; i++)
{
! if (indexStruct->indkey.values[i] == attnum)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("column \"%s\" is in a primary key",
! colName)));
}
}
!
ReleaseSysCache(indexTuple);
}
! list_free(indexoidlist);
!
! /*
! * Okay, actually perform the catalog change ... if needed
*/
! if (((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull)
! {
! ((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull = FALSE;
! simple_heap_update(attr_rel, &tuple->t_self, tuple);
! /* keep the system catalog indexes current */
! CatalogUpdateIndexes(attr_rel, tuple);
! }
- heap_close(attr_rel, RowExclusiveLock);
}
/*
* ALTER TABLE ALTER COLUMN SET NOT NULL
*/
static void
! ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
! const char *colName, LOCKMODE lockmode)
{
HeapTuple tuple;
AttrNumber attnum;
Relation attr_rel;
/*
* lookup the attribute
--- 4600,4684 ----
*/
for (i = 0; i < indexStruct->indnatts; i++)
{
! if (indexStruct->indkey.values[i] == attr_struct->attnum)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("column \"%s\" is in a primary key",
! colname)));
}
}
!
ReleaseSysCache(indexTuple);
}
! /*
! * In case no NOT NULL constraint was set on the
! * column, give up and tell the caller we haven't done
! * anything.
*/
! has_not_null = ((!attr_struct->attnotnull) ? false : true);
! heap_freetuple(attr_tuple);
! heap_close(attr_rel, NoLock);
! return has_not_null;
! }
! /*
! * Drops a NOT NULL constraint from the given attribute number of the
! * specified relation.
! */
! static void
! DropNotNullOnAttributeNum(Relation rel, AttrNumber attnum, bool lock)
! {
! Relation attr_rel;
! HeapTuple attr_tuple;
! Form_pg_attribute attr_struct;
!
! if (lock)
! attr_rel = heap_open(AttributeRelationId, RowExclusiveLock);
! else
! attr_rel = heap_open(AttributeRelationId, NoLock);
!
! attr_tuple = SearchSysCacheCopy(ATTNUM,
! ObjectIdGetDatum(RelationGetRelid(rel)),
! Int16GetDatum(attnum),
! 0, 0);
!
! attr_struct = (Form_pg_attribute) GETSTRUCT(attr_tuple);
!
! /* not expected, so be paranoid */
! Assert(attr_struct->attnotnull);
!
! ((Form_pg_attribute) GETSTRUCT(attr_tuple))->attnotnull = FALSE;
! simple_heap_update(attr_rel, &attr_tuple->t_self, attr_tuple);
!
! /* keep the system catalog indexes current */
! CatalogUpdateIndexes(attr_rel, attr_tuple);
!
! heap_close(attr_rel, NoLock);
}
/*
* ALTER TABLE ALTER COLUMN SET NOT NULL
*/
static void
! ATExecSetNotNull(List **wqueue, AlteredTableInfo *tab, Relation rel,
! const char *colName, bool recurse, bool recursing,
! LOCKMODE lockmode)
{
HeapTuple tuple;
AttrNumber attnum;
Relation attr_rel;
+ List *children;
+ ListCell *child;
+ bool is_new_constraint;
+
+ if (recursing)
+ ATSimplePermissions(rel, false, false);
+
+ children = find_inheritance_children(RelationGetRelid(rel),
+ AccessExclusiveLock);
/*
* lookup the attribute
*************** ATExecSetNotNull(AlteredTableInfo *tab,
*** 4296,4321 ****
if (attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg("cannot alter system column \"%s\"",
! colName)));
/*
! * Okay, actually perform the catalog change ... if needed
*/
! if (!((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull)
{
! ((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull = TRUE;
! simple_heap_update(attr_rel, &tuple->t_self, tuple);
! /* keep the system catalog indexes current */
! CatalogUpdateIndexes(attr_rel, tuple);
! /* Tell Phase 3 it needs to test the constraint */
! tab->new_notnull = true;
}
! heap_close(attr_rel, RowExclusiveLock);
}
/*
--- 4699,4911 ----
if (attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg("cannot alter system column \"%s\" of relation \"%s\"",
! colName, RelationGetRelationName(rel))));
/*
! * Treat it an error if there's an attempt to add
! * a NOT NULL constraint to a table but not to
! * existing child tables (thus, ALTER TABLE ONLY
! * was specified to a table inherited from others).
! * Maybe we should treat this as a no op, but it seems
! * better to inform the user that he's doing something
! * wrong.
*/
! if (!recurse)
! {
! if (children)
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
! errmsg("NOT NULL constraint must be added to child tables too")));
! }
!
! /*
! * Okay, actually perform the catalog changes ...
! *
! * Setting attnotnull to TRUE requires to create a
! * constraint tuple in pg_constraint as well. We do this
! * to record wether this constraint was added locally or
! * inherited by some other attribute. We also record the
! * attribute number to determine which column this
! * constraint belongs to. We need to take care wether
! * the constraint was added locally to a child table or
! * we're currently already recursing through an inheritance
! * tree.
! */
! ATExecSetNotNullInternal(rel, attr_rel, tuple, &is_new_constraint, !recursing);
!
! /* Tell Phase 3 it needs to test the constraint */
! tab->new_notnull = true;
!
! heap_freetuple(tuple);
! heap_close(attr_rel, RowExclusiveLock);
!
! /*
! * Loop through all child relations
! */
! foreach(child, children)
{
! Oid childrelid = lfirst_oid(child);
! Relation childrel;
! AlteredTableInfo *childtab;
! attr_rel = heap_open(AttributeRelationId, RowExclusiveLock);
! childrel = heap_open(childrelid, NoLock);
! CheckTableNotInUse(childrel, "ALTER TABLE");
! /* get work queue entry for child relation */
! childtab = ATGetQueueEntry(wqueue, childrel);
! /*
! * Look for inherited column
! */
! tuple = SearchSysCacheCopyAttName(childrelid, colName);
!
! /* shouldn't happen */
! Assert(HeapTupleIsValid(tuple));
!
! /* perform the actual work */
! ATExecSetNotNull(wqueue, childtab, childrel,
! colName, recurse, true, lockmode);
!
! heap_freetuple(tuple);
! heap_close(attr_rel, NoLock);
! heap_close(childrel, NoLock);
}
+ }
! /*
! * Internal function to ATExecSetNotNull()
! *
! * Takes a relation and the pg_attribute relation and tuple of the
! * target column to create a NOT NULL constraint.
! * The caller is responsible to pass a valid HeapTuple.
! */
! static void
! ATExecSetNotNullInternal(Relation rel, Relation attr_rel,
! HeapTuple atttup, bool *is_new_constraint, bool is_local)
! {
! Form_pg_attribute attr;
!
! attr = (Form_pg_attribute) GETSTRUCT(atttup);
! *is_new_constraint = FALSE;
!
! if (attr->attnotnull)
! {
! SysScanDesc scan;
! ScanKeyData key;
! HeapTuple constr_tup;
! Relation constr_rel;
! bool found;
!
! found = false;
!
! /*
! * Column has already a NOT NULL constraint attached,
! * fetch its pg_constraint tuple and adjust the inheritance
! * information according to its pg_attribute tuple.
! */
! constr_rel = heap_open(ConstraintRelationId, RowExclusiveLock);
! ScanKeyInit(&key,
! Anum_pg_constraint_conrelid,
! BTEqualStrategyNumber, F_OIDEQ,
! ObjectIdGetDatum(RelationGetRelid(rel)));
! scan = systable_beginscan(constr_rel, ConstraintRelidIndexId,
! true, SnapshotNow, 1, &key);
!
! while (HeapTupleIsValid(constr_tup = systable_getnext(scan)))
! {
! Form_pg_constraint constr_struct;
! Datum arrayp;
! Datum *keyvals;
! int nelems;
! bool isnull;
!
! constr_struct = (Form_pg_constraint) GETSTRUCT(constr_tup);
!
! /* We need to look for NOT NULL constraint only */
! if (constr_struct->contype != CONSTRAINT_NOTNULL)
! continue;
!
! /* constraint tuple attached to our column? */
! arrayp = SysCacheGetAttr(CONSTROID, constr_tup,
! Anum_pg_constraint_conkey,
! &isnull);
! /* not expected here */
! Assert(!isnull);
!
! deconstruct_array(DatumGetArrayTypeP(arrayp), INT2OID, 2, true,
! 's', &keyvals, NULL, &nelems);
! if (DatumGetInt16(keyvals[0]) != attr->attnum)
! continue;
!
! /* only reached in case we have found the required constraint tuple */
! found = TRUE;
! break;
! }
!
! /* there should be a NOT NULL constraint tuple */
! if (found)
! {
! HeapTuple copy_tuple = heap_copytuple(constr_tup);
! Form_pg_constraint constr = (Form_pg_constraint) GETSTRUCT(copy_tuple);
!
! /*
! * We don't need to update the constraint tuple when
! * the inheritance counter is already up to date
! */
! if (constr->coninhcount != attr->attinhcount)
! {
! constr->coninhcount = is_local ? 0 : attr->attinhcount;
! simple_heap_update(constr_rel, ©_tuple->t_self, copy_tuple);
!
! /*
! * Keep system catalog indexes current and make
! * changes visible
! */
! CatalogUpdateIndexes(constr_rel, copy_tuple);
! CommandCounterIncrement();
! *is_new_constraint = TRUE;
! }
!
! heap_freetuple(copy_tuple);
! }
!
! systable_endscan(scan);
! heap_close(constr_rel, NoLock);
!
! }
! else
! {
! CookedConstraint *cooked;
!
! /*
! * New NOT NULL constraint
! */
! cooked = palloc(sizeof(CookedConstraint));
! cooked->contype = CONSTR_NOTNULL;
! cooked->expr = NULL;
! cooked->name = ChooseConstraintName(RelationGetRelationName(rel),
! NameStr(attr->attname),
! "not_null", RelationGetNamespace(rel),
! NIL);
! cooked->attnum = attr->attnum;
! cooked->is_local = is_local;
! cooked->inhcount = is_local ? 0 : 1;
!
! StoreColumnNotNullConstraint(rel, cooked);
!
! /* Make changes visible */
! CommandCounterIncrement();
!
! attr->attnotnull = TRUE;
! simple_heap_update(attr_rel, &atttup->t_self, atttup);
!
! /* keep the system catalog indexes current */
! CatalogUpdateIndexes(attr_rel, atttup);
!
! *is_new_constraint = TRUE;
! }
}
/*
*************** ATExecDropConstraint(Relation rel, const
*** 5893,5898 ****
--- 6483,6492 ----
con = (Form_pg_constraint) GETSTRUCT(tuple);
+ /* NOT NULL is handled by ALTER TABLE ... DROP/SET NOT NULL */
+ if (con->contype == CONSTRAINT_NOTNULL)
+ continue;
+
if (strcmp(NameStr(con->conname), constrName) != 0)
continue;
*************** ATExecDropConstraint(Relation rel, const
*** 5903,5909 ****
errmsg("cannot drop inherited constraint \"%s\" of relation \"%s\"",
constrName, RelationGetRelationName(rel))));
! /* Right now only CHECK constraints can be inherited */
if (con->contype == CONSTRAINT_CHECK)
is_check_constraint = true;
--- 6497,6505 ----
errmsg("cannot drop inherited constraint \"%s\" of relation \"%s\"",
constrName, RelationGetRelationName(rel))));
! /* This is a CHECK constraint, remember that we found one
! * and proceed to drop it
! */
if (con->contype == CONSTRAINT_CHECK)
is_check_constraint = true;
*************** ATExecDropConstraint(Relation rel, const
*** 5974,5980 ****
con = (Form_pg_constraint) GETSTRUCT(tuple);
! /* Right now only CHECK constraints can be inherited */
if (con->contype != CONSTRAINT_CHECK)
continue;
--- 6570,6579 ----
con = (Form_pg_constraint) GETSTRUCT(tuple);
! /* Besides CHECK constraint we support inheritance of
! * NOT NULL constraints, too. Ignore them here, since they
! * are handled by ALTER TABLE...DROP/SET NOT NULL
! */
if (con->contype != CONSTRAINT_CHECK)
continue;
*************** ATPostAlterTypeParse(char *cmd, List **w
*** 6701,6706 ****
--- 7300,7306 ----
tab->subcmds[AT_PASS_OLD_INDEX] =
lappend(tab->subcmds[AT_PASS_OLD_INDEX], cmd);
break;
+ case AT_SetNotNull:
case AT_AddConstraint:
tab->subcmds[AT_PASS_OLD_CONSTR] =
lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
*************** ATExecAddInherit(Relation child_rel, Ran
*** 7462,7467 ****
--- 8062,8068 ----
HeapTuple inheritsTuple;
int32 inhseqno;
List *children;
+ List *child_attnums;
/*
* AccessShareLock on the parent is what's obtained during normal CREATE
*************** ATExecAddInherit(Relation child_rel, Ran
*** 7548,7557 ****
RelationGetRelationName(parent_rel))));
/* Match up the columns and bump attinhcount as needed */
! MergeAttributesIntoExisting(child_rel, parent_rel);
/* Match up the constraints and bump coninhcount as needed */
! MergeConstraintsIntoExisting(child_rel, parent_rel);
/*
* OK, it looks valid. Make the catalog entries that show inheritance.
--- 8149,8159 ----
RelationGetRelationName(parent_rel))));
/* Match up the columns and bump attinhcount as needed */
! child_attnums = NIL;
! MergeAttributesIntoExisting(child_rel, parent_rel, &child_attnums);
/* Match up the constraints and bump coninhcount as needed */
! MergeConstraintsIntoExisting(child_rel, parent_rel, child_attnums);
/*
* OK, it looks valid. Make the catalog entries that show inheritance.
*************** constraints_equivalent(HeapTuple a, Heap
*** 7627,7633 ****
* the child must be as well. Defaults are not compared, however.
*/
static void
! MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel)
{
Relation attrrel;
AttrNumber parent_attno;
--- 8229,8236 ----
* the child must be as well. Defaults are not compared, however.
*/
static void
! MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel,
! List **attnums_list)
{
Relation attrrel;
AttrNumber parent_attno;
*************** MergeAttributesIntoExisting(Relation chi
*** 7674,7679 ****
--- 8277,8287 ----
attributeName)));
/*
+ * Record the child's inherited attnum in attnums_list.
+ */
+ *attnums_list = lappend_int(*attnums_list, childatt->attnum);
+
+ /*
* OK, bump the child column's inheritance count. (If we fail
* later on, this change will just roll back.)
*/
*************** MergeAttributesIntoExisting(Relation chi
*** 7712,7718 ****
* a problem though. Even 100 constraints ought not be the end of the world.
*/
static void
! MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel)
{
Relation catalog_relation;
TupleDesc tuple_desc;
--- 8320,8327 ----
* a problem though. Even 100 constraints ought not be the end of the world.
*/
static void
! MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel,
! List *child_attnums)
{
Relation catalog_relation;
TupleDesc tuple_desc;
*************** MergeConstraintsIntoExisting(Relation ch
*** 7739,7745 ****
HeapTuple child_tuple;
bool found = false;
! if (parent_con->contype != CONSTRAINT_CHECK)
continue;
/* Search for a child constraint matching this one */
--- 8348,8355 ----
HeapTuple child_tuple;
bool found = false;
! if (parent_con->contype != CONSTRAINT_CHECK
! && parent_con->contype != CONSTRAINT_NOTNULL)
continue;
/* Search for a child constraint matching this one */
*************** MergeConstraintsIntoExisting(Relation ch
*** 7755,7773 ****
Form_pg_constraint child_con = (Form_pg_constraint) GETSTRUCT(child_tuple);
HeapTuple child_copy;
! if (child_con->contype != CONSTRAINT_CHECK)
! continue;
! if (strcmp(NameStr(parent_con->conname),
! NameStr(child_con->conname)) != 0)
! continue;
! if (!constraints_equivalent(parent_tuple, child_tuple, tuple_desc))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("child table \"%s\" has different definition for check constraint \"%s\"",
! RelationGetRelationName(child_rel),
! NameStr(parent_con->conname))));
/*
* OK, bump the child constraint's inheritance count. (If we fail
--- 8365,8411 ----
Form_pg_constraint child_con = (Form_pg_constraint) GETSTRUCT(child_tuple);
HeapTuple child_copy;
! switch(child_con->contype)
! {
! case CONSTRAINT_CHECK:
! {
! if (strcmp(NameStr(parent_con->conname),
! NameStr(child_con->conname)) != 0)
! continue;
!
! if (!constraints_equivalent(parent_tuple, child_tuple, tuple_desc))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("child table \"%s\" has different definition for check constraint \"%s\"",
! RelationGetRelationName(child_rel),
! NameStr(parent_con->conname))));
! break;
! }
! case CONSTRAINT_NOTNULL:
! {
! Datum arrayp;
! Datum *vals;
! int nelems;
! bool isnull;
! if (child_attnums == NIL)
! continue;
! arrayp = SysCacheGetAttr(CONSTROID, child_tuple,
! Anum_pg_constraint_conkey, &isnull);
! /* should not happen */
! Assert(!isnull);
! deconstruct_array(DatumGetArrayTypeP(arrayp), INT2OID, 2, true,
! 's', &vals, NULL, &nelems);
! Assert(nelems > 0);
!
! if (!list_member_int(child_attnums, DatumGetInt16(vals[0])))
! continue;
! break;
! }
! default:
! continue;
! }
/*
* OK, bump the child constraint's inheritance count. (If we fail
*************** MergeConstraintsIntoExisting(Relation ch
*** 7810,7817 ****
* surprise. But at least we'll never surprise by dropping columns someone
* isn't expecting to be dropped which would actually mean data loss.
*
! * coninhcount and conislocal for inherited constraints are adjusted in
! * exactly the same way.
*/
static void
ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode)
--- 8448,8455 ----
* surprise. But at least we'll never surprise by dropping columns someone
* isn't expecting to be dropped which would actually mean data loss.
*
! * coninhcount and conislocal for inherited CHECK and NOT NULL constraints
! * are adjusted in exactly the same way.
*/
static void
ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode)
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7824,7830 ****
attributeTuple,
constraintTuple,
depTuple;
! List *connames;
bool found = false;
/*
--- 8462,8468 ----
attributeTuple,
constraintTuple,
depTuple;
! List *constraints;
bool found = false;
/*
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7874,7879 ****
--- 8512,8519 ----
RelationGetRelationName(parent_rel),
RelationGetRelationName(rel))));
+ constraints = NIL;
+
/*
* Search through child columns looking for ones matching parent rel
*/
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7887,7892 ****
--- 8527,8533 ----
while (HeapTupleIsValid(attributeTuple = systable_getnext(scan)))
{
Form_pg_attribute att = (Form_pg_attribute) GETSTRUCT(attributeTuple);
+ HeapTuple parentattr_tuple;
/* Ignore if dropped or not inherited */
if (att->attisdropped)
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7894,7922 ****
if (att->attinhcount <= 0)
continue;
! if (SearchSysCacheExistsAttName(RelationGetRelid(parent_rel),
! NameStr(att->attname)))
{
/* Decrement inhcount and possibly set islocal to true */
HeapTuple copyTuple = heap_copytuple(attributeTuple);
Form_pg_attribute copy_att = (Form_pg_attribute) GETSTRUCT(copyTuple);
!
copy_att->attinhcount--;
if (copy_att->attinhcount == 0)
copy_att->attislocal = true;
simple_heap_update(catalogRelation, ©Tuple->t_self, copyTuple);
CatalogUpdateIndexes(catalogRelation, copyTuple);
heap_freetuple(copyTuple);
}
}
systable_endscan(scan);
heap_close(catalogRelation, RowExclusiveLock);
/*
! * Likewise, find inherited check constraints and disinherit them. To do
! * this, we first need a list of the names of the parent's check
! * constraints. (We cheat a bit by only checking for name matches,
* assuming that the expressions will match.)
*/
catalogRelation = heap_open(ConstraintRelationId, RowExclusiveLock);
--- 8535,8584 ----
if (att->attinhcount <= 0)
continue;
! parentattr_tuple = SearchSysCacheCopyAttName(RelationGetRelid(parent_rel),
! NameStr(att->attname));
!
! if (HeapTupleIsValid(parentattr_tuple))
{
/* Decrement inhcount and possibly set islocal to true */
HeapTuple copyTuple = heap_copytuple(attributeTuple);
Form_pg_attribute copy_att = (Form_pg_attribute) GETSTRUCT(copyTuple);
! Form_pg_attribute parent_att = (Form_pg_attribute) GETSTRUCT(parentattr_tuple);
!
copy_att->attinhcount--;
if (copy_att->attinhcount == 0)
copy_att->attislocal = true;
+
+ /*
+ * If attnotnull is set, record the inherited
+ * not null constraint. We save its attnum to deinherit
+ * its representation in pg_constraint below, but only when
+ * its ancestor has a NOT NULL constraint, too.
+ */
+ if (copy_att->attnotnull
+ && parent_att->attnotnull)
+ {
+ DeinheritConstraintInfo *inhInfo
+ = (DeinheritConstraintInfo *)palloc(sizeof(DeinheritConstraintInfo));
+ inhInfo->contype = CONSTRAINT_NOTNULL;
+ inhInfo->conname = NULL;
+ inhInfo->attnum = copy_att->attnum;
+ constraints = lappend(constraints, inhInfo);
+ }
simple_heap_update(catalogRelation, ©Tuple->t_self, copyTuple);
CatalogUpdateIndexes(catalogRelation, copyTuple);
heap_freetuple(copyTuple);
+ heap_freetuple(parentattr_tuple);
}
}
systable_endscan(scan);
heap_close(catalogRelation, RowExclusiveLock);
/*
! * Likewise, find inherited check and NOT NULL constraints and disinherit
! * them. To do this, we first need a list of the names of the parent's check
! * and NOT NULL constraints. (We cheat a bit by only checking for name matches,
* assuming that the expressions will match.)
*/
catalogRelation = heap_open(ConstraintRelationId, RowExclusiveLock);
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7927,7940 ****
scan = systable_beginscan(catalogRelation, ConstraintRelidIndexId,
true, SnapshotNow, 1, key);
- connames = NIL;
-
while (HeapTupleIsValid(constraintTuple = systable_getnext(scan)))
{
Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(constraintTuple);
if (con->contype == CONSTRAINT_CHECK)
! connames = lappend(connames, pstrdup(NameStr(con->conname)));
}
systable_endscan(scan);
--- 8589,8607 ----
scan = systable_beginscan(catalogRelation, ConstraintRelidIndexId,
true, SnapshotNow, 1, key);
while (HeapTupleIsValid(constraintTuple = systable_getnext(scan)))
{
Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(constraintTuple);
if (con->contype == CONSTRAINT_CHECK)
! {
! DeinheritConstraintInfo *inhInfo
! = (DeinheritConstraintInfo *)palloc(sizeof(DeinheritConstraintInfo));
! inhInfo->contype = CONSTRAINT_CHECK;
! inhInfo->conname = pstrdup(NameStr(con->conname));
! inhInfo->attnum = 0;
! constraints = lappend(constraints, inhInfo);
! }
}
systable_endscan(scan);
*************** ATExecDropInherit(Relation rel, RangeVar
*** 7952,7969 ****
Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(constraintTuple);
bool match;
ListCell *lc;
! if (con->contype != CONSTRAINT_CHECK)
continue;
match = false;
! foreach(lc, connames)
{
! if (strcmp(NameStr(con->conname), (char *) lfirst(lc)) == 0)
{
match = true;
break;
}
}
if (match)
--- 8619,8667 ----
Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(constraintTuple);
bool match;
ListCell *lc;
+ Datum arrayp;
+ Datum *keyvals;
+ int nelems;
+ bool isnull;
! if (con->contype != CONSTRAINT_CHECK
! && con->contype != CONSTRAINT_NOTNULL)
continue;
+ /*
+ * We scan through all constraints of the current child relation,
+ * checking for CONSTRAINT_CHECK and CONSTRAINT_NOTNULL occurrences.
+ *
+ * CHECK constraints are assumed to be named equally to in the
+ * relation's ancestor, while NOT NULL constraints always are attached
+ * to a specific column. Check for attnum and adjust the inheritance
+ * information accordingly.
+ */
+
match = false;
! foreach(lc, constraints)
{
! DeinheritConstraintInfo *inhInfo = (DeinheritConstraintInfo *) lfirst(lc);
!
! if ((inhInfo->contype == CONSTRAINT_CHECK)
! && (strcmp(NameStr(con->conname), inhInfo->conname) == 0))
{
match = true;
break;
}
+ else if (inhInfo->contype == CONSTRAINT_NOTNULL)
+ {
+ arrayp = SysCacheGetAttr(CONSTROID, constraintTuple,
+ Anum_pg_constraint_conkey, &isnull);
+
+ deconstruct_array(DatumGetArrayTypeP(arrayp), INT2OID, 2, true,
+ 's', &keyvals, NULL, &nelems);
+ if (keyvals[0] == inhInfo->attnum)
+ {
+ match = true;
+ break;
+ }
+ }
}
if (match)
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 37ca331..b4d8d3d 100644
*** a/src/backend/parser/parse_utilcmd.c
--- b/src/backend/parser/parse_utilcmd.c
*************** transformColumnDefinition(ParseState *ps
*** 466,471 ****
--- 466,472 ----
parser_errposition(pstate,
constraint->location)));
column->is_not_null = TRUE;
+ cxt->ckconstraints = lappend(cxt->ckconstraints, constraint);
saw_nullable = true;
break;
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index f1c1d04..7913cf5 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
*************** pg_get_constraintdef_worker(Oid constrai
*** 1076,1088 ****
if (fullCommand && OidIsValid(conForm->conrelid))
{
! appendStringInfo(&buf, "ALTER TABLE ONLY %s ADD CONSTRAINT %s ",
! generate_relation_name(conForm->conrelid, NIL),
! quote_identifier(NameStr(conForm->conname)));
}
switch (conForm->contype)
{
case CONSTRAINT_FOREIGN:
{
Datum val;
--- 1076,1117 ----
if (fullCommand && OidIsValid(conForm->conrelid))
{
! /* XXX: TODO, not sure how to handle this right now? */
! if (conForm->contype == CONSTRAINT_NOTNULL)
! {
! appendStringInfo(&buf, "ALTER TABLE ONLY %s ALTER COLUMN ",
! generate_relation_name(conForm->conrelid, NIL));
! }
! else
! {
! appendStringInfo(&buf, "ALTER TABLE ONLY %s ADD CONSTRAINT %s ",
! generate_relation_name(conForm->conrelid, NIL),
! quote_identifier(NameStr(conForm->conname)));
! }
}
switch (conForm->contype)
{
+ case CONSTRAINT_NOTNULL:
+ {
+ Datum val;
+ bool isnull;
+
+ /* XXX: TODO, not sure how to handle this right now? */
+
+ /* Fetch referenced column OID */
+ val = SysCacheGetAttr(CONSTROID, tup,
+ Anum_pg_constraint_conkey, &isnull);
+
+ /* Should not happen */
+ if (isnull)
+ elog(ERROR, "null conkey for constraint %u",
+ constraintId);
+
+ decompile_column_index_array(val, conForm->conrelid, &buf);
+ appendStringInfo(&buf, " SET NOT NULL");
+ break;
+ }
case CONSTRAINT_FOREIGN:
{
Datum val;
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index 7795bda..4f55a57 100644
*** a/src/include/catalog/heap.h
--- b/src/include/catalog/heap.h
*************** extern List *AddRelationNewConstraints(R
*** 91,97 ****
bool is_local);
extern void StoreAttrDefault(Relation rel, AttrNumber attnum, Node *expr);
!
extern Node *cookDefault(ParseState *pstate,
Node *raw_default,
Oid atttypid,
--- 91,97 ----
bool is_local);
extern void StoreAttrDefault(Relation rel, AttrNumber attnum, Node *expr);
! extern void StoreColumnNotNullConstraint(Relation rel, CookedConstraint *cooked);
extern Node *cookDefault(ParseState *pstate,
Node *raw_default,
Oid atttypid,
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index 49e7c31..a0abc65 100644
*** a/src/include/catalog/pg_constraint.h
--- b/src/include/catalog/pg_constraint.h
*************** typedef FormData_pg_constraint *Form_pg_
*** 175,180 ****
--- 175,181 ----
/* Valid values for contype */
#define CONSTRAINT_CHECK 'c'
+ #define CONSTRAINT_NOTNULL 'n'
#define CONSTRAINT_FOREIGN 'f'
#define CONSTRAINT_PRIMARY 'p'
#define CONSTRAINT_UNIQUE 'u'
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index ca225d0..7e5b81f 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
*************** typedef enum AlterTableType
*** 1117,1123 ****
--- 1117,1125 ----
AT_AddColumnToView, /* implicitly via CREATE OR REPLACE VIEW */
AT_ColumnDefault, /* alter column default */
AT_DropNotNull, /* alter column drop not null */
+ AT_DropNotNullRecurse, /* internal to commands/tablecmds.c */
AT_SetNotNull, /* alter column set not null */
+ AT_SetNotNullRecurse, /* internal to commands/tablecmds.c */
AT_SetStatistics, /* alter column set statistics */
AT_SetOptions, /* alter column set ( options ) */
AT_ResetOptions, /* alter column reset ( options ) */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index ab19a8e..f1585e1 100644
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
*************** create table atacc1 ( test int );
*** 503,509 ****
insert into atacc1 (test) values (NULL);
-- add a primary key (fails)
alter table atacc1 add constraint atacc_test1 primary key (test);
! ERROR: column "test" contains null values
insert into atacc1 (test) values (3);
drop table atacc1;
-- let's do one where the primary key constraint fails
--- 503,509 ----
insert into atacc1 (test) values (NULL);
-- add a primary key (fails)
alter table atacc1 add constraint atacc_test1 primary key (test);
! ERROR: column "test" of relation "atacc1" contains null values
insert into atacc1 (test) values (3);
drop table atacc1;
-- let's do one where the primary key constraint fails
*************** insert into atacc1 (test) values (0);
*** 520,526 ****
-- add a primary key column without a default (fails).
alter table atacc1 add column test2 int primary key;
NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index "atacc1_pkey" for table "atacc1"
! 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;
NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index "atacc1_pkey" for table "atacc1"
--- 520,526 ----
-- add a primary key column without a default (fails).
alter table atacc1 add column test2 int primary key;
NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index "atacc1_pkey" for table "atacc1"
! ERROR: column "test2" of relation "atacc1" contains null values
-- now add a primary key column with a default (succeeds).
alter table atacc1 add column test2 int default 0 primary key;
NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index "atacc1_pkey" for table "atacc1"
*************** alter table atacc1 drop constraint "atac
*** 583,589 ****
alter table atacc1 alter column test drop not null;
insert into atacc1 values (null);
alter table atacc1 alter test set not null;
! ERROR: column "test" contains null values
delete from atacc1;
alter table atacc1 alter test set not null;
-- try altering a non-existent column, should fail
--- 583,589 ----
alter table atacc1 alter column test drop not null;
insert into atacc1 values (null);
alter table atacc1 alter test set not null;
! ERROR: column "test" of relation "atacc1" contains null values
delete from atacc1;
alter table atacc1 alter test set not null;
-- try altering a non-existent column, should fail
*************** alter table atacc1 alter bar drop not nu
*** 593,601 ****
ERROR: column "bar" of relation "atacc1" does not exist
-- try altering the oid column, should fail
alter table atacc1 alter oid set not null;
! ERROR: cannot alter system column "oid"
alter table atacc1 alter oid drop not null;
! ERROR: cannot alter system column "oid"
-- try creating a view and altering that, should fail
create view myview as select * from atacc1;
alter table myview alter column test drop not null;
--- 593,601 ----
ERROR: column "bar" of relation "atacc1" does not exist
-- try altering the oid column, should fail
alter table atacc1 alter oid set not null;
! ERROR: cannot alter system column "oid" of relation "atacc1"
alter table atacc1 alter oid drop not null;
! ERROR: cannot alter system column "oid" of relation "atacc1"
-- try creating a view and altering that, should fail
create view myview as select * from atacc1;
alter table myview alter column test drop not null;
*************** alter table parent alter a drop not null
*** 616,628 ****
insert into parent values (NULL);
insert into child (a, b) values (NULL, 'foo');
alter table only parent alter a set not null;
! ERROR: column "a" contains null values
alter table child alter a set not null;
! ERROR: column "a" contains null values
delete from parent;
alter table only parent alter a set not null;
insert into parent values (NULL);
- ERROR: null value in column "a" violates not-null constraint
alter table child alter a set not null;
insert into child (a, b) values (NULL, 'foo');
ERROR: null value in column "a" violates not-null constraint
--- 616,628 ----
insert into parent values (NULL);
insert into child (a, b) values (NULL, 'foo');
alter table only parent alter a set not null;
! ERROR: NOT NULL constraint must be added to child tables too
alter table child alter a set not null;
! ERROR: column "a" of relation "child" contains null values
delete from parent;
alter table only parent alter a set not null;
+ ERROR: NOT NULL constraint must be added to child tables too
insert into parent values (NULL);
alter table child alter a set not null;
insert into child (a, b) values (NULL, 'foo');
ERROR: null value in column "a" violates not-null constraint
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index 96bd816..6d0e3e1 100644
*** a/src/test/regress/expected/cluster.out
--- b/src/test/regress/expected/cluster.out
*************** ERROR: insert or update on table "clstr
*** 251,261 ****
DETAIL: Key (b)=(1111) is not present in table "clstr_tst_s".
SELECT conname FROM pg_constraint WHERE conrelid = 'clstr_tst'::regclass
ORDER BY 1;
! conname
! ----------------
clstr_tst_con
clstr_tst_pkey
! (2 rows)
SELECT relname, relkind,
EXISTS(SELECT 1 FROM pg_class WHERE oid = c.reltoastrelid) AS hastoast
--- 251,262 ----
DETAIL: Key (b)=(1111) is not present in table "clstr_tst_s".
SELECT conname FROM pg_constraint WHERE conrelid = 'clstr_tst'::regclass
ORDER BY 1;
! conname
! ----------------------
! clstr_tst_a_not_null
clstr_tst_con
clstr_tst_pkey
! (3 rows)
SELECT relname, relkind,
EXISTS(SELECT 1 FROM pg_class WHERE oid = c.reltoastrelid) AS hastoast
Excerpts from Bernd Helmle's message of jue oct 14 16:44:36 -0300 2010:
Yepp, that was it. I had a CFLAGS='-O0' in my dev build from a former
debugging cycle and forgot about it (which reminds me to do a
maintainer-clean more often between coding). This is also the reason i
haven't seen the compiler warnings and the crash in the regression tests.
Shame on me, but i think i have learned the lesson ;)
A better way to do this is create src/Makefile.custom and add this
line:
CFLAGS := $(filter-out -O2,$(CFLAGS)) -O0
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
--On 14. Oktober 2010 20:47:27 +0100 Dean Rasheed
<dean.a.rasheed@gmail.com> wrote:
OK, here it is.
I've not cured this compiler warning (in fact I'm not sure what it's
complaining about, because the variable *is* used):tablecmds.c: In function 'ATExecSetNotNull':
tablecmds.c:4747: warning: unused variable 'is_new_constraint'
Just a left over from earlier coding, i have removed this in my patch
version. I have adapted your fixes and would like to propose that we are
proceeding with my version, if that's okay for you?
--
Thanks
Bernd
On 15 October 2010 08:32, Bernd Helmle <mailings@oopsware.de> wrote:
--On 14. Oktober 2010 20:47:27 +0100 Dean Rasheed <dean.a.rasheed@gmail.com>
wrote:OK, here it is.
I've not cured this compiler warning (in fact I'm not sure what it's
complaining about, because the variable *is* used):tablecmds.c: In function 'ATExecSetNotNull':
tablecmds.c:4747: warning: unused variable 'is_new_constraint'Just a left over from earlier coding, i have removed this in my patch
version. I have adapted your fixes and would like to propose that we are
proceeding with my version, if that's okay for you?
Ah, I see it (I was looking at the wrong copy of that variable).
Yes, let's proceed with your version.
If these were the only problems with this patch, given that they
required only trivial changes to initialise variables to NIL/false,
perhaps it was premature to mark it as "returned with feedback".
Thoughts?
Regards,
Dean
On Fri, Oct 15, 2010 at 4:06 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
If these were the only problems with this patch, given that they
required only trivial changes to initialise variables to NIL/false,
perhaps it was premature to mark it as "returned with feedback".
Sure. Is someone available to do a detailed code review? Alvaro,
were you planning to pick this one up?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
The new patch applies cleanly to head, there are no compile errors and
all the make check tests pass (linux).
Thanks
Andrew
Show quoted text
On Thu, Oct 14, 2010 at 4:35 PM, Bernd Helmle <mailings@oopsware.de> wrote:
--On 14. Oktober 2010 16:28:51 -0300 Alvaro Herrera
<alvherre@commandprompt.com> wrote:Looking in that function, there is a similar "found" variable that
isn't being initialised (which my compiler didn't warn about).
Initialising that to false, sems to fix the problem and all the
regression tests then pass.Excellent. Please send an updated patch.
Here is an updated version of the patch. It fixes the following issues
Andrew discovered during his review cycle:* Fix compiler warnings and crash due to uninitialized variables (pretty
much the fix Dean proposed)* Remove accidentally added pg_latch.c in my own git repos.
I will do further cycles over Andrew's review report.
--
ThanksBernd
Robert Haas <robertmhaas@gmail.com> writes:
On Fri, Oct 15, 2010 at 4:06 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
If these were the only problems with this patch, given that they
required only trivial changes to initialise variables to NIL/false,
perhaps it was premature to mark it as "returned with feedback".
Sure. Is someone available to do a detailed code review? Alvaro,
were you planning to pick this one up?
I had been planning to take this one once it got past initial review,
since it was mostly my wish to start with.
regards, tom lane
Bernd Helmle <mailings@oopsware.de> writes:
Here is an updated version of the patch. It fixes the following issues
Andrew discovered during his review cycle:
I looked through this a bit. It's not ready to commit unfortunately.
The main gripe I've got is that you've paid very little attention to
updating comments that your code changes invalidate. That's not even
a little bit acceptable: people will still have to read this code later.
Two examples are that struct CookedConstraint is now used for notnull
constraints in addition to its other duties, but you didn't adjust the
comments in its declaration; and that you made transformColumnDefinition
put NOTNULL constraints into the ckconstraints list, ignoring the fact
that both its name and the comments say that that's only CHECK
constraints. In the latter case it'd probably be a better idea to add
a separate "nnconstraints" list to CreateStmtContext.
Some other random points:
* The ALTER TABLE changes seem to be inserting a whole lot of
CommandCounterIncrement calls in places where there were none before.
Do we really need those? Usually the theory is that one at the end
of an operation is sufficient.
* All those bits with deconstruct_array could usefully be folded into
a subroutine, along the lines of
bool constraint_is_for_single_column(HeapTuple constraintTup, int attno)
* As best I can tell from looking, the patch *always* generates a name
for NOT NULL constraints. It should honor the user's name for the
constraint if one is given, ie
create table foo (f1 int constraint nn1 not null);
Historically we've had to drop such names on the floor, but that should
stop.
* pg_dump almost certainly needs some updates. I imagine the behavior
we'll want from it is to print NOT NULL only when the column's notnull
constraint shows that it's locally defined. If it gets printed for
columns that only have an inherited NOT NULL, then dump and reload
will change the not-null inheritance state. This may be a bit tricky
since pg_dump also has to still work against older servers, but with
any luck you can steal its logic for inherited check constraints.
We probably want it to start preserving the names of notnull
constraints, too.
* In general you should probably search for all places that reference
pg_constraint.contype, as a means of spotting any other code that needs
to be updated for this.
* Lots of bogus trailing whitespace. "git diff --check" can help you
with that. Also, please try to avoid unnecessary changes of whitespace
on lines the patch isn't otherwise changing. That makes it harder for
reviewers to see what the patch *is* changing, and it's a useless
activity anyway because the next run of pg_indent will undo such
changes.
* No documentation updates. At the very least, catalogs.sgml has to
be updated: both the pg_attribute and pg_constraint pages probably
have to have something to say about this.
* No regression test updates. There ought to be a few test cases that
demonstrate the new behavior.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes:
* Lots of bogus trailing whitespace. "git diff --check" can help you
with that.
This is a repetitive common remark that I think sharing tips to avoid
that problem is a good idea. Here's an emacs setup to have trailing
whitespace hurt the eyes, and more-than-80 columns lines too. This one
is more controversial as we find lots of long lines in the PostgreSQL
sources. Still:
;; display only tails of lines longer than 80 columns, and
;; trailing whitespaces
(require 'whitespace)
(setq whitespace-line-column 80
whitespace-style '(face trailing lines-tail empty))
;; face for tabs long lines' tails
(set-face-attribute 'whitespace-tab nil
:background "red1"
:foreground "yellow"
:weight 'bold)
(set-face-attribute 'whitespace-line nil
:background "red1"
:foreground "yellow"
:weight 'bold)
;; activate minor whitespace mode when in some coding modes
(add-hook 'emacs-lisp-mode-hook 'whitespace-mode)
(add-hook 'python-mode-hook 'whitespace-mode)
(add-hook 'c-mode-hook 'whitespace-mode)
Now, it's easy to find some more about it, including images of how it
looks. You can't miss trailing whitespace any more :
http://ruslanspivak.com/2010/09/27/keep-track-of-whitespaces-and-column-80-overflow/
http://panela.blog-city.com/python_and_emacs_4_whitespace_tabs_tabwidth_visualizi.htm
http://www.emacswiki.org/emacs/WhiteSpace
I suppose people using other editors or tools will come up with other
tricks and tips. Maybe it should go in src/tools/editors/emacs.samples,
too?
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
--On 15. Oktober 2010 13:36:38 -0400 Tom Lane <tgl@sss.pgh.pa.us> wrote:
Bernd Helmle <mailings@oopsware.de> writes:
Here is an updated version of the patch. It fixes the following issues
Andrew discovered during his review cycle:I looked through this a bit. It's not ready to commit unfortunately.
Thanks for looking at this. I didn't reckon that i really got everything
done (admitted, docs and regression tests were liberally left out), and
given your comments your review is invaluable at this point.
The main gripe I've got is that you've paid very little attention to
updating comments that your code changes invalidate. That's not even
a little bit acceptable: people will still have to read this code later.
Two examples are that struct CookedConstraint is now used for notnull
constraints in addition to its other duties, but you didn't adjust the
comments in its declaration; and that you made transformColumnDefinition
put NOTNULL constraints into the ckconstraints list, ignoring the fact
that both its name and the comments say that that's only CHECK
constraints. In the latter case it'd probably be a better idea to add
a separate "nnconstraints" list to CreateStmtContext.
Agreed, there's much more cleanup needed.
Some other random points:
* The ALTER TABLE changes seem to be inserting a whole lot of
CommandCounterIncrement calls in places where there were none before.
Do we really need those? Usually the theory is that one at the end
of an operation is sufficient.
Hmm i seem to remember that i had some problems during coding and some
testing, where changes were unvisible during recursion....I will go through
them again.
* All those bits with deconstruct_array could usefully be folded into
a subroutine, along the lines of
bool constraint_is_for_single_column(HeapTuple constraintTup, int attno)
Ok
* As best I can tell from looking, the patch *always* generates a name
for NOT NULL constraints. It should honor the user's name for the
constraint if one is given, ie
create table foo (f1 int constraint nn1 not null);
Historically we've had to drop such names on the floor, but that should
stop.
Oh, i really missed that.
* pg_dump almost certainly needs some updates. I imagine the behavior
we'll want from it is to print NOT NULL only when the column's notnull
constraint shows that it's locally defined. If it gets printed for
columns that only have an inherited NOT NULL, then dump and reload
will change the not-null inheritance state. This may be a bit tricky
since pg_dump also has to still work against older servers, but with
any luck you can steal its logic for inherited check constraints.
We probably want it to start preserving the names of notnull
constraints, too.
I will look at it.
* In general you should probably search for all places that reference
pg_constraint.contype, as a means of spotting any other code that needs
to be updated for this.
Ok
* Lots of bogus trailing whitespace. "git diff --check" can help you
with that. Also, please try to avoid unnecessary changes of whitespace
on lines the patch isn't otherwise changing. That makes it harder for
reviewers to see what the patch *is* changing, and it's a useless
activity anyway because the next run of pg_indent will undo such
changes.
whoops...i've set (setq-default show-trailing-whitespace t) in my .emacs,
so i don't miss it again.
* No documentation updates. At the very least, catalogs.sgml has to
be updated: both the pg_attribute and pg_constraint pages probably
have to have something to say about this.* No regression test updates. There ought to be a few test cases that
demonstrate the new behavior.
I'll include them. It was important for me to get a feeling about wether
the direction in refactoring/extending this code is the correct one or
needs more thinking. So, thanks again for reviewing.
--
Thanks
Bernd
Import Notes
Resolved by subject fallback
On fre, 2010-10-15 at 22:45 +0200, Dimitri Fontaine wrote:
I suppose people using other editors or tools will come up with other
tricks and tips.
Here is an alternative recipe that I have been using:
(require 'show-wspace)
(add-hook 'font-lock-mode-hook 'show-ws-highlight-hard-spaces)
(add-hook 'font-lock-mode-hook 'show-ws-highlight-tabs)
(add-hook 'font-lock-mode-hook 'show-ws-highlight-trailing-whitespace)
Maybe it should go in src/tools/editors/emacs.samples, too?
Yeah, I think we should recommend some way to highlight faulty
whitespace.
The problem is, after you turn it on, it will make you cry as you
realize how sloppy most code and other files are written.
--On 16. Oktober 2010 12:35:06 +0300 Peter Eisentraut <peter_e@gmx.net>
wrote:
Maybe it should go in src/tools/editors/emacs.samples, too?
Yeah, I think we should recommend some way to highlight faulty
whitespace.The problem is, after you turn it on, it will make you cry as you
realize how sloppy most code and other files are written.
That's exactly why it is mostly off in my case. But you always can put it
in a special editing mode, which i currently experimenting with. Thanks for
your tips.
--
Thanks
Bernd
Bernd, are you still working on this?
Show quoted text
On fre, 2010-10-15 at 13:36 -0400, Tom Lane wrote:
Bernd Helmle <mailings@oopsware.de> writes:
Here is an updated version of the patch. It fixes the following issues
Andrew discovered during his review cycle:I looked through this a bit. It's not ready to commit unfortunately.
I intend to have a look at this patch and hopefully fix the outstanding
issues.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Thanks, I am looking at the new version from Bernd's git repo. One
problem I noticed is that it doesn't really work correctly for all
callers of heap_create_with_catalog -- you're only passing the cooked
not null constraints in DefineRelation, but there are some other places
that call heap_create_with_catalog too. In particular, bootstrap mode
is not handled; I haven't checked the other callers yet. I'm looking
for the best way to handle that.
So, question: do we need pg_constraint rows to exist for all NOT NULL
constraints, including those in system catalogs, and including those in
bootstrap catalogs? If we're going to require that, we're going to need
to add a few initial data lines to the pg_constraint catalog definition,
plus some code to handle the other bootstrap cases (non bootstrap
relations).
We could also declare that we don't need pg_constraint rows for NOT NULL
constraints in system catalogs; but if we're going to do that, I guess
we'd better disallow tables from inheriting system catalogs. Right now
it's not disallowed but I guess it's pretty useless
alvherre=# create table bar() inherits (pg_class);
CREATE TABLE
... on the other hand, being able to use a catalog in a LIKE column
definition sounds like it could be useful:
alvherre=# create table qux (now timestamp, like pg_class);
CREATE TABLE
alvherre=# \d qux
Tabla «public.qux»
Columna | Tipo | Modificadores
----------------+-----------------------------+---------------
now | timestamp without time zone |
relname | name | not null
relnamespace | oid | not null
reltype | oid | not null
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
So, question: do we need pg_constraint rows to exist for all NOT NULL
constraints, including those in system catalogs, and including those in
bootstrap catalogs? If we're going to require that, we're going to need
to add a few initial data lines to the pg_constraint catalog definition,
plus some code to handle the other bootstrap cases (non bootstrap
relations).
Installing such rows during bootstrap would be problematic, because what
do you do for catalogs that are created before pg_constraint?
Possible solution is to leave bootstrap's behavior alone, and have a
step during initdb's post-bootstrap stuff that creates a matching
pg_constraint row for every pg_attribute entry that's marked attnotnull.
We could also declare that we don't need pg_constraint rows for NOT NULL
constraints in system catalogs; but if we're going to do that, I guess
we'd better disallow tables from inheriting system catalogs.
I have a feeling that omitting these entries for system catalogs would
bite us in other ways down the road, even if inheritance were the only
soft spot right now.
regards, tom lane
On Thu, Jun 16, 2011 at 1:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Possible solution is to leave bootstrap's behavior alone, and have a
step during initdb's post-bootstrap stuff that creates a matching
pg_constraint row for every pg_attribute entry that's marked attnotnull.
That seems like a pretty good solution.
I have a feeling that omitting these entries for system catalogs would
bite us in other ways down the road, even if inheritance were the only
soft spot right now.
I share that feeling.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--On 16. Juni 2011 13:25:05 -0400 Tom Lane <tgl@sss.pgh.pa.us> wrote:
Possible solution is to leave bootstrap's behavior alone, and have a
step during initdb's post-bootstrap stuff that creates a matching
pg_constraint row for every pg_attribute entry that's marked attnotnull.
+1 for this idea. I never came to an end about this because i didn't have any
clue how to do it efficiently.
--
Thanks
Bernd
Excerpts from Bernd Helmle's message of jue jun 16 14:30:48 -0400 2011:
--On 16. Juni 2011 13:25:05 -0400 Tom Lane <tgl@sss.pgh.pa.us> wrote:
Possible solution is to leave bootstrap's behavior alone, and have a
step during initdb's post-bootstrap stuff that creates a matching
pg_constraint row for every pg_attribute entry that's marked attnotnull.+1 for this idea. I never came to an end about this because i didn't have any
clue how to do it efficiently.
Okay, I have done it this way -- adding one more fixup function to
initdb is very easy. I only wish that the ending \n in the query to
initdb would be optional -- it took me a while to realize that if you
omit it, the query doesn't get run at all. Oh well.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Another thing I just noticed is that if you pg_upgrade from a previous
release, all the NOT NULL pg_constraint rows are going to be missing.
I'm not sure what's the best way to deal with this -- should we just
provide a script for the user to run on each database?
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Fri, Jun 24, 2011 at 11:17 AM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
Another thing I just noticed is that if you pg_upgrade from a previous
release, all the NOT NULL pg_constraint rows are going to be missing.
Uh, really? pg_upgrade uses SQL commands to recreate the contents of
the system catalogs, so if these entries aren't getting created
automatically, that sounds like a bug in the patch...
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of vie jun 24 12:06:17 -0400 2011:
On Fri, Jun 24, 2011 at 11:17 AM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:Another thing I just noticed is that if you pg_upgrade from a previous
release, all the NOT NULL pg_constraint rows are going to be missing.Uh, really? pg_upgrade uses SQL commands to recreate the contents of
the system catalogs, so if these entries aren't getting created
automatically, that sounds like a bug in the patch...
Ah -- we should be fine then. I haven't tested that yet (actually I
haven't finished tinkering with the code).
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
--On 24. Juni 2011 12:06:17 -0400 Robert Haas <robertmhaas@gmail.com> wrote:
Uh, really? pg_upgrade uses SQL commands to recreate the contents of
the system catalogs, so if these entries aren't getting created
automatically, that sounds like a bug in the patch...
AFAIR, i've tested it and it worked as expected.
--
Thanks
Bernd
Excerpts from Bernd Helmle's message of vie jun 24 12:56:26 -0400 2011:
--On 24. Juni 2011 12:06:17 -0400 Robert Haas <robertmhaas@gmail.com> wrote:
Uh, really? pg_upgrade uses SQL commands to recreate the contents of
the system catalogs, so if these entries aren't getting created
automatically, that sounds like a bug in the patch...AFAIR, i've tested it and it worked as expected.
Okay, thanks for the confirmation.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
So remind me ... did we discuss PRIMARY KEY constraints? Are they
supposed to show up as inherited not null rows in the child? Obviously,
they do not show up as PKs in the child, but they *are* not null so my
guess is that they need to be inherited as not null as well. (Right
now, unpatched head of course emits the column as attnotnull).
In this case, the inherited name (assuming that the child declaration
does not explicitely state a constraint name) should be the same as the
PK, correct?
It is unclear to me that primary keys shouldn't be inherited by default.
But I guess that's a separate discussion.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Fri, Jun 24, 2011 at 6:39 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
So remind me ... did we discuss PRIMARY KEY constraints? Are they
supposed to show up as inherited not null rows in the child? Obviously,
they do not show up as PKs in the child, but they *are* not null so my
guess is that they need to be inherited as not null as well. (Right
now, unpatched head of course emits the column as attnotnull).In this case, the inherited name (assuming that the child declaration
does not explicitely state a constraint name) should be the same as the
PK, correct?
I would tend to think of the not-null-ness that is required by the
primary constraint as a separate constraint, not an intrinsic part of
the primary key. IOW, if you drop the primary key constraint, IMV,
that should never cause the column to begin allowing nulls.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of vie jun 24 19:01:49 -0400 2011:
On Fri, Jun 24, 2011 at 6:39 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:So remind me ... did we discuss PRIMARY KEY constraints? Are they
supposed to show up as inherited not null rows in the child? Obviously,
they do not show up as PKs in the child, but they *are* not null so my
guess is that they need to be inherited as not null as well. (Right
now, unpatched head of course emits the column as attnotnull).In this case, the inherited name (assuming that the child declaration
does not explicitely state a constraint name) should be the same as the
PK, correct?I would tend to think of the not-null-ness that is required by the
primary constraint as a separate constraint, not an intrinsic part of
the primary key. IOW, if you drop the primary key constraint, IMV,
that should never cause the column to begin allowing nulls.
Yeah, that is actually what happens. (I had never noticed this, but it seems
obvious in hindsight.)
alvherre=# create table foo (a int primary key);
NOTICE: CREATE TABLE / PRIMARY KEY creará el índice implícito «foo_pkey» para la tabla «foo»
CREATE TABLE
alvherre=# alter table foo drop constraint foo_pkey;
ALTER TABLE
alvherre=# \d foo
Tabla «public.foo»
Columna | Tipo | Modificadores
---------+---------+---------------
a | integer | not null
What this says is that this patch needs to be creating pg_constraint
entries for all PRIMARY KEY columns too, which answers my question above
quite nicely.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On 25 June 2011 01:59, Alvaro Herrera <alvherre@commandprompt.com> wrote:
Excerpts from Robert Haas's message of vie jun 24 19:01:49 -0400 2011:
I would tend to think of the not-null-ness that is required by the
primary constraint as a separate constraint, not an intrinsic part of
the primary key. IOW, if you drop the primary key constraint, IMV,
that should never cause the column to begin allowing nulls.
Really? I would expect the reverse, namely that the not-nullness is
part of the PK constraint and dropping the PK *would* then start
allowing NULLs.
I thought that this was one of the reasons for doing this work in the
first place. See http://wiki.postgresql.org/wiki/Todo#CREATE and the
bug reports linked from there.
Yeah, that is actually what happens. (I had never noticed this, but it seems
obvious in hindsight.)alvherre=# create table foo (a int primary key);
NOTICE: CREATE TABLE / PRIMARY KEY creará el índice implícito «foo_pkey» para la tabla «foo»
CREATE TABLE
alvherre=# alter table foo drop constraint foo_pkey;
ALTER TABLE
alvherre=# \d foo
Tabla «public.foo»
Columna | Tipo | Modificadores
---------+---------+---------------
a | integer | not null
Yeah, that's one of the bugs linked from the TODO item, that I hoped
this patch would fix.
[http://archives.postgresql.org/pgsql-hackers/2009-04/msg00062.php]
What this says is that this patch needs to be creating pg_constraint
entries for all PRIMARY KEY columns too, which answers my question above
quite nicely.
If by that you mean that you'd end up with 2 pg_constraint entries for
the PK, then that seems wrong to me. I think the not-nullness should
be part of the PK.
Regards,
Dean
On Sat, Jun 25, 2011 at 2:15 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
Really? I would expect the reverse, namely that the not-nullness is
part of the PK constraint and dropping the PK *would* then start
allowing NULLs.
Hmm, OK. I had assumed we were only trying to fix the problem that
parent and child inheritance tables could get out of step, but maybe
you're right.
If we go with that approach, then consider:
CREATE TABLE foo (a int);
CREATE TABLE bar () INHERITS (foo);
Now if someone adds a primary key foo (a), what happens currently is
that foo.a becomes NOT NULL, but bar.a still allows NULLs. Should
that remain true (on the theory that a primary key constraint is not
inherited) or become false (on the theory that parent and child tables
should match)?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 27 June 2011 03:31, Robert Haas <robertmhaas@gmail.com> wrote:
On Sat, Jun 25, 2011 at 2:15 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
Really? I would expect the reverse, namely that the not-nullness is
part of the PK constraint and dropping the PK *would* then start
allowing NULLs.Hmm, OK. I had assumed we were only trying to fix the problem that
parent and child inheritance tables could get out of step, but maybe
you're right.If we go with that approach, then consider:
CREATE TABLE foo (a int);
CREATE TABLE bar () INHERITS (foo);>
Now if someone adds a primary key foo (a), what happens currently is
that foo.a becomes NOT NULL, but bar.a still allows NULLs. Should
that remain true (on the theory that a primary key constraint is not
inherited) or become false (on the theory that parent and child tables
should match)?
I'm not sure, but my real problem with the current behaviour is its
inconsistency. Consider this case:
CREATE TABLE foo (a int PRIMARY KEY);
CREATE TABLE bar () INHERITS (foo);
Currently this results in bar not allowing NULLs, which is
inconsistent with adding the PK after defining the inheritance. Then
if the PK is dropped, the non-nullness is left behind on both foo and
bar.
I would summarise the consistency requirements as:
1). ADD CONSTRAINT should leave both parent and child tables in the
same state as they would have been if the constraint had been defined
at table creation time.
2). DROP CONSTRAINT should leave both parent and child tables in the
same state as if the constraint had never existed (completely
reversing the effects of ADD CONSTRAINT).
I don't have a strong opinion as to whether or not the NOT NULL part
of a PK should be inherited, provided that it is consistent with the
above.
I guess that if I were forced to choose, I would say that the NOT NULL
part of a PK should not be inherited, since I do think of it as part
of the PK, and PKs are not inherited. But I wouldn't be too upset if
it were inherited (consistently!) and I can't think of a use case
where that would be a problem.
Regards,
Dean
On Mon, Jun 27, 2011 at 3:08 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On 27 June 2011 03:31, Robert Haas <robertmhaas@gmail.com> wrote:
On Sat, Jun 25, 2011 at 2:15 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
Really? I would expect the reverse, namely that the not-nullness is
part of the PK constraint and dropping the PK *would* then start
allowing NULLs.Hmm, OK. I had assumed we were only trying to fix the problem that
parent and child inheritance tables could get out of step, but maybe
you're right.If we go with that approach, then consider:
CREATE TABLE foo (a int);
CREATE TABLE bar () INHERITS (foo);>
Now if someone adds a primary key foo (a), what happens currently is
that foo.a becomes NOT NULL, but bar.a still allows NULLs. Should
that remain true (on the theory that a primary key constraint is not
inherited) or become false (on the theory that parent and child tables
should match)?I'm not sure, but my real problem with the current behaviour is its
inconsistency. Consider this case:CREATE TABLE foo (a int PRIMARY KEY);
CREATE TABLE bar () INHERITS (foo);Currently this results in bar not allowing NULLs, which is
inconsistent with adding the PK after defining the inheritance. Then
if the PK is dropped, the non-nullness is left behind on both foo and
bar.I would summarise the consistency requirements as:
1). ADD CONSTRAINT should leave both parent and child tables in the
same state as they would have been if the constraint had been defined
at table creation time.2). DROP CONSTRAINT should leave both parent and child tables in the
same state as if the constraint had never existed (completely
reversing the effects of ADD CONSTRAINT).I don't have a strong opinion as to whether or not the NOT NULL part
of a PK should be inherited, provided that it is consistent with the
above.I guess that if I were forced to choose, I would say that the NOT NULL
part of a PK should not be inherited, since I do think of it as part
of the PK, and PKs are not inherited.
OK, I see your point, and I agree with you.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of lun jun 27 10:35:59 -0400 2011:
On Mon, Jun 27, 2011 at 3:08 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
I would summarise the consistency requirements as:
1). ADD CONSTRAINT should leave both parent and child tables in the
same state as they would have been if the constraint had been defined
at table creation time.2). DROP CONSTRAINT should leave both parent and child tables in the
same state as if the constraint had never existed (completely
reversing the effects of ADD CONSTRAINT).I don't have a strong opinion as to whether or not the NOT NULL part
of a PK should be inherited, provided that it is consistent with the
above.I guess that if I were forced to choose, I would say that the NOT NULL
part of a PK should not be inherited, since I do think of it as part
of the PK, and PKs are not inherited.OK, I see your point, and I agree with you.
Interesting. This whole thing requires quite a bit of rejiggering in
the initial transformation phase, I think, but yeah, I see the points
here and I will see to them. Does this mean that "NOT NULL PRIMARY KEY"
now behaves differently? I think it does , because if you drop the PK
then the field needs to continue being not null.
And here I was thinking that this was just a quick job to enable NOT
VALID constraints ...
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Wed, Jun 29, 2011 at 12:51 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
Excerpts from Robert Haas's message of lun jun 27 10:35:59 -0400 2011:
On Mon, Jun 27, 2011 at 3:08 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
I would summarise the consistency requirements as:
1). ADD CONSTRAINT should leave both parent and child tables in the
same state as they would have been if the constraint had been defined
at table creation time.2). DROP CONSTRAINT should leave both parent and child tables in the
same state as if the constraint had never existed (completely
reversing the effects of ADD CONSTRAINT).I don't have a strong opinion as to whether or not the NOT NULL part
of a PK should be inherited, provided that it is consistent with the
above.I guess that if I were forced to choose, I would say that the NOT NULL
part of a PK should not be inherited, since I do think of it as part
of the PK, and PKs are not inherited.OK, I see your point, and I agree with you.
Interesting. This whole thing requires quite a bit of rejiggering in
the initial transformation phase, I think, but yeah, I see the points
here and I will see to them. Does this mean that "NOT NULL PRIMARY KEY"
now behaves differently? I think it does , because if you drop the PK
then the field needs to continue being not null.
Yeah, I think an implicit not-null because you made it a primary key
is now different from one that you write out.
And here I was thinking that this was just a quick job to enable NOT
VALID constraints ...
Bwahaha.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of mié jun 29 13:07:25 -0400 2011:
On Wed, Jun 29, 2011 at 12:51 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:Excerpts from Robert Haas's message of lun jun 27 10:35:59 -0400 2011:
Interesting. This whole thing requires quite a bit of rejiggering in
the initial transformation phase, I think, but yeah, I see the points
here and I will see to them. Does this mean that "NOT NULL PRIMARY KEY"
now behaves differently? I think it does , because if you drop the PK
then the field needs to continue being not null.Yeah, I think an implicit not-null because you made it a primary key
is now different from one that you write out.
Actually, it wasn't that hard, but I'm not really sure I like the
resulting code:
/*
* We want to inherit NOT NULL constraints, but not primary keys.
* Since attnotnull flags in pg_attribute stores both, we want to keep only
* the attnotnull flag from those columns that have it from NOT NULL
* constraints. To do this, we create a copy of the table's descriptor
* and scribble on it by resetting all the attnotnull bits to false, and
* the setting them true for columns that appear in a NOT NULL constraint.
*
* Note: we cannot use CreateTupleDescCopy here, because it'd lose
* the atthasdef bits, as well as constraints.
*/
tupleDesc = CreateTupleDescCopyConstr(RelationGetDescr(relation));
constr = tupleDesc->constr;
parent_nns = GetRelationNotNullConstraints(relation);
for (parent_attno = 1; parent_attno <= tupleDesc->natts;
parent_attno++)
tupleDesc->attrs[parent_attno - 1]->attnotnull = false;
foreach (cell, parent_nns)
{
NotNullConstraint *constr = lfirst(cell);
tupleDesc->attrs[constr->attnum - 1]->attnotnull = true;
}
Here's the simple example (sorry for the spanish):
alvherre=# create table foo (a int primary key, b int not null);
NOTICE: CREATE TABLE / PRIMARY KEY creará el índice implícito «foo_pkey» para la tabla «foo»
CREATE TABLE
alvherre=# create table bar () inherits (foo);
CREATE TABLE
alvherre=# \d foo
Tabla «public.foo»
Columna | Tipo | Modificadores
---------+---------+---------------
a | integer | not null
b | integer | not null
Índices:
"foo_pkey" PRIMARY KEY, btree (a)
Número de tablas hijas: 1 (Use \d+ para listarlas.)
alvherre=# \d bar
Tabla «public.bar»
Columna | Tipo | Modificadores
---------+---------+---------------
a | integer |
b | integer | not null
Hereda: foo
alvherre=# create table baz (a int not null primary key);
NOTICE: CREATE TABLE / PRIMARY KEY creará el índice implícito «baz_pkey» para la tabla «baz»
CREATE TABLE
alvherre=# create table qux () inherits (baz);
CREATE TABLE
alvherre=# \d baz
Tabla «public.baz»
Columna | Tipo | Modificadores
---------+---------+---------------
a | integer | not null
Índices:
"baz_pkey" PRIMARY KEY, btree (a)
Número de tablas hijas: 1 (Use \d+ para listarlas.)
alvherre=# \d qux
Tabla «public.qux»
Columna | Tipo | Modificadores
---------+---------+---------------
a | integer | not null
Hereda: baz
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Wed, Jun 29, 2011 at 4:59 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
Excerpts from Robert Haas's message of mié jun 29 13:07:25 -0400 2011:
On Wed, Jun 29, 2011 at 12:51 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:Excerpts from Robert Haas's message of lun jun 27 10:35:59 -0400 2011:
Interesting. This whole thing requires quite a bit of rejiggering in
the initial transformation phase, I think, but yeah, I see the points
here and I will see to them. Does this mean that "NOT NULL PRIMARY KEY"
now behaves differently? I think it does , because if you drop the PK
then the field needs to continue being not null.Yeah, I think an implicit not-null because you made it a primary key
is now different from one that you write out.Actually, it wasn't that hard, but I'm not really sure I like the
resulting code:
What don't you like about it?
My concern is that I'm not sure it's correct...
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of mié jun 29 18:16:20 -0400 2011:
On Wed, Jun 29, 2011 at 4:59 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:Excerpts from Robert Haas's message of mié jun 29 13:07:25 -0400 2011:
On Wed, Jun 29, 2011 at 12:51 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:Excerpts from Robert Haas's message of lun jun 27 10:35:59 -0400 2011:
Interesting. This whole thing requires quite a bit of rejiggering in
the initial transformation phase, I think, but yeah, I see the points
here and I will see to them. Does this mean that "NOT NULL PRIMARY KEY"
now behaves differently? I think it does , because if you drop the PK
then the field needs to continue being not null.Yeah, I think an implicit not-null because you made it a primary key
is now different from one that you write out.Actually, it wasn't that hard, but I'm not really sure I like the
resulting code:What don't you like about it?
Scribbling on attnotnull like that seems ... kludgy (we have to walk the
attr list three times: first to copy, second to reset all the attnotnull
flags, third to set those of interest). The fact that we need a copy to
scribble on, seems wrong as well (we weren't creating a copy before).
The existing mechanisms to copy tupledescs aren't very flexible, but
improving that seems overengineering to me.
My concern is that I'm not sure it's correct...
Ah, well, I don't see any reason not to trust it currently. I am afraid
it could easily break in the future though.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support