Re: inconsistent comparison of CHECK constraints

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> While reviewing Nikhil Sontakke's fix for the inherited constraints open
> item we have, I noticed that MergeWithExistingConstraint and
> MergeConstraintsIntoExisting are using rather different mechanism to
> compare equality of the constraint expressions; the former does this:

> if (equal(expr, stringToNode(TextDatumGetCString(val))))

> So plain string comparison of the node's string representation.

No, that's *not* a "plain string comparison", and if it were it would be
wrong. This is doing equal() on the node trees, which is in fact the
correct implementation. (If we just compared the nodeToString strings
textually, then the result would depend on fields that equal() is
designed to ignore, such as source-line locations. And we definitely
want this comparison to ignore those.)

> MergeConstraintsIntoExisting is instead doing this:

> if (acon->condeferrable != bcon->condeferrable ||
> acon->condeferred != bcon->condeferred ||
> strcmp(decompile_conbin(a, tupleDesc),
> decompile_conbin(b, tupleDesc)) != 0)

That's kind of a crock, but it's necessary because it's trying to detect
equivalence of constraint expressions belonging to different tables,
which could have different physical column numbers as noted by the
comment. So I don't see a way to reduce it to a simple equal().
But for constraints applicable to just one table, equal() should be
preferred as it's simpler and more reliable.

regards, tom lane