Re: REVIEW: Optimize referential integrity checks (todo item)

On 17 June 2012 18:48, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Gurjeet Singh <singh.gurjeet@gmail.com> writes:
>> On Sat, Jun 16, 2012 at 9:50 AM, Dean Rasheed <dean.a.rasheed@gmail.com>wrote:
>> I find it interesting that 'actual time' for top level 'Update on fk_table'
>> is always higher in patched versions, and yet the 'Total runtime' is lower
>> for the patched versions. I would've expected 'Total runtime' to be
>> proportional to the increase in top-level row-source's 'actual time'.
>> Even the time consumed by Seq scans is higher in patched version, so I
>> think the patch's affect on performance needs to be evaluated.
>
> AFAICS, the only way that the given patch could possibly make anything
> slower is that if the old value of some tested attribute is NULL, the
> comparison routines used to fall out immediately; now, they will do an
> additional SPI_getbinval call to extract the new value before making
> any decision.  So that would account for some small increase in the
> ModifyTable runtime in cases where there are a lot of null keys in FK
> rows being updated, which accurately describes Dean's test case, if not
> so much the real world.  I don't have a big problem with it, since the
> point of the patch is to possibly save a great deal more work in exactly
> these cases.
>
> It strikes me though that we are still leaving some money on the table.
> The SQL spec says clearly that no RI action need be taken when a null
> PK key value is updated to non-null, and I think this is right because
> there cannot possibly be any FK rows that are considered to match the
> old value.  (Note that both the spec and our FK code treat the RI
> equality operators as strict, even if the underlying functions aren't
> really.)  So we ought to have asymmetric logic in there when making
> checks on PK rows, such that null->non-null is not considered an
> interesting change.  If done properly this would remove the above-
> described slowdown in the PK case.
>

Yeah, that makes sense.

> Conversely, if an FK value is changed from non-null to null, that is
> either always OK (if MATCH SIMPLE, or if MATCH FULL and all the FK
> columns went to null) or a certain failure (if MATCH FULL and we
> have a mix of nulls and non-nulls).  There's no need to queue a
> trigger event in the "always OK" cases, so I think we need some
> asymmetric logic in the FK case as well.
>

Makes sense too.
I think that the patch already covers the most common use case (in my
experience) but we may as well get as much out of it as we can while
we're here.

Are you planning to tackle this, or should I move the patch back to
"waiting on author" to give Vik Reykja a chance to update it?

Regards,
Dean