Fabien COELHO <coelho@cri.ensmp.fr> 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
