public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 3/6] Optimize allocations in bringetbitmap 4+ messages / 3 participants [nested] [flat]
* [PATCH 3/6] Optimize allocations in bringetbitmap @ 2021-03-02 18:57 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Tomas Vondra @ 2021-03-02 18:57 UTC (permalink / raw) The bringetbitmap function allocates memory for various purposes, which may be quite expensive, depending on the number of scan keys. Instead of allocating them separately, allocate one bit chunk of memory an carve it into smaller pieces as needed - all the pieces have the same lifespan, and it saves quite a bit of CPU and memory overhead. Author: Tomas Vondra <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/backend/access/brin/brin.c | 60 ++++++++++++++++++++++++++-------- 1 file changed, 47 insertions(+), 13 deletions(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 9f2656b8d9..1f82e965f9 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -373,6 +373,9 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) int *nkeys, *nnullkeys; int keyno; + char *ptr; + Size len; + char *tmp PG_USED_FOR_ASSERTS_ONLY; opaque = (BrinOpaque *) scan->opaque; bdesc = opaque->bo_bdesc; @@ -402,15 +405,52 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) * We keep null and regular keys separate, so that we can pass just the * regular keys to the consistent function easily. * + * To reduce the allocation overhead, we allocate one big chunk and then + * carve it into smaller arrays ourselves. All the pieces have exactly the + * same lifetime, so that's OK. + * * XXX The widest table can have ~1600 attributes, so this may allocate a * couple kilobytes of memory). We could invent a more compact approach * (with just space for used attributes) but that would make the matching * more complicated, so it may not be a win. */ - keys = palloc0(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts); - nullkeys = palloc0(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts); - nkeys = palloc0(sizeof(int) * bdesc->bd_tupdesc->natts); - nnullkeys = palloc0(sizeof(int) * bdesc->bd_tupdesc->natts); + len = + MAXALIGN(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts) + /* regular keys */ + MAXALIGN(sizeof(ScanKey) * scan->numberOfKeys) * bdesc->bd_tupdesc->natts + + MAXALIGN(sizeof(int) * bdesc->bd_tupdesc->natts) + + MAXALIGN(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts) + /* NULL keys */ + MAXALIGN(sizeof(ScanKey) * scan->numberOfKeys) * bdesc->bd_tupdesc->natts + + MAXALIGN(sizeof(int) * bdesc->bd_tupdesc->natts); + + ptr = palloc(len); + tmp = ptr; + + keys = (ScanKey **) ptr; + ptr += MAXALIGN(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts); + + nullkeys = (ScanKey **) ptr; + ptr += MAXALIGN(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts); + + nkeys = (int *) ptr; + ptr += MAXALIGN(sizeof(int) * bdesc->bd_tupdesc->natts); + + nnullkeys = (int *) ptr; + ptr += MAXALIGN(sizeof(int) * bdesc->bd_tupdesc->natts); + + for (int i = 0; i < bdesc->bd_tupdesc->natts; i++) + { + keys[i] = (ScanKey *) ptr; + ptr += MAXALIGN(sizeof(ScanKey) * scan->numberOfKeys); + + nullkeys[i] = (ScanKey *) ptr; + ptr += MAXALIGN(sizeof(ScanKey) * scan->numberOfKeys); + } + + Assert(tmp + len == ptr); + + /* zero the number of keys */ + memset(nkeys, 0, sizeof(int) * bdesc->bd_tupdesc->natts); + memset(nnullkeys, 0, sizeof(int) * bdesc->bd_tupdesc->natts); /* * Preprocess the scan keys - split them into per-attribute arrays. @@ -444,9 +484,9 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) { FmgrInfo *tmp; - /* No key/null arrays for this attribute. */ - Assert((keys[keyattno - 1] == NULL) && (nkeys[keyattno - 1] == 0)); - Assert((nullkeys[keyattno - 1] == NULL) && (nnullkeys[keyattno - 1] == 0)); + /* First time we see this attribute, so no key/null keys. */ + Assert(nkeys[keyattno - 1] == 0); + Assert(nnullkeys[keyattno - 1] == 0); tmp = index_getprocinfo(idxRel, keyattno, BRIN_PROCNUM_CONSISTENT); @@ -457,17 +497,11 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) /* Add key to the proper per-attribute array. */ if (key->sk_flags & SK_ISNULL) { - if (!nullkeys[keyattno - 1]) - nullkeys[keyattno - 1] = palloc0(sizeof(ScanKey) * scan->numberOfKeys); - nullkeys[keyattno - 1][nnullkeys[keyattno - 1]] = key; nnullkeys[keyattno - 1]++; } else { - if (!keys[keyattno - 1]) - keys[keyattno - 1] = palloc0(sizeof(ScanKey) * scan->numberOfKeys); - keys[keyattno - 1][nkeys[keyattno - 1]] = key; nkeys[keyattno - 1]++; } -- 2.26.2 --------------6A37408A2C42B5B56C3B3D49 Content-Type: text/x-patch; charset=UTF-8; name="0004-BRIN-bloom-indexes-20210303.patch" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="0004-BRIN-bloom-indexes-20210303.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: allowing extensions to control planner behavior @ 2024-08-26 17:37 Tom Lane <[email protected]> 2024-08-26 19:28 ` Re: allowing extensions to control planner behavior Robert Haas <[email protected]> 2024-08-27 20:07 ` Re: allowing extensions to control planner behavior Robert Haas <[email protected]> 0 siblings, 2 replies; 4+ messages in thread From: Tom Lane @ 2024-08-26 17:37 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: pgsql-hackers Robert Haas <[email protected]> writes: > I'm somewhat expecting to be flamed to a well-done crisp for saying > this, but I think we need better ways for extensions to control the > behavior of PostgreSQL's query planner. Nah, I won't flame you for that, it's a reasonable thing to think about. However, the devil is in the details, and ... > The attached patch, briefly mentioned above, essentially converts the > enable_* GUCs into RelOptInfo properties where the defaults are set by > the corresponding GUCs. ... this doesn't seem like it's moving the football very far at all. The enable_XXX GUCs are certainly blunt instruments, but I'm not sure how much better it is if they're per-rel. For example, I don't see how this gets us any closer to letting an extension fix a poor choice of join order. Or, if your problem is that the planner wants to scan index A but you want it to scan index B, enable_indexscan won't help. > ... On the other hand, the more I look at > what our enable_* GUCs actually do, the less impressed I am. IMHO, > things like enable_hashjoin make a lot of sense, but enable_sort seems > like it just controls an absolutely random smattering of behaviors in > a way that seems to me to have very little to recommend it, and I've > complained elsewhere about how enable_indexscan and > enable_indexonlyscan are really quite odd when you look at how they're > implemented. Yeah, these sorts of questions aren't made better this way either. If anything, having extensions manipulating these variables will make it even harder to rethink what they do. You mentioned that there is prior art out there, but this proposal doesn't seem like it's drawing on any such thing. What ideas should we be stealing? regards, tom lane ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: allowing extensions to control planner behavior 2024-08-26 17:37 Re: allowing extensions to control planner behavior Tom Lane <[email protected]> @ 2024-08-26 19:28 ` Robert Haas <[email protected]> 1 sibling, 0 replies; 4+ messages in thread From: Robert Haas @ 2024-08-26 19:28 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: pgsql-hackers On Mon, Aug 26, 2024 at 1:37 PM Tom Lane <[email protected]> wrote: > Robert Haas <[email protected]> writes: > > I'm somewhat expecting to be flamed to a well-done crisp for saying > > this, but I think we need better ways for extensions to control the > > behavior of PostgreSQL's query planner. > > Nah, I won't flame you for that, it's a reasonable thing to think > about. However, the devil is in the details, and ... Thank you. Not being flamed is one of my favorite things. :-) > > The attached patch, briefly mentioned above, essentially converts the > > enable_* GUCs into RelOptInfo properties where the defaults are set by > > the corresponding GUCs. > > ... this doesn't seem like it's moving the football very far at all. > The enable_XXX GUCs are certainly blunt instruments, but I'm not sure > how much better it is if they're per-rel. For example, I don't see > how this gets us any closer to letting an extension fix a poor choice > of join order. Or, if your problem is that the planner wants to scan > index A but you want it to scan index B, enable_indexscan won't help. Well, I agree that this doesn't address everything you might want to do, and I thought I said so, admittedly in the middle of a long wall of text. This would JUST be a step toward letting an extension control the scan and join methods, not the join order or the choice of index or whatever else there is. But the fact that it doesn't do everything is not a strike against it unless there's some competing design that lets you take care of everything with a single mechanism, which I do not see as realistic. If this proposal -- or really any proposal in this area -- gets through, I will very happily propose more things to address the other problems that I know about, but it doesn't make sense to do a huge amount of work to craft a comprehensive solution before we've had any discussion here. > Yeah, these sorts of questions aren't made better this way either. > If anything, having extensions manipulating these variables will > make it even harder to rethink what they do. Correct, but my proposal to make enable_indexscan behave like enable_indexonlyscan, which I thought was a slam-dunk, just provoked a lot of grumbling. There's a kind of chicken and egg problem here. If the existing GUCs were better designed, then using them here would make sense. And the patch that I attached to my previous email were in master, then cleaning up the design of the GUCs would have more value. But if I can't make any progress with either problem because the other problem also exists, then I'm kind of boxed into a corner. I could also propose something here that is diverges from the enable_* behavior, but then people will complain that the two shouldn't be inconsistent, which I agree with, BTW. I thought maybe doing this first would make sense, and then we could refine afterwards. > You mentioned that there is prior art out there, but this proposal > doesn't seem like it's drawing on any such thing. What ideas should > we be stealing? Depends what you mean. As far as PostgreSQL-related things, the two things that I mentioned in my opening paragraph and for which I provided links seem to be me to the best examples we have. It's pretty easy to see how to make pg_hint_plan require less kludgery, and I think we can just iterate through the various problems there and solve them pretty easily by adding a few hooks here and there and a few extension-settable structure members here and there. I am engaging in some serious hand-waving here, but this is not rocket science. I am confident that if you made it your top priority to get into PG 18 stuff which would thoroughly un-hackify pg_hint_plan, you could be done in months, possibly weeks. It will take me longer, but if we have an agreement in principal that it is worth doing, I just can't see it as being particularly difficult. Amazon's query plan management stuff is a much tougher lift. For that, you're asking the planner to try to create a new plan which is like some old plan that you got before. So in a perfect world, you want to control every planner decision. That's hard just because there are a lot of them. If for example you want to get the same index scan that you got before, you need not only to get the same type of index scan (index, index-only, bitmap) and the same index, but also things like the same non-native saop treatment, which seems like it would be asking an awful lot of a hook system. On the other hand, maybe you can cheat. If your regurgitate-the-same-plan system could force the same join order, join methods, scan methods, choice of indexes, and probably some stuff about aggregate and appendrel strategy, it might be close enough to giving you the same plan you had before that nobody would really care if the non-native saop treatment was different. I'm almost positive it's better than not having a feature, which is where are today. And although allowing control over just the major decisions in query planning doesn't seem like something we can do in one patch, I don't think it takes 100 patches either. Maybe five or ten. If we step outside of the PostgreSQL ecosystem, I think we should look at Oracle as one example. I have never been a real believer in hints like SeqScan(foo), because if you don't fix the cardinality estimate for table foo, then the rest of your plan is going to suck, too. On the other hand, "hint everything" for some people in some situations is a way to address that. It's stupid in a sense, but if you have an automated way to do it, especially one that allows applying hints out-of-query, it's not THAT stupid. Also, Oracle offers some other pretty useful hints. In particular, using the LEADING hint to set the driving table for the query plan does not seem dumb to me at all. Hinting that things should be parallel or not, and with what degree of parallelism, also seem quite reasonable. They've also got ALL_ROWS and FIRST_ROWS(n) hints, which let you say whether you want fast-start behavior or not, and it hardly needs to be said how often we get that wrong or how badly. pg_hint_plan, which copies a lot of stuff that Oracle does, innovates by allowing you to hint that a certain join will return X number of rows or that the number or rows that the planner thinks should be returned should be corrected by multiplying, adding, or subtracting some constant. I'm not sure how useful this is really because I feel like a lot of times you'd just pick some join order where that particular join is no longer used e.g. if. A JOIN B JOIN C and I hint the AB join, perhaps the planner will just start by joining C to either A or B, and then that join will never occur. However, that can be avoided by also using LEADING, or maybe in some other cleverer way, like making an AB selectivity hint apply at whatever point in the plan you join something that includes A to something that includes B. There's some details on SQL server's hinting here: https://learn.microsoft.com/en-us/sql/t-sql/queries/hints-transact-sql?view=sql-server-ver16 It looks pretty complicated, but some of the basic concepts that you'd expect are also present here: force the join method, rule in or out, force the use of an index or of no index, force the join order. Those seem to be the major things that "everyone" supports. I think we'd want to expand a bit on that to allow forcing aggregate strategy and perhaps some PostgreSQL-specific things e.g. other systems won't have a hint to force a TIDRangeScan or not because that's a PostgreSQL-specific concept, but it would be silly to make a system that lets an extension control sequential scans and index scans but not other, more rarely-used ways of scanning a relation, so probably we want to do something. I don't know if that helps, in terms of context. If it doesn't, let me know what would help. And just to be clear, I *absolutely* think we need to take a light touch here. If we install a ton of new highly-opinionated infrastructure we will make a lot of people mad and that's definitely not where I want to end up. I just think we need to grow beyond "the planner is a black box and you shall not presume to direct it." If every other system provides a way to control, say, the join order, then it seems reasonable to suppose that a PostgreSQL extension should be able to control the join order too. A lot of details might be different but if multiple other systems have the concept then the concept itself probably isn't ridiculous. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: allowing extensions to control planner behavior 2024-08-26 17:37 Re: allowing extensions to control planner behavior Tom Lane <[email protected]> @ 2024-08-27 20:07 ` Robert Haas <[email protected]> 1 sibling, 0 replies; 4+ messages in thread From: Robert Haas @ 2024-08-27 20:07 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: pgsql-hackers On Mon, Aug 26, 2024 at 1:37 PM Tom Lane <[email protected]> wrote: > For example, I don't see > how this gets us any closer to letting an extension fix a poor choice > of join order. Thinking more about this particular sub-problem, let's say we're joining four tables A, B, C, and D. An extension wants to compel join order A-B-C-D. Let's suppose, however, that it wants to do this in a way where planning won't fail if that's impossible, so it wants to use disabled_nodes rather than skipping path generation entirely. When we're planning the baserels, we don't need to do anything special. When we plan 2-way joins, we need to mark all paths disabled except those originating from the A-B join. When we plan 3-way joins, we need to mark all paths disabled except those originating from an (A-B)-C join. When we plan the final 4-way join, we don't really need to do anything extra: the only way to end up with a non-disabled path at the top level is to pick a path from the (A-B)-C join and a path from D. There's a bit of nuance here, though. Suppose that when planning the A-B join, the planner generates HashJoin(SeqScan(B),Hash(A)). Does that path need to be disabled? If you think that join order A-B-C-D means that table A should be the driving table, then the answer is yes, because that path will lead to a join order beginning with B-A, not one beginning with A-B. But you might also have a mental model where it doesn't matter which side of the table is on which side of the join, and as long as you start by joining A and B in some way, that's good enough to qualify as an A-B join order. I believe actual implementations vary in which approach they take. I think that the beginning of add_paths_to_joinrel() looks like a useful spot to get control. You could, for example, have a hook there which returns a bitmask indicating which of merge-join, nested-loop, and hash join will be allowable for this call; that hook would then allow for control over the join method and the join order, and the join order control is strong enough that you can implement either of the two interpretations above. This idea theorizes that 0001 was wrong to make the path mask a per-RelOptInfo value, because there could be many calls to add_paths_to_joinrel() for a single RelOptInfo and, in this idea, every one of those can enforce a different mask. Potentially, such a hook could return additional information, either by using more bits of the bitmask or by returning other information via some other data type. For instance, I still believe that distinguishing between parameterized-nestloops and unparameterized-nestloops would be real darn useful, so we could have separate bits for each; or you could have a bit to control whether foreign-join paths get disabled (or are considered at all), or you could have separate bits for merge joins that involve 0, 1, or 2 sorts. Whether we need or want any or all of that is certainly debatable, but the point is that if you did want some of that, or something else, it doesn't look difficult to feed that information through to the places where you would need it to be available. Thoughts? -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-08-27 20:07 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-03-02 18:57 [PATCH 3/6] Optimize allocations in bringetbitmap Tomas Vondra <[email protected]> 2024-08-26 17:37 Re: allowing extensions to control planner behavior Tom Lane <[email protected]> 2024-08-26 19:28 ` Re: allowing extensions to control planner behavior Robert Haas <[email protected]> 2024-08-27 20:07 ` Re: allowing extensions to control planner behavior Robert Haas <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox