On Sat, Jan 21, 2012 at 08:04:20PM -0800, Jeff Janes wrote:
> This is failing "make check" for me.
>
>
> *** /tmp/foo/src/test/regress/expected/alter_table.out Sat Jan 21 19:51:46
> 2012
> --- /tmp/foo/src/test/regress/results/alter_table.out Sat Jan 21 19:54:18
> 2012
> ***************
> *** 1662,1667 ****
> --- 1662,1668 ----
> NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
> "norewrite1_parent_pkey" for table "norewrite1_parent"
> DEBUG: building index "norewrite1_parent_pkey" on table "norewrite1_parent"
> CREATE TABLE norewrite1_child(c text REFERENCES norewrite1_parent);
> + DEBUG: building index "pg_toast_41491_index" on table "pg_toast_41491"
> ALTER TABLE norewrite1_child ALTER c TYPE varchar; -- recheck FK
> DEBUG: validating foreign key constraint "norewrite1_child_c_fkey"
> CREATE DOMAIN other_int AS int;
Thanks. I've attached a new version fixing this problem.
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index cc210f0..bb2cf62 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 276,281 **** static Oid transformFkeyCheckAttrs(Relation pkrel,
--- 276,282 ----
int numattrs, int16 *attnums,
Oid *opclasses);
static void checkFkeyPermissions(Relation rel, int16 *attnums, int natts);
+ static CoercionPathType findFkeyCast(Oid targetTypeId, Oid sourceTypeId, Oid
*funcid);
static void validateCheckConstraint(Relation rel, HeapTuple constrtup);
static void validateForeignKeyConstraint(char *conname,
Relation rel, Relation
pkrel,
***************
*** 357,362 **** static void ATPostAlterTypeCleanup(List **wqueue,
AlteredTableInfo *tab, LOCKMOD
--- 358,364 ----
static void ATPostAlterTypeParse(Oid oldId, char *cmd,
List **wqueue, LOCKMODE lockmode, bool
rewrite);
static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
+ static void TryReuseForeignKey(Oid oldId, Constraint *con);
static void change_owner_fix_column_acls(Oid relationOid,
Oid oldOwnerId, Oid
newOwnerId);
static void change_owner_recurse_to_sequences(Oid relationOid,
***************
*** 5574,5579 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation
rel,
--- 5576,5583 ----
numpks;
Oid indexOid;
Oid constrOid;
+ bool old_check_ok;
+ ListCell *old_pfeqop_item = list_head(fkconstraint->old_conpfeqop);
/*
* Grab an exclusive lock on the pk table, so that someone doesn't
delete
***************
*** 5690,5695 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation
rel,
--- 5694,5706 ----
(errcode(ERRCODE_INVALID_FOREIGN_KEY),
errmsg("number of referencing and referenced
columns for foreign key disagree")));
+ /*
+ * On the strength of a previous constraint, we might avoid scanning
+ * tables to validate this one. See below.
+ */
+ old_check_ok = (fkconstraint->old_conpfeqop != NIL);
+ Assert(!old_check_ok || numfks ==
list_length(fkconstraint->old_conpfeqop));
+
for (i = 0; i < numpks; i++)
{
Oid pktype = pktypoid[i];
***************
*** 5704,5709 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation
rel,
--- 5715,5721 ----
Oid ppeqop;
Oid ffeqop;
int16 eqstrategy;
+ Oid pfeqop_right;
/* We need several fields out of the pg_opclass entry */
cla_ht = SearchSysCache1(CLAOID,
ObjectIdGetDatum(opclasses[i]));
***************
*** 5746,5755 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation
rel,
pfeqop = get_opfamily_member(opfamily, opcintype, fktyped,
eqstrategy);
if (OidIsValid(pfeqop))
ffeqop = get_opfamily_member(opfamily, fktyped, fktyped,
eqstrategy);
else
! ffeqop = InvalidOid; /* keep compiler quiet */
if (!(OidIsValid(pfeqop) && OidIsValid(ffeqop)))
{
--- 5758,5774 ----
pfeqop = get_opfamily_member(opfamily, opcintype, fktyped,
eqstrategy);
if (OidIsValid(pfeqop))
+ {
+ pfeqop_right = fktyped;
ffeqop = get_opfamily_member(opfamily, fktyped, fktyped,
eqstrategy);
+ }
else
! {
! /* keep compiler quiet */
! pfeqop_right = InvalidOid;
! ffeqop = InvalidOid;
! }
if (!(OidIsValid(pfeqop) && OidIsValid(ffeqop)))
{
***************
*** 5771,5777 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation
rel,
--- 5790,5799 ----
target_typeids[1] = opcintype;
if (can_coerce_type(2, input_typeids, target_typeids,
COERCION_IMPLICIT))
+ {
pfeqop = ffeqop = ppeqop;
+ pfeqop_right = opcintype;
+ }
}
if (!(OidIsValid(pfeqop) && OidIsValid(ffeqop)))
***************
*** 5787,5792 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation
rel,
--- 5809,5884 ----
format_type_be(fktype),
format_type_be(pktype))));
+ if (old_check_ok)
+ {
+ /*
+ * When a pfeqop changes, revalidate the constraint.
We could
+ * permit intra-opfamily changes, but that adds subtle
complexity
+ * without any concrete benefit for core types. We
need not
+ * assess ppeqop or ffeqop, which RI_Initial_Check()
does not use.
+ */
+ old_check_ok = (pfeqop == lfirst_oid(old_pfeqop_item));
+ old_pfeqop_item = lnext(old_pfeqop_item);
+ }
+ if (old_check_ok)
+ {
+ Oid old_fktype;
+ Oid new_fktype;
+ CoercionPathType old_pathtype;
+ CoercionPathType new_pathtype;
+ Oid old_castfunc;
+ Oid new_castfunc;
+
+ /*
+ * Identify coercion pathways from each of the old and
new FK-side
+ * column types to the right (foreign) operand type of
the pfeqop.
+ * We may assume that pg_constraint.conkey is not
changing.
+ */
+ old_fktype = tab->oldDesc->attrs[fkattnum[i] -
1]->atttypid;
+ new_fktype = fktype;
+ old_pathtype = findFkeyCast(pfeqop_right, old_fktype,
+
&old_castfunc);
+ new_pathtype = findFkeyCast(pfeqop_right, new_fktype,
+
&new_castfunc);
+
+ /*
+ * Upon a change to the cast from the FK column to its
pfeqop
+ * operand, revalidate the constraint. For this
evaluation, a
+ * binary coercion cast is equivalent to no cast at
all. While
+ * type implementors should design implicit casts with
an eye
+ * toward consistency of operations like equality, we
cannot
+ * assume here that they have done so.
+ *
+ * A function with a polymorphic argument could change
behavior
+ * arbitrarily in response to get_fn_expr_argtype().
Therefore,
+ * when the cast destination is polymorphic, we only
avoid
+ * revalidation if the input type has not changed at
all. Given
+ * just the core data types and operator classes, this
requirement
+ * prevents no would-be optimizations.
+ *
+ * If the cast converts from a base type to a domain
thereon, then
+ * that domain type must be the opcintype of the unique
index.
+ * Necessarily, the primary key column must then be of
the domain
+ * type. Since the constraint was previously valid,
all values on
+ * the foreign side necessarily exist on the primary
side and in
+ * turn conform to the domain. Consequently, we need
not treat
+ * domains specially here.
+ *
+ * Since we elsewhere require that all collations share
the same
+ * notion of equality, don't compare collation here.
+ *
+ * We need not directly consider the PK type. It's
necessarily
+ * binary coercible to the opcintype of the unique
index column,
+ * and ri_triggers.c will only deal with PK datums in
terms of
+ * that opcintype. Changing the opcintype also changes
pfeqop.
+ */
+ old_check_ok = (new_pathtype == old_pathtype &&
+ new_castfunc ==
old_castfunc &&
+
(!IsPolymorphicType(pfeqop_right) ||
+ new_fktype ==
old_fktype));
+
+ }
+
pfeqoperators[i] = pfeqop;
ppeqoperators[i] = ppeqop;
ffeqoperators[i] = ffeqop;
***************
*** 5830,5840 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation
rel,
createForeignKeyTriggers(rel, fkconstraint, constrOid, indexOid);
/*
! * Tell Phase 3 to check that the constraint is satisfied by existing
rows.
! * We can skip this during table creation, or if requested explicitly by
! * specifying NOT VALID in an ADD FOREIGN KEY command.
*/
! if (!fkconstraint->skip_validation)
{
NewConstraint *newcon;
--- 5922,5934 ----
createForeignKeyTriggers(rel, fkconstraint, constrOid, indexOid);
/*
! * Tell Phase 3 to check that the constraint is satisfied by existing
! * rows. We can skip this during table creation, when requested
! * explicitly by specifying NOT VALID in an ADD FOREIGN KEY command, and
! * when we're recreating a constraint following a SET DATA TYPE
operation
! * that did not impugn its validity.
*/
! if (!old_check_ok && !fkconstraint->skip_validation)
{
NewConstraint *newcon;
***************
*** 6284,6289 **** transformFkeyCheckAttrs(Relation pkrel,
--- 6378,6412 ----
return indexoid;
}
+ /*
+ * findFkeyCast -
+ *
+ * Wrapper around find_coercion_pathway() for ATAddForeignKeyConstraint().
+ * Caller has equal regard for binary coercibility and for an exact match.
+ */
+ static CoercionPathType
+ findFkeyCast(Oid targetTypeId, Oid sourceTypeId, Oid *funcid)
+ {
+ CoercionPathType ret;
+
+ if (targetTypeId == sourceTypeId)
+ {
+ ret = COERCION_PATH_RELABELTYPE;
+ *funcid = InvalidOid;
+ }
+ else
+ {
+ ret = find_coercion_pathway(targetTypeId, sourceTypeId,
+
COERCION_IMPLICIT, funcid);
+ if (ret == COERCION_PATH_NONE)
+ /* A previously-relied-upon cast is now gone. */
+ elog(ERROR, "could not find cast from %u to %u",
+ sourceTypeId, targetTypeId);
+ }
+
+ return ret;
+ }
+
/* Permissions checks for ADD FOREIGN KEY */
static void
checkFkeyPermissions(Relation rel, int16 *attnums, int natts)
***************
*** 7670,7675 **** ATPostAlterTypeParse(Oid oldId, char *cmd,
--- 7793,7799 ----
foreach(lcmd, stmt->cmds)
{
AlterTableCmd *cmd =
(AlterTableCmd *) lfirst(lcmd);
+ Constraint *con;
switch (cmd->subtype)
{
***************
*** 7683,7688 **** ATPostAlterTypeParse(Oid oldId, char *cmd,
--- 7807,7818 ----
lappend(tab->subcmds[AT_PASS_OLD_INDEX], cmd);
break;
case AT_AddConstraint:
+
Assert(IsA(cmd->def, Constraint));
+ con =
(Constraint *) cmd->def;
+ /* rewriting
neither side of a FK */
+ if
(con->contype == CONSTR_FOREIGN &&
+
!rewrite && !tab->rewrite)
+
TryReuseForeignKey(oldId, con);
tab->subcmds[AT_PASS_OLD_CONSTR] =
lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
break;
***************
*** 7721,7726 **** TryReuseIndex(Oid oldId, IndexStmt *stmt)
--- 7851,7899 ----
}
}
+ /*
+ * Subroutine for ATPostAlterTypeParse(). ATAddForeignKeyConstraint() will
+ * make the final ruling on whether to skip revalidating this constraint's
+ * successor. Here, we just stash the old conpfeqop for its use.
+ */
+ static void
+ TryReuseForeignKey(Oid oldId, Constraint *con)
+ {
+ HeapTuple tup;
+ Datum adatum;
+ bool isNull;
+ ArrayType *arr;
+ Oid *rawarr;
+ int numkeys;
+ int i;
+
+ Assert(con->contype == CONSTR_FOREIGN);
+ Assert(con->old_conpfeqop == NIL); /* already prepared this node */
+
+ tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(oldId));
+ if (!HeapTupleIsValid(tup)) /* should not happen */
+ elog(ERROR, "cache lookup failed for constraint %u", oldId);
+
+ adatum = SysCacheGetAttr(CONSTROID, tup,
+
Anum_pg_constraint_conpfeqop, &isNull);
+ if (isNull)
+ elog(ERROR, "null conpfeqop for constraint %u", oldId);
+ arr = DatumGetArrayTypeP(adatum); /* ensure not toasted */
+ numkeys = ARR_DIMS(arr)[0];
+ /* test follows the one in ri_FetchConstraintInfo() */
+ if (ARR_NDIM(arr) != 1 ||
+ ARR_HASNULL(arr) ||
+ ARR_ELEMTYPE(arr) != OIDOID)
+ elog(ERROR, "conpfeqop is not a 1-D Oid array");
+ rawarr = (Oid *) ARR_DATA_PTR(arr);
+
+ /* stash a List of the operator Oids in our Constraint node */
+ for (i = 0; i < numkeys; i++)
+ con->old_conpfeqop = lcons_oid(rawarr[i], con->old_conpfeqop);
+
+ ReleaseSysCache(tup);
+ }
+
/*
* ALTER TABLE OWNER
diff --git a/src/backend/nodes/copyfuncindex 71da0d8..e89f4f5 100644
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 2364,2369 **** _copyConstraint(const Constraint *from)
--- 2364,2370 ----
COPY_SCALAR_FIELD(fk_matchtype);
COPY_SCALAR_FIELD(fk_upd_action);
COPY_SCALAR_FIELD(fk_del_action);
+ COPY_NODE_FIELD(old_conpfeqop);
COPY_SCALAR_FIELD(skip_validation);
COPY_SCALAR_FIELD(initially_valid);
diff --git a/src/backend/nodes/equalindex ba949db..e3522e8 100644
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
***************
*** 2195,2200 **** _equalConstraint(const Constraint *a, const Constraint *b)
--- 2195,2201 ----
COMPARE_SCALAR_FIELD(fk_matchtype);
COMPARE_SCALAR_FIELD(fk_upd_action);
COMPARE_SCALAR_FIELD(fk_del_action);
+ COMPARE_NODE_FIELD(old_conpfeqop);
COMPARE_SCALAR_FIELD(skip_validation);
COMPARE_SCALAR_FIELD(initially_valid);
diff --git a/src/backend/nodes/outfunindex 8bc1947..8971846 100644
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
***************
*** 2638,2643 **** _outConstraint(StringInfo str, const Constraint *node)
--- 2638,2644 ----
WRITE_CHAR_FIELD(fk_matchtype);
WRITE_CHAR_FIELD(fk_upd_action);
WRITE_CHAR_FIELD(fk_del_action);
+ WRITE_NODE_FIELD(old_conpfeqop);
WRITE_BOOL_FIELD(skip_validation);
WRITE_BOOL_FIELD(initially_valid);
break;
diff --git a/src/include/nodes/parsindex dce0e72..109a3d4 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
***************
*** 1551,1556 **** typedef struct Constraint
--- 1551,1557 ----
char fk_matchtype; /* FULL, PARTIAL, UNSPECIFIED */
char fk_upd_action; /* ON UPDATE action */
char fk_del_action; /* ON DELETE action */
+ List *old_conpfeqop; /* pg_constraint.conpfeqop of my former
self */
/* Fields used for constraints that allow a NOT VALID specification */
bool skip_validation; /* skip validation of existing
rows? */
diff --git a/src/test/regress/expecteindex 57096f2..0edcb41 100644
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
***************
*** 1656,1661 **** where oid = 'test_storage'::regclass;
--- 1656,1688 ----
t
(1 row)
+ -- SET DATA TYPE without a rewrite
+ CREATE TABLE norewrite1_parent(c name PRIMARY KEY);
+ NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
"norewrite1_parent_pkey" for table "norewrite1_parent"
+ CREATE TABLE norewrite1_child(c text REFERENCES norewrite1_parent);
+ SET client_min_messages = debug1;
+ ALTER TABLE norewrite1_child ALTER c TYPE varchar; -- recheck FK
+ DEBUG: validating foreign key constraint "norewrite1_child_c_fkey"
+ RESET client_min_messages;
+ CREATE DOMAIN other_int AS int;
+ CREATE TABLE norewrite2_parent(c bigint PRIMARY KEY);
+ NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
"norewrite2_parent_pkey" for table "norewrite2_parent"
+ CREATE TABLE norewrite2_child(c int REFERENCES norewrite2_parent);
+ SET client_min_messages = debug1;
+ ALTER TABLE norewrite2_child ALTER c TYPE other_int; -- no work
+ ALTER TABLE norewrite2_parent ALTER c TYPE int; -- full work
+ DEBUG: rewriting table "norewrite2_parent"
+ DEBUG: building index "norewrite2_parent_pkey" on table "norewrite2_parent"
+ DEBUG: validating foreign key constraint "norewrite2_child_c_fkey"
+ ALTER TABLE norewrite2_parent ALTER c TYPE oid; -- rebuild index, recheck FK
+ DEBUG: building index "norewrite2_parent_pkey" on table "norewrite2_parent"
+ DEBUG: validating foreign key constraint "norewrite2_child_c_fkey"
+ ALTER TABLE norewrite2_child ALTER c TYPE oid; -- no work
+ ALTER TABLE norewrite2_parent INHERIT norewrite2_child;
+ ALTER TABLE norewrite2_child ALTER c TYPE int; -- rebuild index, recheck FK
+ DEBUG: building index "norewrite2_parent_pkey" on table "norewrite2_parent"
+ DEBUG: validating foreign key constraint "norewrite2_child_c_fkey"
+ RESET client_min_messages;
--
-- lock levels
--
diff --git a/src/test/regress/sql/alter_table.sqindex faafb22..24068db 100644
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
***************
*** 1190,1195 **** select reltoastrelid <> 0 as has_toast_table
--- 1190,1214 ----
from pg_class
where oid = 'test_storage'::regclass;
+ -- SET DATA TYPE without a rewrite
+ CREATE TABLE norewrite1_parent(c name PRIMARY KEY);
+ CREATE TABLE norewrite1_child(c text REFERENCES norewrite1_parent);
+ SET client_min_messages = debug1;
+ ALTER TABLE norewrite1_child ALTER c TYPE varchar; -- recheck FK
+ RESET client_min_messages;
+
+ CREATE DOMAIN other_int AS int;
+ CREATE TABLE norewrite2_parent(c bigint PRIMARY KEY);
+ CREATE TABLE norewrite2_child(c int REFERENCES norewrite2_parent);
+ SET client_min_messages = debug1;
+ ALTER TABLE norewrite2_child ALTER c TYPE other_int; -- no work
+ ALTER TABLE norewrite2_parent ALTER c TYPE int; -- full work
+ ALTER TABLE norewrite2_parent ALTER c TYPE oid; -- rebuild index, recheck FK
+ ALTER TABLE norewrite2_child ALTER c TYPE oid; -- no work
+ ALTER TABLE norewrite2_parent INHERIT norewrite2_child;
+ ALTER TABLE norewrite2_child ALTER c TYPE int; -- rebuild index, recheck FK
+ RESET client_min_messages;
+
--
-- lock levels
--
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
