Thread
-
Proposed patch for operator lookup caching
Tom Lane <tgl@sss.pgh.pa.us> — 2007-11-27T02:13:46Z
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
-
Re: [PATCHES] Proposed patch for operator lookup caching
Bruce Momjian <bruce@momjian.us> — 2007-11-27T04:12:07Z
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. +
-
Re: [PATCHES] Proposed patch for operator lookup caching
Tom Lane <tgl@sss.pgh.pa.us> — 2007-11-27T04:19:58Z
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
-
Re: [PATCHES] Proposed patch for operator lookup caching
Bruce Momjian <bruce@momjian.us> — 2007-11-27T05:34:42Z
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. +
-
Re: [PATCHES] Proposed patch for operator lookup caching
Michael Paesold <mpaesold@gmx.at> — 2007-11-27T07:20:43Z
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
-
Re: [PATCHES] Proposed patch for operator lookup caching
Guillaume Smet <guillaume.smet@gmail.com> — 2007-11-27T08:08:56Z
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
-
Re: Proposed patch for operator lookup caching
Greg Sabino Mullane <greg@turnstep.com> — 2007-11-27T14:55:50Z
-----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-----
-
Re: Proposed patch for operator lookup caching
Simon Riggs <simon@2ndquadrant.com> — 2007-11-27T16:38:09Z
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