MIME-Version: 1.0
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>
In-Reply-To: <8EBFBFBC-DAC2-4521-AE16-069F7FD8EFBC@postgresql.org>
From: Robert Haas <robertmhaas@gmail.com>
Date: Wed, 23 Apr 2025 12:12:11 -0400
Message-ID: <CA+Tgmob1HFe-EbDmLs-xoQQfWAj8aB_cPToW-OOfhs4mFLfizg@mail.gmail.com>
Subject: Re: pgsql: Add function to get memory context stats for processes
To: Daniel Gustafsson <dgustafsson@postgresql.org>
Cc: Andres Freund <andres@anarazel.de>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Rahila Syed <rahilasyed90@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CA%2BTgmob1HFe-EbDmLs-xoQQfWAj8aB_cPToW-OOfhs4mFLfizg%40mail.gmail.com>
Precedence: bulk

On Tue, Apr 22, 2025 at 4:12=E2=80=AFAM Daniel Gustafsson
<dgustafsson@postgresql.org> wrote:
> Do you mean that an interrupt handler will make DSA allocations?  I don't=
 think
> that would be something we'd want to allow regardless of this patch.  Or =
did I
> misunderstand the above?

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.

I thought that the issues here would be similar to
https://commitfest.postgresql.org/patch/5473/ and
https://commitfest.postgresql.org/patch/5330/ where it seems we need
to go to fairly extraordinary lengths to try to make the operation
safe -- the proposal there is essentially to have a CFI handler run
around the executor state tree and replace every ExecProcNode pointer
with a pointer to some new wrapper function which in turn does the
actual query-plan printing. Even that requires some tricky footwork to
get right, but the core idea is that you can't imagine that
CHECK_FOR_INTERRUPTS() is a safe place to do arbitrary stuff, so you
have to use it to trigger the actual work at some later point where it
will be safe to do the stuff that you want to do. I suppose the
difference in this case is that memory-allocation is a lower-level
subsystem than query execution and therefore there are presumably
fewer places where it's not safe to assume that you can do
memory-allocation type operations. But I at least feel like DSA is a
pretty high-level system that I wouldn't want to count on being able
to access from a fairly-arbitrary point in the code, which is why I'm
quite astonished to hear Andres basically saying "don't worry, it's
all fine!". But my astonishment does not mean that he is wrong.

--=20
Robert Haas
EDB: http://www.enterprisedb.com