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 (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers