Re: costly foreign key ri checks (4)

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