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,
> * 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 - [EMAIL PROTECTED]
---------------------------(end of broadcast)---------------------------
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])