Обсуждение: costly foreign key ri checks (4)
Dear patchers, 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. 1/ it generates a WARNING 2/ it DETAILs the attributes and types 3/ some regression tests are also appended to foreign_key.sql It validates for me against current "head". Have a nice day, -- Fabien Coelho - coelho@cri.ensmp.fr
Вложения
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Fabien COELHO wrote:
>
> Dear patchers,
>
> 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.
>
> 1/ it generates a WARNING
>
> 2/ it DETAILs the attributes and types
>
> 3/ some regression tests are also appended to foreign_key.sql
>
> It validates for me against current "head".
>
> Have a nice day,
>
> --
> Fabien Coelho - coelho@cri.ensmp.fr
Content-Description:
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: the planner will ignore your desire to choose an index scan if your
> joining column's datatypes do not match
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
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
Вложения
Applied by Tom. Thanks. --------------------------------------------------------------------------- Fabien COELHO wrote: > > Dear patchers, > > 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. > > 1/ it generates a WARNING > > 2/ it DETAILs the attributes and types > > 3/ some regression tests are also appended to foreign_key.sql > > It validates for me against current "head". > > Have a nice day, > > -- > Fabien Coelho - coelho@cri.ensmp.fr Content-Description: [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 9: the planner will ignore your desire to choose an index scan if your > joining column's datatypes do not match -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Tom Lane wrote: > > * 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. So the issue wasn't that the conversion was costly, but that an index couldn't be used to look up the primary key? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> * 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.
> So the issue wasn't that the conversion was costly, but that an index
> couldn't be used to look up the primary key?
Exactly. That's why the patch is looking for operator membership in the
index's opclass.
There are actually two cases where the RI check can use the index even
though the datatypes aren't identical:
1. There's no cross-type equality operator, but there is an implicit
coercion from the referencing column's type to the referenced column's
type. In this case the selected equality operator will actually take
the referenced column's type on both sides, and will naturally be a
member of the index opclass.
2. There is a cross-type operator and it's a member of the referenced
column's index opclass.
Case 2 is new in 7.5 but case 1 has always existed. Note that case 1
does include a type conversion, but it can still use the index and will
be plenty fast enough.
The cases where we lose and have to use a seqscan, even though a
relevant equality operator was found, are:
3. Implicit coercion occurred on the primary key side instead of
the referencing column side. In this case the RI query becomes
"WHERE pk::something = fk", and the PK index doesn't apply.
4. A cross-type operator was selected and it's not in the PK opclass.
In case 4, we are slow even though there isn't necessarily any
cross-type conversion happening at all (depending on what goes on inside
the cross-type operator).
So it seems to me that blaming the problem on conversion per se is a
bit wide of the mark.
regards, tom lane
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
Fabien COELHO <coelho@cri.ensmp.fr> writes:
> 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?
There is a provision for "big tests" in the regression test
infrastructure. The only additional test at present is a rerun of the
numeric test with greater precision choices; which personally I think is
quite useless. But we could imagine using the mechanism for other stuff.
regards, tom lane