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(at)cri(dot)ensmp(dot)fr