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
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
--
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
