Message-ID: <3ff847de-de78-4933-8566-78335da29883@vondra.me>
Date: Mon, 7 Apr 2025 19:14:21 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Improve monitoring of shared memory allocations
To: Rahila Syed <rahilasyed90@gmail.com>
Cc: Nazir Bilal Yavuz <byavuz81@gmail.com>, Andres Freund
 <andres@anarazel.de>, PostgreSQL-development <pgsql-hackers@postgresql.org>
References: <CAH2L28vHzRankszhqz7deXURxKncxfirnuW68zD7+hVAqaS5GQ@mail.gmail.com>
 <CAH2L28uL-8EQTRSeyTpW1DqAXsDXRkXCkT1dus2u6p4HYDrxAg@mail.gmail.com>
 <CAH2L28uGLhkXBKDWFKm5XZtp_0nNqpYQ3Hc35vG++mM7wuOhgg@mail.gmail.com>
 <CAN55FZ3cJxy0VkeXpuO3K4BpjzJo3S6oU+iMyc00P6gEjqPztw@mail.gmail.com>
 <CAH2L28uwxJREzB62UjRDBumE87hHWUJJvRwxqqbO+7qFmoZfTg@mail.gmail.com>
 <3e40eeec-d8bf-4496-854e-485dd901f6a2@vondra.me>
 <CAH2L28uG_g1Ljo8aL-g1MupJXO4Y7-a-bUCriE7w2213+KSGdA@mail.gmail.com>
 <6bf7194e-4c34-4e6d-8215-f6acf8903974@vondra.me>
 <6705dbd2-060b-4f3c-9fcb-1c7f10880b26@vondra.me>
 <CAH2L28tzCFEk2bxQ+oYv6zda=LFLfd_9cmq7HzsT4nj9KN1Yvg@mail.gmail.com>
 <66283922-0448-44e7-bbdd-38aaf2be34b9@vondra.me>
 <CAH2L28soW40DafNmLEEWMrNDTMWYZ9STWSS5axP910WQUT=JcQ@mail.gmail.com>
 <7987a0b5-9933-457a-b5ba-d28e9cce883b@vondra.me>
 <CAH2L28vaktm0UXw19y22w+DThcqV80-UKtVfMxPeVw8ro2yTDg@mail.gmail.com>
 <4e9a124d-29cb-4fd5-99fb-a5016e508f20@vondra.me>
 <CAH2L28vgzvTUqNwQay=jx4w30sHMx_pC+emnZErv8oX0R+SALQ@mail.gmail.com>
Content-Language: en-US
From: Tomas Vondra <tomas@vondra.me>
In-Reply-To: <CAH2L28vgzvTUqNwQay=jx4w30sHMx_pC+emnZErv8oX0R+SALQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/3ff847de-de78-4933-8566-78335da29883%40vondra.me>
Precedence: bulk

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