Message-ID: <91f216ec-e367-42a5-8586-f554c4c28574@vondra.me>
Date: Sat, 26 Apr 2025 23:07:46 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: pgsql: Add function to get memory context stats for processes
To: Tom Lane <tgl@sss.pgh.pa.us>, Robert Haas <robertmhaas@gmail.com>
Cc: Daniel Gustafsson <dgustafsson@postgresql.org>,
 Andres Freund <andres@anarazel.de>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Rahila Syed <rahilasyed90@gmail.com>
References: <E1u24yn-003Ggy-01@gemulon.postgresql.org>
 <CA+Tgmoaey-kOP1k5FaUnQFd1fR0majVebWcL8ogfLbG_nt-Ytg@mail.gmail.com>
 <zppwd3scfznxhptdwm765l73wttegi5mibaxyxdwihlmzg3khs@fdhzkq3uxyct>
 <CA+Tgmob7yyONbdv-DRRrWxQPL9AsXFxzs3OH1F6vxSc14prLFA@mail.gmail.com>
 <rkbjmh4fipn5wzf4kqnge3p2szvhd7kgxgccoho5dtntojmkvt@uppvdxpywiba>
 <CA+TgmoZgOwiF1auY+wxUBC+7GcjKqXFBf7D97sF4WL7HER4LgA@mail.gmail.com>
 <8EBFBFBC-DAC2-4521-AE16-069F7FD8EFBC@postgresql.org>
 <CA+Tgmob1HFe-EbDmLs-xoQQfWAj8aB_cPToW-OOfhs4mFLfizg@mail.gmail.com>
 <1428549.1745432074@sss.pgh.pa.us>
Content-Language: en-US
From: Tomas Vondra <tomas@vondra.me>
In-Reply-To: <1428549.1745432074@sss.pgh.pa.us>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/91f216ec-e367-42a5-8586-f554c4c28574%40vondra.me>
Precedence: bulk

On 4/23/25 20:14, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> My primary concern about the patch is that
>> ProcessGetMemoryContextInterrupt() can be called from any
>> CHECK_FOR_INTERRUPTS() and calls lots of DSA functions, including
>> dsa_create() and, via PublishMemoryContext(), dsa_allocate0(). I'm
>> shocked to hear that you and Andres think that's safe to do at any
>> current or future CHECK_FOR_INTERRUPTS() anywhere in the code; but
>> Andres seems very confident that it's fine, so perhaps I should just
>> stop worrying and be happy that we have the feature.
> 
> Just for the record, it sounds quite unsafe to me too.  I could
> credit it being all right to examine the process' MemoryContext data
> structures, but calling dsa_create() from CFI seems really insane.
> Way too many moving parts there.
> 

Maybe I'm oblivious to some very obvious issues, but why is this so
different from everything else that is already called from
ProcessInterrupts()? Perhaps dsa_create() is more complex compared to
the other stuff (haven't checked), but why would it be unsafe?

The one risk I can think of is a risk of recursion - if any of the
functions called from ProcessGetMemoryContextInterrupt() does CFI, could
that be a problem if there's another pending signal. I see some other
handlers (e.g. ProcessParallelApplyMessages) handle this by explicitly
holding interrupts. Should ProcessGetMemoryContextInterrupt() do the
same thing?

In any case, if DSA happens to not be the right way to transfer this,
what should we use instead? The only thing I can think of is some sort
of pre-allocated chunk of shared memory.


regards

-- 
Tomas Vondra