Обсуждение: inconsistent comparison of CHECK constraints

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:
   {       Datum        val;       bool        isnull;
       val = fastgetattr(tup,                         Anum_pg_constraint_conbin,
conDesc->rd_att,&isnull);       if (isnull)           elog(ERROR, "null conbin for rel %s",
RelationGetRelationName(rel));      if (equal(expr, stringToNode(TextDatumGetCString(val))))           found = true;
}

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

MergeConstraintsIntoExisting is instead doing this:
   if (acon->condeferrable != bcon->condeferrable ||       acon->condeferred != bcon->condeferred ||
strcmp(decompile_conbin(a,tupleDesc),              decompile_conbin(b, tupleDesc)) != 0) 

where decompile_conbin is defined roughly as
   expr = DirectFunctionCall2(pg_get_expr, attr,                              ObjectIdGetDatum(con->conrelid));
returnTextDatumGetCString(expr); 

So it is first decompiling the node into its source representation, then
comparing that.


Do we care about this?  If so, which version is preferrable?

I also noticed that MergeConstraintsIntoExisting is doing a scan on
conrelid and then manually filtering for conname, which seems worse than
the other code that's just using conname/connamespace as scankey.  This
is probably better on tables with tons of constraints.

--
Álvaro Herrera <alvherre@alvh.no-ip.org>

Alvaro Herrera <alvherre@alvh.no-ip.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

Excerpts from Tom Lane's message of lun ene 16 12:44:57 -0300 2012:
> Alvaro Herrera <alvherre@alvh.no-ip.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.

Aha, that makes sense.

> > 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.

It makes plenty of sense too.

I've left the two separate implementations alone.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support