Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
Started by Noah Mischabout 14 years ago9 messages
Git master can already avoid rewriting the table for column type changes like
varchar(10) -> varchar(20). However, if the column in question is on either
side of a FK relationship, we always revalidate the foreign key. Concretely,
I wanted these no-rewrite type changes to also assume FK validity:
- Any typmod-only change
- text <-> varchar
- domain <-> base type
To achieve that, this patch skips the revalidation when two conditions hold:
(a) Old and new pg_constraint.conpfeqop match exactly. This is actually
stronger than needed; we could loosen things by way of operator families.
However, no core type would benefit, and my head exploded when I tried to
define the more-generous test correctly.
(b) The functions, if any, implementing a cast from the foreign type to the
primary opcintype are the same. For this purpose, we can consider a binary
coercion equivalent to an exact type match. When the opcintype is
polymorphic, require that the old and new foreign types match exactly. (Since
ri_triggers.c does use the executor, the stronger check for polymorphic types
is no mere future-proofing. However, no core type exercises its necessity.)
These follow from the rules used to decide when to rebuild an index. I
further justify them in source comments.
To implement this, I have ATPostAlterTypeParse() stash the content of the old
pg_constraint.conpfeqop in the Constraint node. ATAddForeignKeyConstraint()
notices that and evaluates the above rules. If they both pass, it omits the
validation step just as though skip_validation had been given.
Thanks,
nm
Attachments:
at-foreign-key-v1.patchtext/plain; charset=us-asciiDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3b52415..0cde503 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,
***************
*** 5696,5701 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5698,5705 ----
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
***************
*** 5812,5817 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5816,5828 ----
(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];
***************
*** 5826,5831 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5837,5843 ----
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]));
***************
*** 5868,5877 **** 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)))
{
--- 5880,5896 ----
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)))
{
***************
*** 5893,5899 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5912,5921 ----
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)))
***************
*** 5909,5914 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5931,6006 ----
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 all collations share the same notion
+ * of equality, we 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;
***************
*** 5952,5962 **** 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;
--- 6044,6056 ----
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;
***************
*** 6406,6411 **** transformFkeyCheckAttrs(Relation pkrel,
--- 6500,6534 ----
return indexoid;
}
+ /*
+ * findFkeyCast -
+ *
+ * Wrapper around find_coercion_pathway() for ATAddForeignKeyConstraint().
+ * Caller has equal regard for an exact match and binary coercibility.
+ */
+ 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)
***************
*** 7792,7797 **** ATPostAlterTypeParse(Oid oldId, char *cmd,
--- 7915,7921 ----
foreach(lcmd, stmt->cmds)
{
AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);
+ Constraint *con;
switch (cmd->subtype)
{
***************
*** 7805,7810 **** ATPostAlterTypeParse(Oid oldId, char *cmd,
--- 7929,7940 ----
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;
***************
*** 7843,7848 **** TryReuseIndex(Oid oldId, IndexStmt *stmt)
--- 7973,8021 ----
}
}
+ /*
+ * Subroutine for ATPostAlterTypeParse(). ATAddForeignKeyConstraint() will
+ * make the final concerning whether 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 c9d3e2e..3dd0400 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 efe2c96..76e4b50 100644
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
***************
*** 2194,2199 **** _equalConstraint(const Constraint *a, const Constraint *b)
--- 2194,2200 ----
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 cb94614..d210cf3 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 bee0e18..09c424b 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
***************
*** 1550,1555 **** typedef struct Constraint
--- 1550,1556 ----
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..4c4df15 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
+ SET client_min_messages = debug1;
+ CREATE TABLE norewrite1_parent(c name PRIMARY KEY);
+ 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);
+ 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;
+ CREATE TABLE norewrite2_parent(c bigint PRIMARY KEY);
+ NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "norewrite2_parent_pkey" for table "norewrite2_parent"
+ DEBUG: building index "norewrite2_parent_pkey" on table "norewrite2_parent"
+ CREATE TABLE norewrite2_child(c int REFERENCES norewrite2_parent);
+ 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..f46776e 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,1212 ----
from pg_class
where oid = 'test_storage'::regclass;
+ -- SET DATA TYPE without a rewrite
+ SET client_min_messages = debug1;
+ CREATE TABLE norewrite1_parent(c name PRIMARY KEY);
+ CREATE TABLE norewrite1_child(c text REFERENCES norewrite1_parent);
+ ALTER TABLE norewrite1_child ALTER c TYPE varchar; -- recheck FK
+
+ CREATE DOMAIN other_int AS int;
+ CREATE TABLE norewrite2_parent(c bigint PRIMARY KEY);
+ CREATE TABLE norewrite2_child(c int REFERENCES norewrite2_parent);
+ 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
--
Re: Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
I neglected to commit after revising the text of a few comments; use this
version instead. Thanks.
Attachments:
at-foreign-key-v2.patchtext/plain; charset=us-asciiDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3b52415..9eba8e8 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,
***************
*** 5696,5701 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5698,5705 ----
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
***************
*** 5812,5817 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5816,5828 ----
(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];
***************
*** 5826,5831 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5837,5843 ----
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]));
***************
*** 5868,5877 **** 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)))
{
--- 5880,5896 ----
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)))
{
***************
*** 5893,5899 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5912,5921 ----
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)))
***************
*** 5909,5914 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5931,6006 ----
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;
***************
*** 5952,5962 **** 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;
--- 6044,6056 ----
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;
***************
*** 6406,6411 **** transformFkeyCheckAttrs(Relation pkrel,
--- 6500,6534 ----
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)
***************
*** 7792,7797 **** ATPostAlterTypeParse(Oid oldId, char *cmd,
--- 7915,7921 ----
foreach(lcmd, stmt->cmds)
{
AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);
+ Constraint *con;
switch (cmd->subtype)
{
***************
*** 7805,7810 **** ATPostAlterTypeParse(Oid oldId, char *cmd,
--- 7929,7940 ----
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;
***************
*** 7843,7848 **** TryReuseIndex(Oid oldId, IndexStmt *stmt)
--- 7973,8021 ----
}
}
+ /*
+ * 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 c9d3e2e..3dd0400 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 efe2c96..76e4b50 100644
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
***************
*** 2194,2199 **** _equalConstraint(const Constraint *a, const Constraint *b)
--- 2194,2200 ----
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 cb94614..d210cf3 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 bee0e18..09c424b 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
***************
*** 1550,1555 **** typedef struct Constraint
--- 1550,1556 ----
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..4c4df15 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
+ SET client_min_messages = debug1;
+ CREATE TABLE norewrite1_parent(c name PRIMARY KEY);
+ 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);
+ 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;
+ CREATE TABLE norewrite2_parent(c bigint PRIMARY KEY);
+ NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "norewrite2_parent_pkey" for table "norewrite2_parent"
+ DEBUG: building index "norewrite2_parent_pkey" on table "norewrite2_parent"
+ CREATE TABLE norewrite2_child(c int REFERENCES norewrite2_parent);
+ 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..f46776e 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,1212 ----
from pg_class
where oid = 'test_storage'::regclass;
+ -- SET DATA TYPE without a rewrite
+ SET client_min_messages = debug1;
+ CREATE TABLE norewrite1_parent(c name PRIMARY KEY);
+ CREATE TABLE norewrite1_child(c text REFERENCES norewrite1_parent);
+ ALTER TABLE norewrite1_child ALTER c TYPE varchar; -- recheck FK
+
+ CREATE DOMAIN other_int AS int;
+ CREATE TABLE norewrite2_parent(c bigint PRIMARY KEY);
+ CREATE TABLE norewrite2_child(c int REFERENCES norewrite2_parent);
+ 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
--
Re: Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
On Wed, Jan 4, 2012 at 12:35 PM, Noah Misch <noah@leadboat.com> wrote:
I neglected to commit after revising the text of a few comments; use this
version instead. Thanks.--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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;Settings:
name | current_setting
-----------------+------------------------------------------------------------------------------------------------------------
version | PostgreSQL 9.2devel on x86_64-unknown-linux-gnu,
compiled by gcc (GCC) 4.1.2 20070115 (SUSE Linux), 64-bit
lc_collate | C
lc_ctype | C
max_connections | 100
max_stack_depth | 2MB
server_encoding | SQL_ASCII
shared_buffers | 32MB
TimeZone | US/Pacific
wal_buffers | 1MB
Cheers,
Jeff
Re: Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
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.
Attachments:
at-foreign-key-v3.patchtext/plain; charset=us-asciiDownload
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
--
Re: Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
On Sat, Jan 21, 2012 at 9:05 PM, Noah Misch <noah@leadboat.com> wrote:
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.
Thanks, I've verified that it now passes make check.
Looking at the code now, I don't think I'll be able to do a meaningful
review in a reasonable time. I'm not familiar with that part or the
code, and it is too complex for me to learn right now.
Cheers,
Jeff
Re: Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
Excerpts from Noah Misch's message of dom ene 22 02:05:31 -0300 2012:
Thanks. I've attached a new version fixing this problem.
Looks good to me. Can you please clarify this bit?
* Since we elsewhere require that all collations share the same
* notion of equality, don't compare collation here.
Since I'm not familiar with this code, it's difficult for me to figure
out where this "elsewhere" is, and what does "same notion of equality"
mean.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Re: Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
On Wed, Jan 25, 2012 at 11:17:27AM -0300, Alvaro Herrera wrote:
Excerpts from Noah Misch's message of dom ene 22 02:05:31 -0300 2012:
Thanks. I've attached a new version fixing this problem.
Looks good to me. Can you please clarify this bit?
* Since we elsewhere require that all collations share the same
* notion of equality, don't compare collation here.Since I'm not familiar with this code, it's difficult for me to figure
out where this "elsewhere" is, and what does "same notion of equality"
mean.
Good questions. See the comment in front of ri_GenerateQualCollation(), the
comment in process_ordered_aggregate_single() at nodeAgg.c:605, the larger
comment in add_sort_column(), and the two XXX comments in
relation_has_unique_index_for(). We should probably document that assumption
in xindex.sgml to keep type implementors apprised.
"Same notion of equality" just means that "a COLLATE x = b COLLATE y" has the
same value for every x, y.
In any event, the patch needed a rebase, so I've attached it rebased and with
that comment edited to reference ri_GenerateQualCollation(), that being the
most-relevant source for the assumption in question.
Thanks for reviewing,
nm
Attachments:
at-foreign-key-v4.patchtext/plain; charset=us-asciiDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index cb8ac67..ba20950 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,
***************
*** 5591,5596 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5593,5600 ----
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
***************
*** 5707,5712 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5711,5723 ----
(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];
***************
*** 5721,5726 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5732,5738 ----
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]));
***************
*** 5763,5772 **** 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)))
{
--- 5775,5791 ----
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)))
{
***************
*** 5788,5794 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5807,5816 ----
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)))
***************
*** 5804,5809 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5826,5902 ----
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 require that all collations share the same notion of
+ * equality (see comment at ri_GenerateQualCollation()), 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;
***************
*** 5847,5857 **** 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;
--- 5940,5952 ----
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;
***************
*** 6301,6306 **** transformFkeyCheckAttrs(Relation pkrel,
--- 6396,6430 ----
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)
***************
*** 7687,7692 **** ATPostAlterTypeParse(Oid oldId, char *cmd,
--- 7811,7817 ----
foreach(lcmd, stmt->cmds)
{
AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);
+ Constraint *con;
switch (cmd->subtype)
{
***************
*** 7700,7705 **** ATPostAlterTypeParse(Oid oldId, char *cmd,
--- 7825,7836 ----
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;
***************
*** 7738,7743 **** TryReuseIndex(Oid oldId, IndexStmt *stmt)
--- 7869,7917 ----
}
}
+ /*
+ * 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 cc3168d..7fec4db 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 2295195..d2a79eb 100644
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
***************
*** 2199,2204 **** _equalConstraint(const Constraint *a, const Constraint *b)
--- 2199,2205 ----
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 1d33ceb..ab55639 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
***************
*** 1552,1557 **** typedef struct Constraint
--- 1552,1558 ----
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 41bfa85..3387516 100644
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
***************
*** 1674,1679 **** ALTER TABLE norewrite_array ALTER c TYPE text[]; -- no work
--- 1674,1705 ----
ALTER TABLE norewrite_array ALTER c TYPE other_textarr; -- rebuild index
DEBUG: building index "norewrite_array_pkey" on table "norewrite_array"
RESET client_min_messages;
+ 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 494c150..465ebdd 100644
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
***************
*** 1205,1210 **** ALTER TABLE norewrite_array ALTER c TYPE text[]; -- no work
--- 1205,1228 ----
ALTER TABLE norewrite_array ALTER c TYPE other_textarr; -- rebuild index
RESET client_min_messages;
+ 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
--
Re: Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
On Wed, Jan 25, 2012 at 10:39:56PM -0500, Noah Misch wrote:
In any event, the patch needed a rebase, so I've attached it rebased and with
that comment edited to reference ri_GenerateQualCollation(), that being the
most-relevant source for the assumption in question.
Commit 9d35116611e6a1fc10f2298944fbf0e4e1a826be invalidated the test case hunks
again. We'll need to either remove the test cases, as Robert chose to do for
that other patch, or bolster them per
http://archives.postgresql.org/message-id/20120126115536.GD15670%40tornado.leadboat.com
Re: Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
Excerpts from Noah Misch's message of jue ene 26 12:00:49 -0300 2012:
On Wed, Jan 25, 2012 at 10:39:56PM -0500, Noah Misch wrote:
In any event, the patch needed a rebase, so I've attached it rebased and with
that comment edited to reference ri_GenerateQualCollation(), that being the
most-relevant source for the assumption in question.Commit 9d35116611e6a1fc10f2298944fbf0e4e1a826be invalidated the test case hunks
again. We'll need to either remove the test cases, as Robert chose to do for
that other patch, or bolster them per
http://archives.postgresql.org/message-id/20120126115536.GD15670%40tornado.leadboat.com
Committed, removing the tests.
I also chose to update catversion, even though I can't figure out how to
make a Constraint node be stored anywhere. Maybe it's not even
possible, but then I thought maybe I'm just lacking imagination.
Thanks!
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support