public inbox for [email protected]
help / color / mirror / Atom feedpgsql: Improve accounting for memory used by shared hash tables
5+ messages / 3 participants
[nested] [flat]
* pgsql: Improve accounting for memory used by shared hash tables
@ 2025-04-02 15:16 Tomas Vondra <[email protected]>
2025-04-03 22:57 ` Re: pgsql: Improve accounting for memory used by shared hash tables David Rowley <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Tomas Vondra @ 2025-04-02 15:16 UTC (permalink / raw)
To: [email protected]
Improve accounting for memory used by shared hash tables
pg_shmem_allocations tracks the memory allocated by ShmemInitStruct(),
but for shared hash tables that covered only the header and hash
directory. The remaining parts (segments and buckets) were allocated
later using ShmemAlloc(), which does not update the shmem accounting.
Thus, these allocations were not shown in pg_shmem_allocations.
This commit improves the situation by allocating all the hash table
parts at once, using a single ShmemInitStruct() call. This way the
ShmemIndex entries (and thus pg_shmem_allocations) better reflect the
proper size of the hash table.
This affects allocations for private (non-shared) hash tables too, as
the hash_create() code is shared. For non-shared tables this however
makes no practical difference.
This changes the alignment a bit. ShmemAlloc() aligns the chunks using
CACHELINEALIGN(), which means some parts (header, directory, segments)
were aligned this way. Allocating all parts as a single chunk removes
this (implicit) alignment. We've considered adding explicit alignment,
but we've decided not to - it seems to be merely a coincidence due to
using the ShmemAlloc() API, not due to necessity.
Author: Rahila Syed <[email protected]>
Reviewed-by: Andres Freund <[email protected]>
Reviewed-by: Nazir Bilal Yavuz <[email protected]>
Reviewed-by: Tomas Vondra <[email protected]>
Discussion: https://postgr.es/m/CAH2L28vHzRankszhqz7deXURxKncxfirnuW68zD7+hVAqaS5GQ@mail.gmail.com
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/f5930f9a98ea65d659d41600a138e608988ad122
Modified Files
--------------
src/backend/storage/ipc/shmem.c | 4 +-
src/backend/utils/hash/dynahash.c | 281 +++++++++++++++++++++++++++++---------
src/include/utils/hsearch.h | 3 +-
3 files changed, 222 insertions(+), 66 deletions(-)
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgsql: Improve accounting for memory used by shared hash tables
2025-04-02 15:16 pgsql: Improve accounting for memory used by shared hash tables Tomas Vondra <[email protected]>
@ 2025-04-03 22:57 ` David Rowley <[email protected]>
2025-04-03 23:43 ` Re: pgsql: Improve accounting for memory used by shared hash tables Tomas Vondra <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: David Rowley @ 2025-04-03 22:57 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: [email protected]
On Thu, 3 Apr 2025 at 04:16, Tomas Vondra <[email protected]> wrote:
> Improve accounting for memory used by shared hash tables
I've not looked into why, but this is causing an issue in the
join_rel_hash during add_join_rel(). See the attached script.
ERROR: pfree called with invalid pointer 0x60a15edc44e0 (header
0x0000002000000008)
bisecting found this commit.
#0 BogusFree (pointer=0x5b36cd85a5e8) at ../src/backend/utils/mmgr/mcxt.c:288
__func__ = "BogusFree"
#1 0x00005b36c3d70e74 in dir_realloc (hashp=0x5b36cd4c0aa0) at
../src/backend/utils/hash/dynahash.c:1787
new_dirsize = <optimized out>
p = 0x5b36cd4c0b30
old_p = 0x5b36cd85a5e8
new_dsize = <optimized out>
old_dirsize = <optimized out>
p = <optimized out>
old_p = <optimized out>
new_dsize = <optimized out>
old_dirsize = <optimized out>
new_dirsize = <optimized out>
_vstart = <optimized out>
_val = <optimized out>
_len = <optimized out>
_start = <optimized out>
_stop = <optimized out>
#2 expand_table (hashp=0x5b36cd4c0aa0) at
../src/backend/utils/hash/dynahash.c:1691
hctl = 0x5b36cd85a298
old_bucket = <optimized out>
newlink = <optimized out>
nextElement = <optimized out>
old_seg = <optimized out>
new_bucket = 65536
new_segnum = 256
new_segndx = 0
currElement = <optimized out>
new_seg = <optimized out>
old_segnum = <optimized out>
old_segndx = <optimized out>
oldlink = <optimized out>
hctl = <optimized out>
old_seg = <optimized out>
new_seg = <optimized out>
old_bucket = <optimized out>
new_bucket = <optimized out>
new_segnum = <optimized out>
new_segndx = <optimized out>
old_segnum = <optimized out>
old_segndx = <optimized out>
oldlink = <optimized out>
newlink = <optimized out>
currElement = <optimized out>
nextElement = <optimized out>
#3 hash_search_with_hash_value (hashp=0x5b36cd4c0aa0,
keyPtr=0x5b3716ccc6f0, hashvalue=2952019273, action=<optimized out>,
foundPtr=<optimized out>) at ../src/backend/utils/hash/dynahash.c:1112
hctl = 0x5b36cd85a298
freelist_idx = 0
keysize = <optimized out>
currBucket = <optimized out>
prevBucketPtr = <optimized out>
match = <optimized out>
__func__ = "hash_search_with_hash_value"
#4 0x00005b36c3d71031 in hash_search (hashp=<optimized out>,
keyPtr=<optimized out>, action=<optimized out>, foundPtr=<optimized
out>) at ../src/backend/utils/hash/dynahash.c:1069
No locals.
#5 0x00005b36c3b195b3 in add_join_rel (root=0x5b36cd420d28,
joinrel=0x5b3716ccc6e8) at ../src/backend/optimizer/util/relnode.c:638
hentry = <optimized out>
found = false
David
Attachments:
[application/octet-stream] membug.sql (53.8K, 2-membug.sql)
download
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgsql: Improve accounting for memory used by shared hash tables
2025-04-02 15:16 pgsql: Improve accounting for memory used by shared hash tables Tomas Vondra <[email protected]>
2025-04-03 22:57 ` Re: pgsql: Improve accounting for memory used by shared hash tables David Rowley <[email protected]>
@ 2025-04-03 23:43 ` Tomas Vondra <[email protected]>
2025-04-04 00:41 ` Re: pgsql: Improve accounting for memory used by shared hash tables Tomas Vondra <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Tomas Vondra @ 2025-04-03 23:43 UTC (permalink / raw)
To: David Rowley <[email protected]>; Tomas Vondra <[email protected]>; +Cc: [email protected]
On 4/4/25 00:57, David Rowley wrote:
> On Thu, 3 Apr 2025 at 04:16, Tomas Vondra <[email protected]> wrote:
>> Improve accounting for memory used by shared hash tables
>
> I've not looked into why, but this is causing an issue in the
> join_rel_hash during add_join_rel(). See the attached script.
>
> ERROR: pfree called with invalid pointer 0x60a15edc44e0 (header
> 0x0000002000000008)
>
Thanks for the report and reproducer. I'll take a look tomorrow.
--
Tomas Vondra
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgsql: Improve accounting for memory used by shared hash tables
2025-04-02 15:16 pgsql: Improve accounting for memory used by shared hash tables Tomas Vondra <[email protected]>
2025-04-03 22:57 ` Re: pgsql: Improve accounting for memory used by shared hash tables David Rowley <[email protected]>
2025-04-03 23:43 ` Re: pgsql: Improve accounting for memory used by shared hash tables Tomas Vondra <[email protected]>
@ 2025-04-04 00:41 ` Tomas Vondra <[email protected]>
2025-04-04 02:58 ` Re: pgsql: Improve accounting for memory used by shared hash tables Tomas Vondra <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Tomas Vondra @ 2025-04-04 00:41 UTC (permalink / raw)
To: David Rowley <[email protected]>; Tomas Vondra <[email protected]>; +Cc: [email protected]
On 4/4/25 01:43, Tomas Vondra wrote:
> On 4/4/25 00:57, David Rowley wrote:
>> On Thu, 3 Apr 2025 at 04:16, Tomas Vondra <[email protected]> wrote:
>>> Improve accounting for memory used by shared hash tables
>>
>> I've not looked into why, but this is causing an issue in the
>> join_rel_hash during add_join_rel(). See the attached script.
>>
>> ERROR: pfree called with invalid pointer 0x60a15edc44e0 (header
>> 0x0000002000000008)
>>
>
> Thanks for the report and reproducer. I'll take a look tomorrow.
>
I took a quick look, and I think the reason is fairly simple - the
commit allocates the header and the directory as a single chunk. And for
shared hash tables that's fine, because those have non-expandable
directory. But the patch does the same thing for non-shared hash tables
(not intentionally), which means that if we end up expanding the hash,
it fails in dir_realloc(). Because hashp->dir is not a separately
allocated chunk.
This is clearly a bug in the patch, I should have caught this during a
review. But I'm also quite surprised none of the regression tests seems
to expand the hash table ...
I'll think about a way to fix this tomorrow.
--
Tomas Vondra
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgsql: Improve accounting for memory used by shared hash tables
2025-04-02 15:16 pgsql: Improve accounting for memory used by shared hash tables Tomas Vondra <[email protected]>
2025-04-03 22:57 ` Re: pgsql: Improve accounting for memory used by shared hash tables David Rowley <[email protected]>
2025-04-03 23:43 ` Re: pgsql: Improve accounting for memory used by shared hash tables Tomas Vondra <[email protected]>
2025-04-04 00:41 ` Re: pgsql: Improve accounting for memory used by shared hash tables Tomas Vondra <[email protected]>
@ 2025-04-04 02:58 ` Tomas Vondra <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Tomas Vondra @ 2025-04-04 02:58 UTC (permalink / raw)
To: David Rowley <[email protected]>; Tomas Vondra <[email protected]>; +Cc: [email protected]
On 4/4/25 02:41, Tomas Vondra wrote:
> On 4/4/25 01:43, Tomas Vondra wrote:
>> On 4/4/25 00:57, David Rowley wrote:
>>> On Thu, 3 Apr 2025 at 04:16, Tomas Vondra <[email protected]> wrote:
>>>> Improve accounting for memory used by shared hash tables
>>>
>>> I've not looked into why, but this is causing an issue in the
>>> join_rel_hash during add_join_rel(). See the attached script.
>>>
>>> ERROR: pfree called with invalid pointer 0x60a15edc44e0 (header
>>> 0x0000002000000008)
>>>
>>
>> Thanks for the report and reproducer. I'll take a look tomorrow.
>>
>
> I took a quick look, and I think the reason is fairly simple - the
> commit allocates the header and the directory as a single chunk. And for
> shared hash tables that's fine, because those have non-expandable
> directory. But the patch does the same thing for non-shared hash tables
> (not intentionally), which means that if we end up expanding the hash,
> it fails in dir_realloc(). Because hashp->dir is not a separately
> allocated chunk.
>
> This is clearly a bug in the patch, I should have caught this during a
> review. But I'm also quite surprised none of the regression tests seems
> to expand the hash table ...
>
> I'll think about a way to fix this tomorrow.
>
I ended up reverting this. Unfortunately, the patch assumed the
directory is pre-allocated and not expanding in more places. I wasn't
sure how long would it take me to fix this, or how invasive the fix
would be. It seems more appropriate to revert and then maybe apply a
reworked patch (not going to happen for PG18).
Thanks for the report, sorry for missing the issue in the first place.
--
Tomas Vondra
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2025-04-04 02:58 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-04-02 15:16 pgsql: Improve accounting for memory used by shared hash tables Tomas Vondra <[email protected]>
2025-04-03 22:57 ` David Rowley <[email protected]>
2025-04-03 23:43 ` Tomas Vondra <[email protected]>
2025-04-04 00:41 ` Tomas Vondra <[email protected]>
2025-04-04 02:58 ` Tomas Vondra <[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