Re: Improve monitoring of shared memory allocations

Hi,

I'm sorry, but I'm afraid this won't make it into PG18 :-(

AFAICS the updated patch is correct / not buggy for non-shared hash
tables, but considering I missed a pretty serious flaw before pushing
the original patch, and that the tests didn't catch that either ...

The risk/benefit is not really worth it for PG18. I think this needs a
bit more work/testing. The dynahash is a bit too critical piece, I don't
want to be reverting a patch a second time.

Sorry, I realize it's not a great outcome ...

On 4/4/25 17:45, Rahila Syed wrote:
> Hi,
>
> Analysis of the Bug <https://www.postgresql.org/message-
> id/3d36f787-8ba5-4aa2-abb1-7ade129a7b0e%40vondra.me> in 0002 reported by
> David Rowley :
> The 0001* patch allocates memory for the hash header, directory, segments,
> and elements collectively for both shared and non-shared hash tables. While
>  this approach works well for shared hash tables, it presents an issue
> for non-
> shared hash tables. Specifically, during the expand_table() process,
> non-shared hash tables may reallocate a new directory and free the old one.
> Since the directory's memory is no longer allocated individually, it
> cannot be freed
> separately. This results in the following error:
>
> ERROR: pfree called with invalid pointer 0x60a15edc44e0 (header
> 0x0000002000000008)
>
> These allocations are done together to improve reporting of shared
> memory allocations
> for shared hash tables. Similar change is done for non-shared hash
> tables only to maintain
> consistency since hash_create code is shared between both types of hash
> tables.
>

Thanks, that matches my conclusions after looking into this.

> One solution could be separating allocation of directory from rest of
> the allocations for 
> the non-shared hash tables, but this approach would undermine the
> purpose of
> doing the change for a non-shared hash table.
>

I don't follow. Why would that undermine the change for non-shared hash
tables? Why should this affect non-shared hash tables at all? The goal
was to improve accounting for shared hash tables.

> A better/safer solution would be to do this change only for shared hash
> tables and
> exclude the non-shared hash tables.
>
> I believe it's acceptable to allocate everything in a single block
> provided we are not trying
> to individually free any of these, which we do only for the directory
> pointer in dir_realloc.
> Additional segment allocation goes through seg_alloc and element
> allocations through
> element_alloc, which do not free existing chunks but instead allocate
> new ones with
> pointers in existing directories and segments.
> Thus, as long as we don't reallocate the directory, which we avoid in
> the case of shared
> hash tables, it should be safe to proceed with this change.
>

Right, I believe that's correct. But I admit I find the current dynahash
code a bit confusing, especially in how it mixes handling of non-shared
and shared hash tables. (Not the fault of this patch, ofc.)

> Please find attached the patch which removes the changes for non-shared
> hash tables
> and keeps them for shared hash tables.
>
> I tested this by running make-check, make-check world and the reproducer
> script shared
> by David. I also ran pgbench to test creation and expansion of some of the
> shared hash tables.
>

Thanks. I'm afraid the existing regression tests can't tell us much,
considering it didn't fail once with the reverted patch :-(

I did check the coverage in:

https://coverage.postgresql.org/src/backend/utils/hash/dynahash.c.gcov.html

and sure enough, dir_realloc() is not executed once. And there's a
couple more pieces that have the same issue (e.g. hash_freeze or parts
of get_hash_entry).

I realize that's not the fault of this patch, but would you be willing
to improve that? It'd certainly make me less concerned if we improved
this first.

regards

--
Tomas Vondra