On 2020/08/20 0:01, Tom Lane wrote:
Hadn't been paying attention to this thread up till now, but ...
Michael Paquier <mich...@paquier.xyz> writes:
By the way, I was looking at the code that has been committed, and I
think that it is awkward to have a SQL function in mcxt.c, which is a
rather low-level interface. I think that this new code should be
moved to its own file, one suggestion for a location I have being
src/backend/utils/adt/mcxtfuncs.c.
I agree with that, but I think this patch has a bigger problem:
why bother at all? It seems like a waste of code space and future
maintenance effort, because there is no use-case. In the situations
where you need to know where the memory went, you are almost never
in a position to leisurely execute a query and send the results over
to your client. This certainly would be useless to figure out why
an already-running query is eating space, for instance.
The only situation I could imagine where this would have any use is
where there is long-term (cross-query) bloat in, say, CacheMemoryContext
Yes, this feature is useful to check a cross-query memory bloat like
the bloats of relcache, prepared statements, PL/pgSQL cache,
SMgrRelationHash, etc. For example, several years before, my colleague
investigated the cause of the memory bloat by using the almost same
feature that pg_cheat_funcs extension provides, and then found that
the cause was that the application forgot to release lots of SAVEPONT.
--- but it's not even very helpful for that, since you can't examine
anything finer-grain than a memory context.
Yes, but even that information can be good hint when investigating
the memory bloat.
Plus you need to be
running an interactive session, or else be willing to hack up your
application to try to get it to inspect the view (and log the
results somewhere) at useful times.
On top of all that, the functionality has Heisenberg problems,
because simply using it changes what you are trying to observe,
in complex and undocumented ways (not that the documentation
would be of any use to non-experts anyway).
My own thoughts about improving the debugging situation would've been
to create a way to send a signal to a session to make it dump its
current memory map to the postmaster log (not the client, since the
client is unlikely to be prepared to receive anything extraneous).
But this is nothing like that.
I agree that we need something like this, i.e., the way to monitor the memory
usage of any process even during query running. OTOH, I think that the added
feature has a use case and is good as the first step.
Given the lack of clear use-case, and the possibility (admittedly
not strong) that this is still somehow a security hazard, I think
we should revert it. If it stays, I'd like to see restrictions
on who can read the view.
For example, allowing only the role with pg_monitor to see this view?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
