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