[HACKERS] 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:
{
Datumval;
boolisnull;
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 alvhe...@alvh.no-ip.org
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inconsistent comparison of CHECK constraints
Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inconsistent comparison of CHECK constraints
Excerpts from Tom Lane's message of lun ene 16 12:44:57 -0300 2012: Alvaro Herrera alvhe...@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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
