Can't ri_KeysEqual() consider two nulls as equal?

A recent discussion led me to the idea that FK triggers are fired
unnecessarily during an UPDATE if the foreign-key column(s) contain
any NULLs, because ri_KeysEqual() treats two nulls as unequal,
and therefore we conclude the row has changed when it has not.
I claim that both ri_KeysEqual() and ri_OneKeyEqual() could consider
two nulls to be equal. Furthermore it seems like ri_AllKeysUnequal()
should do so too; the case can't arise at the moment because the sole
caller already knows that one of the key sets contains no nulls, but
if this weren't so, the optimization would be actively wrong if we
concluded that two nulls were unequal.

Comments?

Also, I am wondering to what extent the ri_KeysEqual() calls in
ri_triggers.c are redundant, given that commands/trigger.c now has
the smarts to not even queue the trigger when those cases apply.

regards, tom lane

On Tue, 2007-04-17 at 17:16 -0400, Tom Lane wrote:
> A recent discussion led me to the idea that FK triggers are fired
> unnecessarily during an UPDATE if the foreign-key column(s) contain
> any NULLs, because ri_KeysEqual() treats two nulls as unequal,
> and therefore we conclude the row has changed when it has not.

FK trigger *can be optimised away* is true. No need to have a discussion
about whether NULL == NULL, but the critical test is: if I overwrote it,
would you be able to tell? The answer is No, so away it goes.

> I claim that both ri_KeysEqual() and ri_OneKeyEqual() could consider
> two nulls to be equal. Furthermore it seems like ri_AllKeysUnequal()
> should do so too; the case can't arise at the moment because the sole
> caller already knows that one of the key sets contains no nulls, but
> if this weren't so, the optimization would be actively wrong if we
> concluded that two nulls were unequal.
>
> Comments?
>
> Also, I am wondering to what extent the ri_KeysEqual() calls in
> ri_triggers.c are redundant, given that commands/trigger.c now has
> the smarts to not even queue the trigger when those cases apply.

Would be good to document that behaviour in ri_triggers.c - I was
completely misled when I looked at the code a while back. Probably more
than one thing can go.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

On Tue, 17 Apr 2007, Tom Lane wrote:

> A recent discussion led me to the idea that FK triggers are fired
> unnecessarily during an UPDATE if the foreign-key column(s) contain
> any NULLs, because ri_KeysEqual() treats two nulls as unequal,
> and therefore we conclude the row has changed when it has not.
> I claim that both ri_KeysEqual() and ri_OneKeyEqual() could consider
> two nulls to be equal.

For ri_KeysEqual, I think so, since we actually aren't testing equality as
much as difference between the rows that might invalidate the constraint.
And, it does seem like with the code in trigger.c that the other checks in
the _upd functions in ri_triggers.c are redundant, but I'm vaguely afraid
I've forgotten something.

For ri_OneKeyEqual, I think like ri_AllKeysUnequal we know that the old
row doesn't have NULLs in the places it's currently called (although I
don't think this is commented). It seems like it should stay consistent
with ri_KeysEqual and that not putting the foo = NULL or foo = DEFAULT
seems better for the current calling cases besides.

> Furthermore it seems like ri_AllKeysUnequal() should do so too; the case
> can't arise at the moment because the sole caller already knows that one
> of the key sets contains no nulls, but if this weren't so, the
> optimization would be actively wrong if we concluded that two nulls were
> unequal.

Hmm, probably so, although at least this does appear to be commented at
the calling site to mention that it's depending on the fact that there are
no NULLs.

Simon Riggs wrote:
> On Tue, 2007-04-17 at 17:16 -0400, Tom Lane wrote:
>> A recent discussion led me to the idea that FK triggers are fired
>> unnecessarily during an UPDATE if the foreign-key column(s) contain
>> any NULLs, because ri_KeysEqual() treats two nulls as unequal,
>> and therefore we conclude the row has changed when it has not.
>
> FK trigger *can be optimised away* is true. No need to have a discussion
> about whether NULL == NULL, but the critical test is: if I overwrote it,
> would you be able to tell? The answer is No, so away it goes.

The test should perhaps be named "unchanged" rather than "equal".

--
Richard Huxton
Archonet Ltd

On Tue, 17 Apr 2007, Tom Lane wrote:

> A recent discussion led me to the idea that FK triggers are fired
> unnecessarily during an UPDATE if the foreign-key column(s) contain
> any NULLs, because ri_KeysEqual() treats two nulls as unequal,
> and therefore we conclude the row has changed when it has not.
> I claim that both ri_KeysEqual() and ri_OneKeyEqual() could consider
> two nulls to be equal. Furthermore it seems like ri_AllKeysUnequal()
> should do so too; the case can't arise at the moment because the sole
> caller already knows that one of the key sets contains no nulls, but
> if this weren't so, the optimization would be actively wrong if we
> concluded that two nulls were unequal.

Do you have any suggestions for alternate names? Keeping them using Equal
seems to be dangerous since people would likely expect it to act like
normal equality (with nulls being different).

On Wed, Apr 18, 2007 at 07:51:48AM -0700, Stephan Szabo wrote:
> On Tue, 17 Apr 2007, Tom Lane wrote:
>
> > A recent discussion led me to the idea that FK triggers are fired
> > unnecessarily during an UPDATE if the foreign-key column(s)
> > contain any NULLs, because ri_KeysEqual() treats two nulls as
> > unequal, and therefore we conclude the row has changed when it has
> > not. I claim that both ri_KeysEqual() and ri_OneKeyEqual() could
> > consider two nulls to be equal. Furthermore it seems like
> > ri_AllKeysUnequal() should do so too; the case can't arise at the
> > moment because the sole caller already knows that one of the key
> > sets contains no nulls, but if this weren't so, the optimization
> > would be actively wrong if we concluded that two nulls were
> > unequal.
>
> Do you have any suggestions for alternate names? Keeping them using
> Equal seems to be dangerous since people would likely expect it to
> act like normal equality (with nulls being different).

How about NotDistinct as in SQL's IS NOT DISTINCT FROM ?

Cheers,
D
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
phone: +1 415 235 3778 AIM: dfetter666
Skype: davidfetter

Remember to vote!
Consider donating to PostgreSQL: http://www.postgresql.org/about/donate

Stephan Szabo <sszabo(at)megazone(dot)bigpanda(dot)com> writes:
> On Tue, 17 Apr 2007, Tom Lane wrote:
>> I claim that both ri_KeysEqual() and ri_OneKeyEqual() could consider
>> two nulls to be equal.

> Do you have any suggestions for alternate names? Keeping them using Equal
> seems to be dangerous since people would likely expect it to act like
> normal equality (with nulls being different).

I think Richard's suggestion of KeysUnchanged would work fine.

regards, tom lane