Обсуждение: 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