public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 2/8] Pass all scan keys to BRIN consistent function at once 3+ messages / 3 participants [nested] [flat]
* [PATCH 2/8] Pass all scan keys to BRIN consistent function at once @ 2020-09-12 13:07 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Tomas Vondra @ 2020-09-12 13:07 UTC (permalink / raw) Passing all scan keys to the BRIN consistent function at once may allow elimination of additional ranges, which would be impossible when only passing individual scan keys. The code continues to support both the original (one scan key at a time) and new (all scan keys at once) approaches, depending on whether the consistent function accepts three or four arguments. Author: Tomas Vondra <[email protected]> Reviewed-by: Alvaro Herrera <[email protected]> Reviewed-by: Mark Dilger <[email protected]> Reviewed-by: Alexander Korotkov <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/backend/access/brin/brin.c | 158 +++++++++++++++++++----- src/backend/access/brin/brin_validate.c | 4 +- 2 files changed, 126 insertions(+), 36 deletions(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 27ba596c6e..963b7079cf 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -390,6 +390,9 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) BrinMemTuple *dtup; BrinTuple *btup = NULL; Size btupsz = 0; + ScanKey **keys; + int *nkeys; + int keyno; opaque = (BrinOpaque *) scan->opaque; bdesc = opaque->bo_bdesc; @@ -411,6 +414,66 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) */ consistentFn = palloc0(sizeof(FmgrInfo) * bdesc->bd_tupdesc->natts); + /* + * Make room for per-attribute lists of scan keys that we'll pass to the + * consistent support procedure. We allocate space for all attributes, so + * that we don't have to bother determining which attributes are used. + * + * 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); + nkeys = palloc0(sizeof(int) * bdesc->bd_tupdesc->natts); + + /* + * Preprocess the scan keys - split them into per-attribute arrays. + */ + for (keyno = 0; keyno < scan->numberOfKeys; keyno++) + { + ScanKey key = &scan->keyData[keyno]; + AttrNumber keyattno = key->sk_attno; + + /* + * The collation of the scan key must match the collation used in the + * index column (but only if the search is not IS NULL/ IS NOT NULL). + * Otherwise we shouldn't be using this index ... + */ + Assert((key->sk_flags & SK_ISNULL) || + (key->sk_collation == + TupleDescAttr(bdesc->bd_tupdesc, + keyattno - 1)->attcollation)); + + /* First time we see this index attribute, so init as needed. */ + if (!keys[keyattno - 1]) + { + FmgrInfo *tmp; + + /* + * This is a bit of an overkill - we don't know how many scan keys + * are there for this attribute, so we simply allocate the largest + * number possible. This may waste a bit of memory, but we only + * expect small number of scan keys in general, so this should be + * negligible, and it's cheaper than having to repalloc + * repeatedly. + */ + keys[keyattno - 1] = palloc0(sizeof(ScanKey) * scan->numberOfKeys); + + /* First time this column, so look up consistent function */ + Assert(consistentFn[keyattno - 1].fn_oid == InvalidOid); + + tmp = index_getprocinfo(idxRel, keyattno, + BRIN_PROCNUM_CONSISTENT); + fmgr_info_copy(&consistentFn[keyattno - 1], tmp, + CurrentMemoryContext); + } + + /* Add key to the per-attribute array. */ + keys[keyattno - 1][nkeys[keyattno - 1]] = key; + nkeys[keyattno - 1]++; + } + /* allocate an initial in-memory tuple, out of the per-range memcxt */ dtup = brin_new_memtuple(bdesc); @@ -471,7 +534,7 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) } else { - int keyno; + int attno; /* * Compare scan keys with summary values stored for the range. @@ -481,51 +544,78 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) * no keys. */ addrange = true; - for (keyno = 0; keyno < scan->numberOfKeys; keyno++) + for (attno = 1; attno <= bdesc->bd_tupdesc->natts; attno++) { - ScanKey key = &scan->keyData[keyno]; - AttrNumber keyattno = key->sk_attno; - BrinValues *bval = &dtup->bt_columns[keyattno - 1]; + BrinValues *bval; Datum add; - /* - * The collation of the scan key must match the collation - * used in the index column (but only if the search is not - * IS NULL/ IS NOT NULL). Otherwise we shouldn't be using - * this index ... - */ - Assert((key->sk_flags & SK_ISNULL) || - (key->sk_collation == - TupleDescAttr(bdesc->bd_tupdesc, - keyattno - 1)->attcollation)); + /* skip attributes without any san keys */ + if (nkeys[attno - 1] == 0) + continue; - /* First time this column? look up consistent function */ - if (consistentFn[keyattno - 1].fn_oid == InvalidOid) - { - FmgrInfo *tmp; + bval = &dtup->bt_columns[attno - 1]; - tmp = index_getprocinfo(idxRel, keyattno, - BRIN_PROCNUM_CONSISTENT); - fmgr_info_copy(&consistentFn[keyattno - 1], tmp, - CurrentMemoryContext); - } + Assert((nkeys[attno - 1] > 0) && + (nkeys[attno - 1] <= scan->numberOfKeys)); /* * Check whether the scan key is consistent with the page * range values; if so, have the pages in the range added * to the output bitmap. * - * When there are multiple scan keys, failure to meet the - * criteria for a single one of them is enough to discard - * the range as a whole, so break out of the loop as soon - * as a false return value is obtained. + * The opclass may or may not support processing of + * multiple scan keys. We can determine that based on the + * number of arguments - functions with extra parameter + * (number of scan keys) do support this, otherwise we + * have to simply pass the scan keys one by one, as + * before. */ - add = FunctionCall3Coll(&consistentFn[keyattno - 1], - key->sk_collation, - PointerGetDatum(bdesc), - PointerGetDatum(bval), - PointerGetDatum(key)); - addrange = DatumGetBool(add); + if (consistentFn[attno - 1].fn_nargs >= 4) + { + Oid collation; + + /* + * Collation from the first key (has to be the same + * for all keys for the same attribue). + */ + collation = keys[attno - 1][0]->sk_collation; + + /* Check all keys at once */ + add = FunctionCall4Coll(&consistentFn[attno - 1], + collation, + PointerGetDatum(bdesc), + PointerGetDatum(bval), + PointerGetDatum(keys[attno - 1]), + Int32GetDatum(nkeys[attno - 1])); + addrange = DatumGetBool(add); + } + else + { + /* + * Check keys one by one + * + * When there are multiple scan keys, failure to meet + * the criteria for a single one of them is enough to + * discard the range as a whole, so break out of the + * loop as soon as a false return value is obtained. + */ + int keyno; + + for (keyno = 0; keyno < nkeys[attno - 1]; keyno++) + { + add = FunctionCall3Coll(&consistentFn[attno - 1], + keys[attno - 1][keyno]->sk_collation, + PointerGetDatum(bdesc), + PointerGetDatum(bval), + PointerGetDatum(keys[attno - 1][keyno])); + addrange = DatumGetBool(add); + + /* mismatching key, no need to look further */ + if (!addrange) + break; + } + } + if (!addrange) break; } diff --git a/src/backend/access/brin/brin_validate.c b/src/backend/access/brin/brin_validate.c index 6d4253c05e..11835d85cd 100644 --- a/src/backend/access/brin/brin_validate.c +++ b/src/backend/access/brin/brin_validate.c @@ -97,8 +97,8 @@ brinvalidate(Oid opclassoid) break; case BRIN_PROCNUM_CONSISTENT: ok = check_amproc_signature(procform->amproc, BOOLOID, true, - 3, 3, INTERNALOID, INTERNALOID, - INTERNALOID); + 3, 4, INTERNALOID, INTERNALOID, + INTERNALOID, INT4OID); break; case BRIN_PROCNUM_UNION: ok = check_amproc_signature(procform->amproc, BOOLOID, true, -- 2.26.2 --------------AC3FFB734F56C5F52D422EFC Content-Type: text/x-patch; charset=UTF-8; name="0003-Process-all-scan-keys-in-existing-BRIN-opcl-20210305.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0003-Process-all-scan-keys-in-existing-BRIN-opcl-20210305.pa"; filename*1="tch" ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Use generation context to speed up tuplesorts @ 2022-01-07 12:03 Tomas Vondra <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Tomas Vondra @ 2022-01-07 12:03 UTC (permalink / raw) To: Ronan Dunklau <[email protected]>; [email protected]; +Cc: Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; David Rowley <[email protected]> On 1/7/22 12:03, Ronan Dunklau wrote: > Le vendredi 31 décembre 2021, 22:26:37 CET David Rowley a écrit : >> I've attached some benchmark results that I took recently. The >> spreadsheet contains results from 3 versions. master, master + 0001 - >> 0002, then master + 0001 - 0003. The 0003 patch makes the code a bit >> more conservative about the chunk sizes it allocates and also tries to >> allocate the tuple array according to the number of tuples we expect >> to be able to sort in a single batch for when the sort is not >> estimated to fit inside work_mem. > > (Sorry for trying to merge back the discussion on the two sides of the thread) > > In https://www.postgresql.org/message-id/4776839.iZASKD2KPV%40aivenronan, I > expressed the idea of being able to tune glibc's malloc behaviour. > > I implemented that (patch 0001) to provide a new hook which is called on > backend startup, and anytime we set work_mem. This hook is # defined depending > on the malloc implementation: currently a default, no-op implementation is > provided as well as a glibc's malloc implementation. > Not sure I'd call this a hook - that usually means a way to plug-in custom code through a callback, and this is simply ifdefing a block of code to pick the right implementation. Which may be a good way to do that, just let's not call that a hook. There's a commented-out MallocTuneHook() call, probably not needed. I wonder if #ifdefing is sufficient solution, because it happens at compile time, so what if someone overrides the allocator in LD_PRELOAD? That was a fairly common way to use a custom allocator in an existing application. But I don't know how many people do that with Postgres (I'm not aware of anyone doing that) or if we support that (it'd probably apply to other stuff too, not just malloc). So maybe it's OK, and I can't think of a better way anyway. > The glibc's malloc implementation relies on a new GUC, > glibc_malloc_max_trim_threshold. When set to it's default value of -1, we > don't tune malloc at all, exactly as in HEAD. If a different value is provided, > we set M_MMAP_THRESHOLD to half this value, and M_TRIM_TRESHOLD to this value, > capped by work_mem / 2 and work_mem respectively. > > The net result is that we can then allow to keep more unused memory at the top > of the heap, and to use mmap less frequently, if the DBA chooses too. A > possible other use case would be to on the contrary, limit the allocated > memory in idle backends to a minimum. > > The reasoning behind this is that glibc's malloc default way of handling those > two thresholds is to adapt to the size of the last freed mmaped block. > > I've run the same "up to 32 columns" benchmark as you did, with this new patch > applied on top of both HEAD and your v2 patchset incorporating planner > estimates for the block sizez. Those are called "aset" and "generation" in the > attached spreadsheet. For each, I've run it with > glibc_malloc_max_trim_threshold set to -1, 1MB, 4MB and 64MB. In each case > I've measured two things: > - query latency, as reported by pgbench > - total memory allocated by malloc at backend ext after running each query > three times. This represents the "idle" memory consumption, and thus what we > waste in malloc inside of releasing back to the system. This measurement has > been performed using the very small module presented in patch 0002. Please > note that I in no way propose that we include this module, it was just a > convenient way for me to measure memory footprint. > > My conclusion is that the impressive gains you see from using the generation > context with bigger blocks mostly comes from the fact that we allocate bigger > blocks, and that this moves the mmap thresholds accordingly. I wonder how much > of a difference it would make on other malloc implementation: I'm afraid the > optimisation presented here would in fact be specific to glibc's malloc, since > we have almost the same gains with both allocators when tuning malloc to keep > more memory. I still think both approaches are useful, and would be necessary. > Interesting measurements. It's intriguing that for generation contexts, the default "-1" often outperforms "1MB" (but not the other options), while for aset it's pretty much "the higher value the better". > Since this affects all memory allocations, I need to come up with other > meaningful scenarios to benchmarks. > OK. Are you thinking about a different microbenchmark, or something closer to real workload? regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Use generation context to speed up tuplesorts @ 2022-01-07 12:49 Ronan Dunklau <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Ronan Dunklau @ 2022-01-07 12:49 UTC (permalink / raw) To: [email protected]; +Cc: Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; David Rowley <[email protected]>; Tomas Vondra <[email protected]> Le vendredi 7 janvier 2022, 13:03:28 CET Tomas Vondra a écrit : > On 1/7/22 12:03, Ronan Dunklau wrote: > > Le vendredi 31 décembre 2021, 22:26:37 CET David Rowley a écrit : > >> I've attached some benchmark results that I took recently. The > >> spreadsheet contains results from 3 versions. master, master + 0001 - > >> 0002, then master + 0001 - 0003. The 0003 patch makes the code a bit > >> more conservative about the chunk sizes it allocates and also tries to > >> allocate the tuple array according to the number of tuples we expect > >> to be able to sort in a single batch for when the sort is not > >> estimated to fit inside work_mem. > > > > (Sorry for trying to merge back the discussion on the two sides of the > > thread) > > > > In https://www.postgresql.org/message-id/4776839.iZASKD2KPV%40aivenronan, > > I expressed the idea of being able to tune glibc's malloc behaviour. > > > > I implemented that (patch 0001) to provide a new hook which is called on > > backend startup, and anytime we set work_mem. This hook is # defined > > depending on the malloc implementation: currently a default, no-op > > implementation is provided as well as a glibc's malloc implementation. > > Not sure I'd call this a hook - that usually means a way to plug-in > custom code through a callback, and this is simply ifdefing a block of > code to pick the right implementation. Which may be a good way to do > that, just let's not call that a hook. > > There's a commented-out MallocTuneHook() call, probably not needed. Ok, I'll clean that up if we decide to proceed with this. > > I wonder if #ifdefing is sufficient solution, because it happens at > compile time, so what if someone overrides the allocator in LD_PRELOAD? > That was a fairly common way to use a custom allocator in an existing > application. But I don't know how many people do that with Postgres (I'm > not aware of anyone doing that) or if we support that (it'd probably > apply to other stuff too, not just malloc). So maybe it's OK, and I > can't think of a better way anyway. I couldn't think of a better way either, maybe there is something to be done with trying to dlsym something specific to glibc's malloc implementation ? > > > The glibc's malloc implementation relies on a new GUC, > > glibc_malloc_max_trim_threshold. When set to it's default value of -1, we > > don't tune malloc at all, exactly as in HEAD. If a different value is > > provided, we set M_MMAP_THRESHOLD to half this value, and M_TRIM_TRESHOLD > > to this value, capped by work_mem / 2 and work_mem respectively. > > > > The net result is that we can then allow to keep more unused memory at the > > top of the heap, and to use mmap less frequently, if the DBA chooses too. > > A possible other use case would be to on the contrary, limit the > > allocated memory in idle backends to a minimum. > > > > The reasoning behind this is that glibc's malloc default way of handling > > those two thresholds is to adapt to the size of the last freed mmaped > > block. > > > > I've run the same "up to 32 columns" benchmark as you did, with this new > > patch applied on top of both HEAD and your v2 patchset incorporating > > planner estimates for the block sizez. Those are called "aset" and > > "generation" in the attached spreadsheet. For each, I've run it with > > glibc_malloc_max_trim_threshold set to -1, 1MB, 4MB and 64MB. In each case > > > > I've measured two things: > > - query latency, as reported by pgbench > > - total memory allocated by malloc at backend ext after running each > > query > > > > three times. This represents the "idle" memory consumption, and thus what > > we waste in malloc inside of releasing back to the system. This > > measurement has been performed using the very small module presented in > > patch 0002. Please note that I in no way propose that we include this > > module, it was just a convenient way for me to measure memory footprint. > > > > My conclusion is that the impressive gains you see from using the > > generation context with bigger blocks mostly comes from the fact that we > > allocate bigger blocks, and that this moves the mmap thresholds > > accordingly. I wonder how much of a difference it would make on other > > malloc implementation: I'm afraid the optimisation presented here would > > in fact be specific to glibc's malloc, since we have almost the same > > gains with both allocators when tuning malloc to keep more memory. I > > still think both approaches are useful, and would be necessary. > Interesting measurements. It's intriguing that for generation contexts, > the default "-1" often outperforms "1MB" (but not the other options), > while for aset it's pretty much "the higher value the better". For generation context with "big block sizes" this result is expected, as the malloc dynamic tuning will adapt to the big block size. This can also be seen on the "idle memory" measurement: the memory consumption is identical to the 64MB value when using -1, since that's what we converge to. This makes it possible to configure postgres to be more conservative with memory: for example, if we have long lived backend where we sometime temporarily set work_mem to a high value, we may end up with a large memory foot print. The implementation I provide also requests a malloc trim when we lower the threshold, making it possible to release memory that would have otherwise been kept around forever. For aset, the memory allocation pattern is a bit more complicated, and we don't end up with such a high value for mmap_threshold. Also, one thing that I haven't explained yet is the weird outlier when there is only one column. > > > Since this affects all memory allocations, I need to come up with other > > meaningful scenarios to benchmarks. > > OK. Are you thinking about a different microbenchmark, or something > closer to real workload? Both. As for microbenchmarking, I'd like to test the following scenarios: - set returning functions allocating a lot of memory - maintenance operations: REINDEX TABLE and the like, where we may end up with a large amount of memory used. - operations involving large hash tables For real workloads, if you have something specific in mind let me know. One thing I didn't mention is that I set max_parallel_workers_per_gather to 0 in all tests. -- Ronan Dunklau ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2022-01-07 12:49 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-09-12 13:07 [PATCH 2/8] Pass all scan keys to BRIN consistent function at once Tomas Vondra <[email protected]> 2022-01-07 12:03 Re: Use generation context to speed up tuplesorts Tomas Vondra <[email protected]> 2022-01-07 12:49 ` Re: Use generation context to speed up tuplesorts Ronan Dunklau <[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