On 22.03.24 01:35, Paul Jungwirth wrote:
> > 1. In ri_triggers.c ri_KeysEqual, you swap the order of arguments to
> ri_AttributesEqual():
> >
> > - if (!ri_AttributesEqual(riinfo->ff_eq_oprs[i],
> RIAttType(rel, attnums[i]),
> > - oldvalue, newvalue))
> > + if (!ri_AttributesEqual(eq_opr, RIAttType(rel, attnums[i]),
> > + newvalue, oldvalue))
> >
> > But the declared arguments of ri_AttributesEqual() are oldvalue and
> newvalue, so passing them
> > backwards is really confusing. And the change does matter in the tests.
> >
> > Can we organize this better?
>
> I renamed the params and actually the whole function. All it's doing is
> execute `oldvalue op newvalue`, casting if necessary. So I changed it to
> ri_CompareWithCast and added some documentation. In an earlier version
> of this patch I had a separate function for the PERIOD comparison, but
> it's just doing the same thing, so I think the best thing is to give the
> function a more accurate name and use it.
Ok, I see now, and the new explanation is better.
But after reading the comment in the function about collations, I think
there could be trouble. As long as we are only comparing for equality
(and we don't support nondeterministic global collations), then we can
use any collation to compare for equality. But if we are doing
contained-by, then the collation does matter, so we would need to get
the actual collation somehow. So as written, this might not always work
correctly.
I think it would be safer for now if we just kept using the equality
operation even for temporal foreign keys. If we did that, then in the
case that you update a key to a new value that is contained by the old
value, this function would say "not equal" and fire all the checks, even
though it wouldn't need to. This is kind of similar to the "false
negatives" that the comment already talks about.
What do you think?