Re: costly foreign key ri checks (4) - Mailing list pgsql-patches
From | Fabien COELHO |
---|---|
Subject | Re: costly foreign key ri checks (4) |
Date | |
Msg-id | Pine.LNX.4.58.0403150946240.1913@sablons.cri.ensmp.fr Whole thread Raw |
In response to | Re: costly foreign key ri checks (4) (Tom Lane <tgl@sss.pgh.pa.us>) |
Responses |
Re: costly foreign key ri checks (4)
|
List | pgsql-patches |
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
pgsql-patches by date: