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));
return TextDatumGetCString(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(at)alvh(dot)no-ip(dot)org>