Fabien COELHO <[EMAIL PROTECTED]> writes: > Following the discussion about previous versions of this patch, please > find attached a new patch candidate for warning about costly foreign key > referential integrity checks.
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. * 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. * 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. * 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. * 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, -- This next should fail, because inet=int does not exist ALTER TABLE FKTABLE ADD FOREIGN KEY(ftest1) references pktable; ! ERROR: operator does not exist: inet = integer ! HINT: No operator matches the given name and argument type(s). You may need to add explicit type casts. becomes -- This next should fail, because inet=int does not exist ALTER TABLE FKTABLE ADD FOREIGN KEY(ftest1) references pktable; ! ERROR: foreign key constraint "$1" cannot be implemented ! DETAIL: Key columns "ftest1" and "ptest1" are of incompatible types: inet and integer. Again, I'm not wedded to this wording, but it seems like a step forward to me. The old way's HINT was certainly pretty inappropriate for the context of a foreign key. * 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. Overall though, a good effort. This was your first backend patch, wasn't it? Nice job. regards, tom lane
---------------------------(end of broadcast)--------------------------- TIP 8: explain analyze is your friend