Re: [HACKERS] [PATCHES] Proposed patch for operator lookup caching
On Nov 27, 2007 6:34 AM, Bruce Momjian <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [HACKERS] [PATCHES] Proposed patch for operator lookup caching
Bruce Momjian wrote: Tom Lane wrote: Bruce Momjian <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] [PATCHES] Proposed patch for operator lookup caching
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [HACKERS] [PATCHES] Proposed patch for operator lookup caching
Bruce Momjian <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [HACKERS] [PATCHES] Proposed patch for operator lookup caching
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 <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
