Re: [PATCH] Support for Array ELEMENT Foreign Keys

On Wed, Oct 24, 2012 at 12:06:35PM -0400, Tom Lane wrote:
> Noah Misch <noah(at)leadboat(dot)com> writes:
> > For FKs, we currently document that "The referenced columns must be the
> > columns of a non-deferrable unique or primary key constraint in the referenced
> > table." Taking that literally, one might imagine that bare UNIQUE indexes do
> > not qualify. However, transformFkeyCheckAttrs() does accept them, including
> > indexes with non-default operator classes:
>
> Indeed, and considerable sweat was spilled to make that happen. I'm
> pretty unimpressed with any proposal that we should just blow that off
> for array keys. Now, I concede that cross-type FKs are a corner case to
> begin with, and we may well end up concluding that it's just too much
> work to handle it for arrays because of the lack of infrastructure for
> applying non-default comparison operators to arrays. But I don't want
> that to happen just because we failed to even think about it.

Sure, it's important to raise for discussion. The way I see it, it's the
existing functions and operators for arrays that blew off non-default element
operator classes. This patch is just dealing with those building blocks on
their own terms. I would hesitate to give up cross-type support, but support
for a non-default operator class on the PK side seems expendable. Given the
limitations that I mentioned for the corresponding feature of ordinary foreign
keys, I'm skeptical of its importance for ELEMENT foreign keys.

On further reflection, we could stop short of preemptively forbidding
non-default operator classes and just teach transformFkeyCheckAttrs() to
select an affected index only as a last resort. The
equality_ops_are_compatible() check in ATAddForeignKeyConstraint() may proceed
to reject the index. A text_pattern_ops UNIQUE index uses the same equality
operator as a UNIQUE constraint, and it would continue to be rightly accepted.

> However, I'm about to bounce this patch back for rework anyway, because
> I've just noticed that it has fatal performance issues. If you issue
> any UPDATE or DELETE against the PK table, you get a query like this
> (shorn of some uninteresting syntactic details) for checking to see
> if the RI constraint would be violated:
>
> SELECT 1 FROM ONLY fktable x WHERE $1 = ANY (fkcol) FOR SHARE OF x;
>
> It is impossible to implement this query except with a full-table
> seqscan on the FK table. You can put a GIN index on the array fkcol,
> but that won't help, because "something = ANY (indexedcol)" isn't an
> indexable condition. I don't think we can ship a feature that's
> unusable for anything except toy-sized tables, and that's what this is
> right now.
>
> One way we could consider making this GIN-indexable is to change it to
>
> SELECT 1 FROM ONLY fktable x WHERE ARRAY[$1] <@ fkcol FOR SHARE OF x;
>
> However, that puts us right back into the problem that we have no
> control over the specific comparison semantics that <@ uses.
>
> Or we could try to teach PG to make "something = ANY (indexedcol)"
> indexable. That would likely be a pretty substantial amount of work
> though. In particular, matching such a query to a GIN index would
> require knowing whether the "=" operator corresponds to the idea of
> equality embodied in the GIN opclass's key compare() method, and that
> isn't information that's available from the current opclass API.

Perhaps, then, excluding all cross-type ELEMENT FKs is the right start. With
that and the operator compatibility check already appearing in the patch, we
can prove that <@ has the right semantics. Doing better adds either large
subprojects or seemingly-ad-hoc limitations. It's ugly, but only in a manner
comparable to "ARRAY[0.0] < ARRAY[0]" finding no operator.

Thanks,
nm