Proposed patch for operator lookup caching
Since Simon seems intent on hacking something in there, here is a patch
that I think is actually sane for improving operator lookup speed.
This patch caches all lookups, exact or ambiguous (since even the exact
ones require multiple cache searches in common cases); and behaves sanely
in the presence of search_path, pg_operator, or pg_cast changes.
I see about a 45% speedup (2110 vs 1445 tps) on Guillame Smet's test case.
On straight pgbench --- which has no ambiguous operators, plus it's not
read-only --- it's hard to measure any consistent speedup, but I can say
that it's not slower. Some other test cases would be nice.
I went through the code that's being bypassed in some detail, to see what
dependencies were being skipped over. I think that as long as we assume
that no *existing* type changes its domain base type, typtype, array
status, type category, or preferred-type status, we don't need to flush
the cache on pg_type changes. This is a good thing since pg_type changes
frequently (eg, at temp table create or drop).
The only case that I believe to be unhandled is that the cache doesn't pay
attention to ALTER TABLE ... INHERIT / NO INHERIT events. This means it
is theoretically possible to return the wrong operator if an operator
takes a complex type as input and the calling situation involves another
complex type whose inheritance relationship to that one changes. That's
sufficiently far out of the normal case that I'm not very worried about it
(in fact, we probably have bugs in that area even without this patch,
since for instance cached plans don't respond to such changes either).
We could plug the hole by forcing a system-wide cache reset during ALTER
TABLE ... INHERIT / NO INHERIT, if anyone insists.
I'm not entirely happy about applying a patch like this so late in
the beta cycle, but I'd much rather do this than than any of the
less-than-half-baked ideas that have been floated in the discussion
so far.
regards, tom lane
Tom Lane wrote:
Since Simon seems intent on hacking something in there, here is a patch
that I think is actually sane for improving operator lookup speed.
This patch caches all lookups, exact or ambiguous (since even the exact
ones require multiple cache searches in common cases); and behaves sanely
in the presence of search_path, pg_operator, or pg_cast changes.I see about a 45% speedup (2110 vs 1445 tps) on Guillame Smet's test case.
On straight pgbench --- which has no ambiguous operators, plus it's not
read-only --- it's hard to measure any consistent speedup, but I can say
that it's not slower. Some other test cases would be nice.I went through the code that's being bypassed in some detail, to see what
dependencies were being skipped over. I think that as long as we assume
that no *existing* type changes its domain base type, typtype, array
status, type category, or preferred-type status, we don't need to flush
the cache on pg_type changes. This is a good thing since pg_type changes
frequently (eg, at temp table create or drop).The only case that I believe to be unhandled is that the cache doesn't pay
attention to ALTER TABLE ... INHERIT / NO INHERIT events. This means it
is theoretically possible to return the wrong operator if an operator
takes a complex type as input and the calling situation involves another
complex type whose inheritance relationship to that one changes. That's
sufficiently far out of the normal case that I'm not very worried about it
(in fact, we probably have bugs in that area even without this patch,
since for instance cached plans don't respond to such changes either).
We could plug the hole by forcing a system-wide cache reset during ALTER
TABLE ... INHERIT / NO INHERIT, if anyone insists.I'm not entirely happy about applying a patch like this so late in
the beta cycle, but I'd much rather do this than than any of the
less-than-half-baked ideas that have been floated in the discussion
so far.
Thanks for the patch. I can see it is clearly of significant size.
I also noted that you found that the case of:
SELECT col FROM tab WHERE text_col = 'ABC';
also took 37% of CPU in January, I think meaning we had this problem in
8.2.
On the one hand we have a pretty significant patch that we might apply.
It gives us a major speedup (+30%) for a common query type. I assume
8.3 was slightly slower than 8.2 only because we have a few more
pg_catalog entries in 8.3 than 8.2. (I am still baffled how a lookup
function could take so much CPU compared to what else is done for a
query.)
We are also talking about catlog changes for 8.3. Are we comfortable
doing catalog changes between the beta and RC? I am wondering if the
right plan is to have someone else review your patch, apply it, make the
catalog changes, and release another beta this weekend. Give the beta
one week of testing and go for RC. That gives us testing of the patch,
and testing of the catalog changes before going to RC1.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
We are also talking about catlog changes for 8.3. Are we comfortable
doing catalog changes between the beta and RC?
The catalog changes in question seem entirely safe ... certainly much
more so than this patch ...
I do see your point that another beta might be prudent, but on the other
hand I'm not sure it's really needed. The only difference between a
beta and an RC is that we try not to change the code anymore after RC.
regards, tom lane
Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
We are also talking about catlog changes for 8.3. Are we comfortable
doing catalog changes between the beta and RC?The catalog changes in question seem entirely safe ... certainly much
more so than this patch ...I do see your point that another beta might be prudent, but on the other
hand I'm not sure it's really needed. The only difference between a
beta and an RC is that we try not to change the code anymore after RC.
To me RC means we think this might be the release candidate and I would
like to get some testing in of this in beta before hitting that point.
And an additional beta might encourage more testing too.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote:
Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
We are also talking about catlog changes for 8.3. Are we comfortable
doing catalog changes between the beta and RC?The catalog changes in question seem entirely safe ... certainly much
more so than this patch ...I do see your point that another beta might be prudent, but on the other
hand I'm not sure it's really needed. The only difference between a
beta and an RC is that we try not to change the code anymore after RC.To me RC means we think this might be the release candidate and I would
like to get some testing in of this in beta before hitting that point.And an additional beta might encourage more testing too.
I agree with Bruce here. If you want to apply that operator lookup cache
patch, I would have another beta. (And I am not personally against it,
because I feel major performance fixes may sometimes slip in as "bug fixes".)
If you all decide against that patch, we might as well just go for RC1. The
catalog changes seem rather trivial, and just a required initdb is no
reason for calling it another beta, IMHO.
Great work on that patch, btw.!
Best Regards
Michael Paesold
On Nov 27, 2007 6:34 AM, Bruce Momjian <bruce@momjian.us> wrote:
And an additional beta might encourage more testing too.
I'm not that sure of this point. I'm really worried about the lack of
people testing 8.3 at the moment. We have really too little feedback.
Perhaps they didn't meet any problem but even that could be good to
know.
That said, if this patch is applied, another beta is the reasonable
way to go. Not sure it's worth it though.
--
Guillaume
-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160
Since Simon seems intent on hacking something in there, here is a patch
that I think is actually sane for improving operator lookup speed.
+1 on the patch (reviewed and tested), and +1 for rolling it into RC.
- --
Greg Sabino Mullane greg@turnstep.com
PGP Key: 0x14964AC8 200711270954
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----
iD8DBQFHTC+rvJuQZxSWSsgRA7RdAJ9nGwaRPeUXLeLjBGsPfLi64dTmOwCeK/40
W/7/8n2Q1YvLyNABFHnv7No=
=o5Vy
-----END PGP SIGNATURE-----
On Mon, 2007-11-26 at 21:13 -0500, Tom Lane wrote:
Since Simon seems intent on hacking something in there, here is a patch
that I think is actually sane for improving operator lookup speed.
This patch caches all lookups, exact or ambiguous (since even the exact
ones require multiple cache searches in common cases); and behaves sanely
in the presence of search_path, pg_operator, or pg_cast changes.I see about a 45% speedup (2110 vs 1445 tps) on Guillame Smet's test case.
On straight pgbench --- which has no ambiguous operators, plus it's not
read-only --- it's hard to measure any consistent speedup, but I can say
that it's not slower. Some other test cases would be nice.
I see 45% speedup also on my previously published tests.
No noticeable difference on the integer test, so looks good.
I went through the code that's being bypassed in some detail, to see what
dependencies were being skipped over. I think that as long as we assume
that no *existing* type changes its domain base type, typtype, array
status, type category, or preferred-type status, we don't need to flush
the cache on pg_type changes. This is a good thing since pg_type changes
frequently (eg, at temp table create or drop).The only case that I believe to be unhandled is that the cache doesn't pay
attention to ALTER TABLE ... INHERIT / NO INHERIT events. This means it
is theoretically possible to return the wrong operator if an operator
takes a complex type as input and the calling situation involves another
complex type whose inheritance relationship to that one changes. That's
sufficiently far out of the normal case that I'm not very worried about it
(in fact, we probably have bugs in that area even without this patch,
since for instance cached plans don't respond to such changes either).
We could plug the hole by forcing a system-wide cache reset during ALTER
TABLE ... INHERIT / NO INHERIT, if anyone insists.
No, thats enough.
I'm not entirely happy about applying a patch like this so late in
the beta cycle, but I'd much rather do this than than any of the
less-than-half-baked ideas that have been floated in the discussion
so far.
Well, as long as we fix this, I don't mind how we do it.
The reason for writing the other patch was your requirement for a
minimally invasive patch. If we're willing to lift that requirement then
I'm happy to go with your patch. Personally, I am.
--
Simon Riggs
2ndQuadrant http://www.2ndQuadrant.com