Re: costly foreign key ri checks (4)

Dear Tom,

On Sat, 13 Mar 2004, Tom Lane wrote:
>
> I have reviewed and applied this, with some tweaking.  I attach the
> patch as applied.  Some comments on the changes:
>
> * You were careless about updating the comments to match the code.
>   This is an essential activity to keep things intelligible for
>   future developers.

Argh, that's possible.

> * The operator lookup needed to have the left and right operand types
>   switched; as it stood, the test for opclass membership failed in many
>   more cases than it was supposed to, because you were feeding it the
>   wrong member of a commutator pair.

Some subtlety I definitely missed. Note that the issue was there before I
came, I did not change the oper() call.

> * I changed the message wording to conform to the message style
>   guidelines.  I also made it complain about "costly sequential scans"
>   instead of "costly cross-type conversion", since ISTM that's what's
>   really at issue here.  I'm not completely wedded to that wording
>   though, if anyone feels the previous version was better.

I thought I copied-pasted the message as decided in the discussion.
I agree that the new version looks better.

> * BTW, you were generating the type names in the error message in the
>   wrong way --- format_type_be is preferred for this, as it is easier
>   to call and generates nicer output too.

Thanks. I was a little bit at lost with internal functions so as
to select the good one for a particular task.

"format_type_be" does not really mean anything to me. I wouldn't have
suspected that this would give the type name for prettyprinting.

> * It seemed to me that while we were at it, we should improve the
>   message for complete failure (no available equality operator)
>   to complain about the foreign key constraint rather than the
>   operator per se.  That is,

Sure.

> * The number of regression cases you added seemed excessive for
>   such a minor feature.  We do need to have some consideration for the
>   runtime of the regression tests, because they are used so often by
>   so many developers.  I pared it down a little, and made sure it
>   exercised both promotion and crosstype-index-operator cases.

I'll keep that in mind. However, from other projects I've worked on, I
found that large regression tests are not wasted.

Maybe two level of tests wouldn't be bad, as when you're about to release
a new version, it's better to pass large tests, but when installing some
light checks are enough just to check that all functionnalities are there.
I think I already saw something like that somewhere in the sources?

> Overall though, a good effort.  This was your first backend patch,
> wasn't it?  Nice job.

Thanks. Especially for you're very useful comments, help and pointers.

Actually it is the second. I passed a (major;-) patch to add an "ALSO"
keyword next to "INSTEAD" in the "CREATE RULE" syntax.

Anyway, I'll try to improve my standards next time.

--
Fabien Coelho - coelho@cri.ensmp.fr