public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 3/6] Optimize allocations in bringetbitmap 5+ messages / 3 participants [nested] [flat]
* [PATCH 3/6] Optimize allocations in bringetbitmap @ 2020-09-13 10:12 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Tomas Vondra @ 2020-09-13 10:12 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 | 62 +++++++++++++++++++++++++++------- 1 file changed, 49 insertions(+), 13 deletions(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 14da9ed17f..3735c41788 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; @@ -398,11 +401,50 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) * Make room for per-attribute lists of scan keys that we'll pass to the * consistent support procedure. We keep null and regular keys separate, * so that we can easily pass regular keys to the consistent function. + * + * 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. */ - 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 = + /* regular keys */ + MAXALIGN(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts) + + MAXALIGN(sizeof(ScanKey) * scan->numberOfKeys * bdesc->bd_tupdesc->natts) + + MAXALIGN(sizeof(int) * bdesc->bd_tupdesc->natts) + + /* NULL keys */ + MAXALIGN(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts) + + 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. @@ -438,9 +480,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); @@ -451,17 +493,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 --------------310A2AE1CC4C2E2E77559E3D Content-Type: text/x-patch; charset=UTF-8; name="0004-BRIN-bloom-indexes-20210114-2.patch" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="0004-BRIN-bloom-indexes-20210114-2.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Enhancing Memory Context Statistics Reporting @ 2025-04-08 16:41 Fujii Masao <[email protected]> 2025-04-08 16:44 ` Re: Enhancing Memory Context Statistics Reporting Daniel Gustafsson <[email protected]> 2025-04-08 21:27 ` Re: Enhancing Memory Context Statistics Reporting Daniel Gustafsson <[email protected]> 0 siblings, 2 replies; 5+ messages in thread From: Fujii Masao @ 2025-04-08 16:41 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; Rahila Syed <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers On 2025/04/08 18:46, Daniel Gustafsson wrote: >> On 8 Apr 2025, at 10:03, Daniel Gustafsson <[email protected]> wrote: > >> There was a bug in the shmem init function which caused it to fail on Windows, >> the attached fixes that. > > With this building green in CI over several re-builds, and another pass over > the docs and code with pgindent etc done, I pushed this earlier today. A few > BF animals have built green so far but I will continue to monitor it. Thanks for committing this feature! I noticed that the third argument of pg_get_process_memory_contexts() is named "retries" in pg_proc.dat, while the documentation refers to it as "timeout". Since "retries" is misleading, how about renaming it to "timeout" in pg_proc.dat? Patch attached. Also, as I mentioned earlier, I encountered an issue when calling pg_get_process_memory_contexts() on the PID of a backend that had just encountered an error but hadn't finished rolling back. It led to the following situation: Session 1 (PID=70011): =# begin; =# select 1/0; ERROR: division by zero Session 2: =# select * from pg_get_process_memory_contexts(70011, false, 10); Session 1 terminated with: ERROR: ResourceOwnerEnlarge called after release started FATAL: terminating connection because protocol synchronization was lost Shouldn't this be addressed? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION From a79084fe3caaf791f2aa8d466603c085ccb8c5af Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Wed, 9 Apr 2025 01:27:48 +0900 Subject: [PATCH v1] Rename misleading argument in pg_get_process_memory_contexts(). Previously, the third argument of pg_get_process_memory_contexts() was named retries in pg_proc.dat, even though it actually specifies a timeout value in seconds. This name was misleading to users and inconsistent with the documentation, which correctly referred to it as timeout. This commit renames the argument to timeout in pg_proc.dat to improve clarity and maintain consistency with the documentation. --- src/include/catalog/pg_proc.dat | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 4708f55be18..62beb71da28 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -8578,7 +8578,7 @@ prorettype => 'record', proargtypes => 'int4 bool float8', proallargtypes => '{int4,bool,float8,text,text,text,_int4,int4,int8,int8,int8,int8,int8,int4,timestamptz}', proargmodes => '{i,i,i,o,o,o,o,o,o,o,o,o,o,o,o}', - proargnames => '{pid, summary, retries, name, ident, type, path, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes, num_agg_contexts, stats_timestamp}', + proargnames => '{pid, summary, timeout, name, ident, type, path, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes, num_agg_contexts, stats_timestamp}', prosrc => 'pg_get_process_memory_contexts' }, # non-persistent series generator -- 2.49.0 Attachments: [text/plain] v1-0001-Rename-misleading-argument-in-pg_get_process_memo.patch (1.6K, ../../[email protected]/2-v1-0001-Rename-misleading-argument-in-pg_get_process_memo.patch) download | inline diff: From a79084fe3caaf791f2aa8d466603c085ccb8c5af Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Wed, 9 Apr 2025 01:27:48 +0900 Subject: [PATCH v1] Rename misleading argument in pg_get_process_memory_contexts(). Previously, the third argument of pg_get_process_memory_contexts() was named retries in pg_proc.dat, even though it actually specifies a timeout value in seconds. This name was misleading to users and inconsistent with the documentation, which correctly referred to it as timeout. This commit renames the argument to timeout in pg_proc.dat to improve clarity and maintain consistency with the documentation. --- src/include/catalog/pg_proc.dat | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 4708f55be18..62beb71da28 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -8578,7 +8578,7 @@ prorettype => 'record', proargtypes => 'int4 bool float8', proallargtypes => '{int4,bool,float8,text,text,text,_int4,int4,int8,int8,int8,int8,int8,int4,timestamptz}', proargmodes => '{i,i,i,o,o,o,o,o,o,o,o,o,o,o,o}', - proargnames => '{pid, summary, retries, name, ident, type, path, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes, num_agg_contexts, stats_timestamp}', + proargnames => '{pid, summary, timeout, name, ident, type, path, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes, num_agg_contexts, stats_timestamp}', prosrc => 'pg_get_process_memory_contexts' }, # non-persistent series generator -- 2.49.0 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Enhancing Memory Context Statistics Reporting 2025-04-08 16:41 Re: Enhancing Memory Context Statistics Reporting Fujii Masao <[email protected]> @ 2025-04-08 16:44 ` Daniel Gustafsson <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Daniel Gustafsson @ 2025-04-08 16:44 UTC (permalink / raw) To: Fujii Masao <[email protected]>; +Cc: Rahila Syed <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers > On 8 Apr 2025, at 18:41, Fujii Masao <[email protected]> wrote: > On 2025/04/08 18:46, Daniel Gustafsson wrote: >>> On 8 Apr 2025, at 10:03, Daniel Gustafsson <[email protected]> wrote: >>> There was a bug in the shmem init function which caused it to fail on Windows, >>> the attached fixes that. >> With this building green in CI over several re-builds, and another pass over >> the docs and code with pgindent etc done, I pushed this earlier today. A few >> BF animals have built green so far but I will continue to monitor it. > > Thanks for committing this feature! > > I noticed that the third argument of pg_get_process_memory_contexts() is named > "retries" in pg_proc.dat, while the documentation refers to it as "timeout". > Since "retries" is misleading, how about renaming it to "timeout" in pg_proc.dat? > Patch attached. Ugh, that's my bad. It was changed from using retries to a timeout and I missed that. > Also, as I mentioned earlier, I encountered an issue when calling > pg_get_process_memory_contexts() on the PID of a backend that had just > encountered an error but hadn't finished rolling back. It led to > the following situation: > > Session 1 (PID=70011): > =# begin; > =# select 1/0; > ERROR: division by zero > > Session 2: > =# select * from pg_get_process_memory_contexts(70011, false, 10); > > Session 1 terminated with: > ERROR: ResourceOwnerEnlarge called after release started > FATAL: terminating connection because protocol synchronization was lost > > Shouldn't this be addressed? Sorry, this must've been missed in this fairly lon thread, will have a look at it tonight. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Enhancing Memory Context Statistics Reporting 2025-04-08 16:41 Re: Enhancing Memory Context Statistics Reporting Fujii Masao <[email protected]> @ 2025-04-08 21:27 ` Daniel Gustafsson <[email protected]> 2025-04-08 23:28 ` Re: Enhancing Memory Context Statistics Reporting Fujii Masao <[email protected]> 1 sibling, 1 reply; 5+ messages in thread From: Daniel Gustafsson @ 2025-04-08 21:27 UTC (permalink / raw) To: Fujii Masao <[email protected]>; +Cc: Rahila Syed <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers > On 8 Apr 2025, at 18:41, Fujii Masao <[email protected]> wrote: > I noticed that the third argument of pg_get_process_memory_contexts() is named > "retries" in pg_proc.dat, while the documentation refers to it as "timeout". I've committed this patch as it was obviously correct, thanks! > Also, as I mentioned earlier, I encountered an issue when calling > pg_get_process_memory_contexts() on the PID of a backend that had just > encountered an error but hadn't finished rolling back. It led to > the following situation: I reconfirmed that the bugfix that Rahila shared in [0] fixes this issue (and will fix others like it, as it's not related to this patch in particular but is a bug in DSM attaching). My plan is to take that for a more thorough review and test tomorrow and see how far it can be safely backpatched. Thanks for bringing this up, sorry about it getting a bit lost among all the emails. -- Daniel Gustafsson [0] CAH2L28shr0j3JE5V3CXDFmDH-agTSnh2V8pR23X0UhRMbDQD9Q@mail.gmail.com ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Enhancing Memory Context Statistics Reporting 2025-04-08 16:41 Re: Enhancing Memory Context Statistics Reporting Fujii Masao <[email protected]> 2025-04-08 21:27 ` Re: Enhancing Memory Context Statistics Reporting Daniel Gustafsson <[email protected]> @ 2025-04-08 23:28 ` Fujii Masao <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Fujii Masao @ 2025-04-08 23:28 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Rahila Syed <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers On 2025/04/09 6:27, Daniel Gustafsson wrote: >> On 8 Apr 2025, at 18:41, Fujii Masao <[email protected]> wrote: > >> I noticed that the third argument of pg_get_process_memory_contexts() is named >> "retries" in pg_proc.dat, while the documentation refers to it as "timeout". > > I've committed this patch as it was obviously correct, thanks! Thanks a lot! Since pg_proc.dat was modified, do we need to bump the catalog version? >> Also, as I mentioned earlier, I encountered an issue when calling >> pg_get_process_memory_contexts() on the PID of a backend that had just >> encountered an error but hadn't finished rolling back. It led to >> the following situation: > > I reconfirmed that the bugfix that Rahila shared in [0] fixes this issue (and > will fix others like it, as it's not related to this patch in particular but is > a bug in DSM attaching). My plan is to take that for a more thorough review > and test tomorrow and see how far it can be safely backpatched. Thanks for > bringing this up, sorry about it getting a bit lost among all the emails. Appreciate your work on this! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2025-04-08 23:28 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-09-13 10:12 [PATCH 3/6] Optimize allocations in bringetbitmap Tomas Vondra <[email protected]> 2025-04-08 16:41 Re: Enhancing Memory Context Statistics Reporting Fujii Masao <[email protected]> 2025-04-08 16:44 ` Re: Enhancing Memory Context Statistics Reporting Daniel Gustafsson <[email protected]> 2025-04-08 21:27 ` Re: Enhancing Memory Context Statistics Reporting Daniel Gustafsson <[email protected]> 2025-04-08 23:28 ` Re: Enhancing Memory Context Statistics Reporting Fujii Masao <[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