Backend memory dump analysis

Hi,

On 2018-03-23 16:18:52 +0000, Vladimir Sitnikov wrote:

Hi,

I investigate an out of memory-related case for PostgreSQL 9.6.5, and it
looks like MemoryContextStatsDetail + gdb are the only friends there.

MemoryContextStatsDetail does print some info, however it is rarely
possible to associate the used memory with business cases.
For insance:
CachedPlanSource: 146224 total in 8 blocks; 59768 free (3 chunks); 86456
used
CachedPlanQuery: 130048 total in 7 blocks; 29952 free (2 chunks);
100096 used

It does look like a 182KiB has been spent for some SQL, however there's no
clear way to tell which SQL is to blame.

Another case: PL/pgSQL function context: 57344 total in 3 blocks; 17200
free (2 chunks); 40144 used
It is not clear what is there inside, which "cached plans" are referenced
by that pgsql context (if any), etc.

It would be great if there was an ability to dump the memory in a
machine-readable format (e.g. Java's HPROF).

Eclipse Memory Analyzer (https://www.eclipse.org/mat/) can visualize Java
memory dumps quite well, and I think HPROF format is trivial to generate
(the generation is easy, the hard part is to parse memory contents).
That is we could get analysis UI for free if PostgreSQL produces the dump.

Is it something welcome or non-welcome?
Is it something worth including in-core?

The overhead required for it (in cycles, in higher memory usage due to
additional bookeeping, in maintenance) makes me highly doubtful it's
worth going there. While I definitely can see the upside, it doesn't
seem to justify the cost.

Greetings,

Andres Freund

Andres>The overhead required for it (in cycles, in higher memory usage due
to
additional bookeeping

Does that mean the memory contexts are unparseable? (there's not enough
information to enumerate contents)

What if memory dump is produced by walking the C structures?
For instance, I assume statament cache is stored in some sort of a hash
table, so there should be a way to enumerate it in a programmatic way. Of
course it would take time, however I do not think it creates cpu/memory
overheads. The overhead is to maintain "walker" code.

Vladimir

On 2018-03-23 18:05:38 +0000, Vladimir Sitnikov wrote:

Andres>The overhead required for it (in cycles, in higher memory usage due
to
additional bookeeping

Does that mean the memory contexts are unparseable? (there's not enough
information to enumerate contents)

You can enumerate them (that's what the stats dump you're referring to
do), but you can't associate them with individual statements etc without
further bookepping.

What if memory dump is produced by walking the C structures?

We don't know the types of individual allocations.

For instance, I assume statament cache is stored in some sort of a hash
table, so there should be a way to enumerate it in a programmatic way. Of
course it would take time, however I do not think it creates cpu/memory
overheads. The overhead is to maintain "walker" code.

Sure, you could, entirely independent of the memory stats dump, do
that. But what information would you actually gain from it? Which row
something in the catcache belongs to isn't *that* interesting.

- Andres

Andres Freund <andres@anarazel.de> writes:

On 2018-03-23 18:05:38 +0000, Vladimir Sitnikov wrote:

For instance, I assume statament cache is stored in some sort of a hash
table, so there should be a way to enumerate it in a programmatic way. Of
course it would take time, however I do not think it creates cpu/memory
overheads. The overhead is to maintain "walker" code.

Sure, you could, entirely independent of the memory stats dump, do
that. But what information would you actually gain from it? Which row
something in the catcache belongs to isn't *that* interesting.

It'd certainly be easy to define this in a way that makes it require
a bunch of support code, which we'd be unlikely to want to write and
maintain.  However, I've often wished that the contexts in a memory
dump were less anonymous.  If you didn't just see a pile of "PL/pgSQL
function context" entries, but could (say) see the name of each function,
that would be a big step forward.  Similarly, if we could see the source
text for each CachedPlanSource in a dump, that'd be useful.  I mention
these things because we do actually store them already, in many cases
--- but the memory stats code doesn't know about them.

Now, commit 9fa6f00b1 already introduced a noticeable penalty for
contexts with nonconstant names, so trying to stick extra info like
this into the context name is not appetizing. But what if we allowed
the context name to have two parts, a fixed part and a variable part?
We could actually require that the fixed part be a compile-time-constant
string, simplifying matters on that end. The variable part would best
be assigned later than initial context creation, because you'd need a
chance to copy the string into the context before pointing to it.
So maybe, for contexts where this is worth doing, it'd look something
like this for plpgsql:

func_cxt = AllocSetContextCreate(TopMemoryContext,
"PL/pgSQL function context",
ALLOCSET_DEFAULT_SIZES);
plpgsql_compile_tmp_cxt = MemoryContextSwitchTo(func_cxt);

function->fn_signature = format_procedure(fcinfo->flinfo->fn_oid);
+ MemoryContextSetIdentifier(func_cxt, function->fn_signature);
function->fn_oid = fcinfo->flinfo->fn_oid;
function->fn_xmin = HeapTupleHeaderGetRawXmin(procTup->t_data);

This would cost an extra char * field in struct MemoryContextData,
which is slightly annoying but it doesn't exactly seem like a killer.
Then the memory stats dump code would just need to know to print this
field if it isn't NULL.

If we wanted to do this I'd suggest sneaking it into v11, so that
if people have to adapt their code because of 9fa6f00b1 breaking
usages with nonconstant context names, they have a solution to turn to
immediately rather than having to change things again in v12.

Thoughts?

regards, tom lane

Hi,

On 2018-03-23 14:33:25 -0400, Tom Lane wrote:

func_cxt = AllocSetContextCreate(TopMemoryContext,
"PL/pgSQL function context",
ALLOCSET_DEFAULT_SIZES);
plpgsql_compile_tmp_cxt = MemoryContextSwitchTo(func_cxt);

function->fn_signature = format_procedure(fcinfo->flinfo->fn_oid);
+ MemoryContextSetIdentifier(func_cxt, function->fn_signature);
function->fn_oid = fcinfo->flinfo->fn_oid;
function->fn_xmin = HeapTupleHeaderGetRawXmin(procTup->t_data);

This would cost an extra char * field in struct MemoryContextData,
which is slightly annoying but it doesn't exactly seem like a killer.
Then the memory stats dump code would just need to know to print this
field if it isn't NULL.

That's not a bad idea. How about storing a Node* instead of a char*?
Then we could have MemoryContextStats etc support digging out details
for a few types, without having to generate strings at runtime.

If we wanted to do this I'd suggest sneaking it into v11, so that
if people have to adapt their code because of 9fa6f00b1 breaking
usages with nonconstant context names, they have a solution to turn to
immediately rather than having to change things again in v12.

Yea, that'd make sense.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2018-03-23 14:33:25 -0400, Tom Lane wrote:

+ MemoryContextSetIdentifier(func_cxt, function->fn_signature);

This would cost an extra char * field in struct MemoryContextData,
which is slightly annoying but it doesn't exactly seem like a killer.
Then the memory stats dump code would just need to know to print this
field if it isn't NULL.

That's not a bad idea. How about storing a Node* instead of a char*?
Then we could have MemoryContextStats etc support digging out details
for a few types, without having to generate strings at runtime.

Well, in the cases I'm thinking of at the moment, there's no handy Node
to point at, just module-private structs like PLpgSQL_function. So doing
anything like that would add nonzero overhead to construct something.
Not sure we want to pay that. There's also the fact that we don't want
MemoryContextStats doing anything very complicated, because of the risk of
failure and the likelihood that any attempt to palloc would fail (if we're
there because we're up against OOM already).

If we wanted to do this I'd suggest sneaking it into v11, so that
if people have to adapt their code because of 9fa6f00b1 breaking
usages with nonconstant context names, they have a solution to turn to
immediately rather than having to change things again in v12.

Yea, that'd make sense.

I'll put together a draft patch.

regards, tom lane

On 2018-03-23 15:12:43 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2018-03-23 14:33:25 -0400, Tom Lane wrote:

+ MemoryContextSetIdentifier(func_cxt, function->fn_signature);

This would cost an extra char * field in struct MemoryContextData,
which is slightly annoying but it doesn't exactly seem like a killer.
Then the memory stats dump code would just need to know to print this
field if it isn't NULL.

That's not a bad idea. How about storing a Node* instead of a char*?
Then we could have MemoryContextStats etc support digging out details
for a few types, without having to generate strings at runtime.

Well, in the cases I'm thinking of at the moment, there's no handy Node
to point at, just module-private structs like PLpgSQL_function.

Well, the cases Vladimir were concerned about seem less clear
though. It'd be nice if we could just point to a CachedPlanSource and
such.

So doing anything like that would add nonzero overhead to construct
something.

I'm not that sure there aren't easy way to overcome those - couldn't we
"just" make FmgrInfo etc be tagged types? The space overhead of that
can't matter in comparison to the size of the relevant structs.

There's also the fact that we don't want MemoryContextStats doing
anything very complicated, because of the risk of failure and the
likelihood that any attempt to palloc would fail (if we're there
because we're up against OOM already).

That's true. But I'm not sure there's a meaningful difference in risk
here. Obviously you shouldn't try to print a node tree or something, but
an if statement looking

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2018-03-23 15:12:43 -0400, Tom Lane wrote:

Well, in the cases I'm thinking of at the moment, there's no handy Node
to point at, just module-private structs like PLpgSQL_function.

Well, the cases Vladimir were concerned about seem less clear
though. It'd be nice if we could just point to a CachedPlanSource and
such.

You could imagine adding *two* pointers to memory contexts, a callback
function and an arg to pass to it, so that the callback localizes the
knowledge of how to dig an identifier string out of whatever struct
is involved. I really doubt this is worth that much overhead though.
I think all of the actually interesting cases have a string available
already (though I might find out differently while doing the patch).
Furthermore, if they don't have a string available already, I'm not
real clear on how the callback would create one without doing a palloc.

I'm not that sure there aren't easy way to overcome those - couldn't we
"just" make FmgrInfo etc be tagged types? The space overhead of that
can't matter in comparison to the size of the relevant structs.

Not for extensions, eg PLs, which would be one of the bigger use-cases
IMO.

regards, tom lane

Hi!

Some help you could get from
https://github.com/postgrespro/memstat

Vladimir Sitnikov wrote:

Hi,

I investigate an out of memory-related case for PostgreSQL 9.6.5, and it
looks likeО©╫MemoryContextStatsDetailО©╫+ gdbО©╫are the only friends there.
--

Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/

I wrote:

I'll put together a draft patch.

Here's a draft patch for this. Some notes:

* I'm generally pretty happy with the way this turned out. For instance,
you can now tell the difference between index info, partition descriptor,
and RLS policy sub-contexts of CacheMemoryContext; previously they were
all just labeled with the name of their relation, which is useful but not
really enough anymore. I've attached sample MemoryContextStats output
captured near the end of the plpgsql.sql regression test.

* I reverted the addition of the "flags" parameter to the context creation
functions, since it no longer had any use. We could have left it there
for future expansion, but doing so seems like an unnecessary deviation
from the v10 APIs. We can cross that bridge when and if we come to it.

* I'd have liked to get rid of the AllocSetContextCreate wrapper macro
entirely, reverting that to the way it was in v10 as well, but I don't see
any way to do so without giving up the __builtin_constant_p(name) check,
which seems like it'd be a bad idea.

* In some places there's a pstrdup more than there was before (hidden
inside MemoryContextCopySetIdentifier), but I don't think this is really
much more expensive than the previous behavior with MEMCONTEXT_COPY_NAME.
In particular, the fact that having a non-constant identifier no longer
disqualifies a context from participating in aset.c's freelist scheme
probably buys back the overhead. I haven't done any performance testing,
though.

* It seemed worth expending a pstrdup to copy the source string for a
CachedPlan into its context so it could be labeled properly. We can't
just point to the source string in the originating CachedPlanSource
because that might have a different/shorter lifespan. I think in the
long run this would come out to be "free" anyway because someday we're
going to insist on having the source string available at execution for
error-reporting reasons.

* On the other hand, I didn't copy the source string into SPI Plan
contexts. Looking at the sample output, I started to get a bit annoyed
that we have those as separate contexts at all --- at least in the
standard case of one subsidiary CachedPlanSource, seems like we could
combine the contexts. That's fit material for a separate patch, though.

* While I didn't do anything about it here, I think it'd likely be a
good idea for MemoryContextStats printout to truncate the context ID
strings at 100 characters or so. Otherwise, in situations where we
have long queries with cached plans, the output would get unreasonably
bulky. Any thoughts about the exact rule?

* With the preceding idea in mind, I decided to fix things so that the
formatting of MemoryContextStats output is centralized in one place
instead of being repeated in each context module. So there's now a
callback-function API there. This has the additional benefit that people
could write extensions that collect stats output and do $whatever with it,
without relying on anything more than what memnodes.h exposes.

Comments, objections?

regards, tom lane

It looks much better.

While I didn't do anything about it here, I think it'd likely be a
good idea for MemoryContextStats printout to truncate the context ID
strings at 100 characters or so

It would be great if there was an option to show full sql.
For instance, current statistics is not sorted (should it be?), so it just
prints the first 100 items no matter the size of entries.
Luckily there's a function that accepts "number of nodes to print" as an
argument.
It would be better if it printed the 100 top consumers (including
descendants) though.

I think it makes sense to move all the numerics to the front, so the
numbers are located in the more or less the same columns.
Current output is hard to read as you basically have to search for "free"
and "used" markers.

What do you think of (number of bytes are printed with 6 characters, and
number of chunks with 2 characters)

16384 total in 2 blocks; 12448 used; 3936 free ( 5 chunks):
PL/pgSQL function tg_pslot_biu()

instead of

PL/pgSQL function tg_pslot_biu(): 16384 total in 2 blocks; 3936 free
(5 chunks); 12448 used

?
I think "used memory" is more important than "free". As far as I
understand, the main use-case is "analyze memory consumption", so one cares
"what is consuming the memory" more than "what context have enough free
space".

PS. "TopMemoryContext: 2143904 total" and "Grand total: 13115912 bytes"
does confuse. It basically says "TopMemoryContext is 2MiB, and grant total
is somehow 12MiB". It does not clarify if totals are "self memory" or
"including descendant contexts". In your case the difference is 10MiB, and
it is not that obvious why is that.

Vladimir

Vladimir Sitnikov <sitnikov.vladimir@gmail.com> writes:

While I didn't do anything about it here, I think it'd likely be a
good idea for MemoryContextStats printout to truncate the context ID
strings at 100 characters or so

It would be great if there was an option to show full sql.

Well, as I said, you can do anything you want now in an extension.
But we've had complaints specifically about overly-verbose memory maps
getting spewed to the postmaster log --- that's why there's a default
limit to 100 child contexts now. So I think the standard behavior has
to limit the length of the ID printouts.

(I've since updated my draft patch to do that, and also to convert all
ASCII control characters in an ID to spaces, so that the printouts are
back to a single line per context.)

For instance, current statistics is not sorted (should it be?), so it just
prints the first 100 items no matter the size of entries.

It's not going to be sorted, because of the concerns around not consuming
extra memory when we are reporting an ENOMEM problem. Again, if you're
writing an extension that's going to capture memory usage in non-emergency
scenarios, you can make it do whatever you like.

I think it makes sense to move all the numerics to the front, so the
numbers are located in the more or less the same columns.

I don't agree. Actually the key number is the one that already is printed
first, ie the total space consumed by the context; all the rest is detail.
Very occasionally, you might be interested in spotting contexts that have
a disproportionate amount of free space, but IME that's seldom the main
issue.

There might be an argument for putting the context ID at the end, along
the lines of

PL/pgSQL function: 16384 total in 2 blocks; 6672 free (4 chunks); 9712 used (alter_table_under_transition_tables_upd_func())

or maybe better with a colon instead of parens:

CachedPlan: 8192 total in 4 blocks; 1504 free (0 chunks); 6688 used: SELECT (SELECT COUNT(*) FROM (SELECT * FROM new_test UNION ALL SELECT * FROM new_test) ss)

so that the total is still reasonably prominent --- it's certainly true
that long context IDs are going to make it harder to see that number,
if they're in front. But this doesn't look terribly nice otherwise, so
I'm not sure. Thoughts?

regards, tom lane

Tom>Well, as I said, you can do anything you want now in an extension.

That is true. However it basically means "everybody who cares to
troubleshoot the memory use of a production system should install an
extension".
Should
https://wiki.postgresql.org/wiki/Developer_FAQ#Examining_backend_memory_use
provide
a link to the extension then?

Tom>Actually the key number is the one that already is printed
Tom>first, ie the total space consumed by the context

The space used is more important than the context name itself.

What do you think of

8192 (2 blocks) CachedPlan: 1504 free (0 chunks); 6688 used: SELECT
(SELECT COUNT(*) FROM (SELECT * FROM new_test UNION ALL SELECT *
FROM new_test) ss)

?
PS. "1504 free (0 chunks)" reads odd.

Tom>Very occasionally, you might be interested in spotting contexts that
have
Tom>a disproportionate amount of free space, but IME that's seldom the main
Tom>issue.

Fully agree. That is why I suggest "total, used, free" order so it matches
the likelihood of usage.

Vladimir

Vladimir Sitnikov <sitnikov.vladimir@gmail.com> writes:

Tom>Well, as I said, you can do anything you want now in an extension.

That is true. However it basically means "everybody who cares to
troubleshoot the memory use of a production system should install an
extension".

If you're interested in capturing memory usage short of an ENOMEM
condition, or reporting the results anywhere but stderr, you're going
to need such a thing anyway. There's a lot of use-cases that
MemoryContextStats() doesn't cover, and can't be made to cover without
breaking its essential requirement to report ENOMEM successfully.

What do you think of

8192 (2 blocks) CachedPlan: 1504 free (0 chunks); 6688 used: SELECT
(SELECT COUNT(*) FROM (SELECT * FROM new_test UNION ALL SELECT *
FROM new_test) ss)

Not much. Maybe it's just that I've been looking at the existing output
format for too many years. But given the lack of previous complaints,
I'm disinclined to make large changes in it.

One concrete objection to the above is it'd obscure hierarchical
relationships in the context tree, such as

TopPortalContext: 8192 total in 1 blocks; 7656 free (2 chunks); 536 used
PortalContext: 1024 total in 1 blocks; 584 free (0 chunks); 440 used
ExecutorState: 8192 total in 1 blocks; 2960 free (0 chunks); 5232 used
printtup: 8192 total in 1 blocks; 7936 free (0 chunks); 256 used
ExprContext: 8192 total in 1 blocks; 7656 free (0 chunks); 536 used

regards, tom lane

Tom>One concrete objection to the above is it'd obscure hierarchical
relationships in the context tree,

What is the problem with relationships? Context names are aligned as well
provided 8192 is justified to 6-7-8-9 (you pick) characters.

Tom>But given the lack of previous complaints

1) Here it is:
/messages/by-id/547CEE32.9000606@fuzzy.cz

The log lists "TopMemoryContext: 136614192 total", so everybody follows
"ah, there's 130MiB used" route.
Nobody in the thread mentions 300MiB taken by "ExecutorState: 318758960
total".

It takes just a single pass to compute "total" (and it takes no memory), so
it would be much better if "TopMemoryContext: ..." was replaced with
"Total memory used by all contexts is XXX bytes"
Current TopMemoryContext row is extremely misleading.

2) Here is a complain on "100 contexts" limit:
/messages/by-id/55D7F9CE.3040904@2ndquadrant.com
Note: 100 was invented "at random" in response to "let's not print
everything by default". I do agree with having limit by default, however it
would be so much better
if it selected the rows to print even at a cost of increased CPU cycles for
the print procedure.
For instance: pgjdbc limits to 256 server-prepared statements by
default (per backend). That is current "...Stats" would just ignore at
least half of the prepared statements.

3) If you care so much on the number of passes (frankly speaking, I think
one can easily wait for 5-10 seconds for debugging/troubleshooting stuff),
then aggregate summary can still be computed and printed with no additional
passes (and very limited memory) if the tree is printed in "child-parent"
order.
That is print "parent context" information after children iteration is done.

PS. SQL text might involve sensitive information (e.g. logins, passwords,
personal IDs), so there might be security issues with printing SQL by
default.

Vladimir

On 24 March 2018 at 02:33, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

On 2018-03-23 18:05:38 +0000, Vladimir Sitnikov wrote:

For instance, I assume statament cache is stored in some sort of a hash
table, so there should be a way to enumerate it in a programmatic way.

Of

course it would take time, however I do not think it creates cpu/memory
overheads. The overhead is to maintain "walker" code.

Sure, you could, entirely independent of the memory stats dump, do
that. But what information would you actually gain from it? Which row
something in the catcache belongs to isn't *that* interesting.

It'd certainly be easy to define this in a way that makes it require
a bunch of support code, which we'd be unlikely to want to write and
maintain.  However, I've often wished that the contexts in a memory
dump were less anonymous.  If you didn't just see a pile of "PL/pgSQL
function context" entries, but could (say) see the name of each function,
that would be a big step forward.  Similarly, if we could see the source
text for each CachedPlanSource in a dump, that'd be useful.  I mention
these things because we do actually store them already, in many cases
--- but the memory stats code doesn't know about them.

Now, commit 9fa6f00b1 already introduced a noticeable penalty for
contexts with nonconstant names, so trying to stick extra info like
this into the context name is not appetizing. But what if we allowed
the context name to have two parts, a fixed part and a variable part?
We could actually require that the fixed part be a compile-time-constant
string, simplifying matters on that end. The variable part would best
be assigned later than initial context creation, because you'd need a
chance to copy the string into the context before pointing to it.
So maybe, for contexts where this is worth doing, it'd look something
like this for plpgsql:

func_cxt = AllocSetContextCreate(TopMemoryContext,
"PL/pgSQL function context",
ALLOCSET_DEFAULT_SIZES);
plpgsql_compile_tmp_cxt = MemoryContextSwitchTo(func_cxt);

function->fn_signature = format_procedure(fcinfo->flinfo->fn_oid);
+ MemoryContextSetIdentifier(func_cxt, function->fn_signature);
function->fn_oid = fcinfo->flinfo->fn_oid;
function->fn_xmin = HeapTupleHeaderGetRawXmin(procTup->t_data);

I'm a big fan of this, having stared at way too many dumps of "no idea
what's going on in there" memory usage.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 24 March 2018 at 03:01, Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2018-03-23 14:33:25 -0400, Tom Lane wrote:

func_cxt = AllocSetContextCreate(TopMemoryContext,
"PL/pgSQL function context",
ALLOCSET_DEFAULT_SIZES);
plpgsql_compile_tmp_cxt = MemoryContextSwitchTo(func_cxt);

function->fn_signature = format_procedure(fcinfo->flinfo->fn_oid);
+ MemoryContextSetIdentifier(func_cxt, function->fn_signature);
function->fn_oid = fcinfo->flinfo->fn_oid;
function->fn_xmin = HeapTupleHeaderGetRawXmin(procTup->t_data);

This would cost an extra char * field in struct MemoryContextData,
which is slightly annoying but it doesn't exactly seem like a killer.
Then the memory stats dump code would just need to know to print this
field if it isn't NULL.

That's not a bad idea. How about storing a Node* instead of a char*?
Then we could have MemoryContextStats etc support digging out details
for a few types, without having to generate strings at runtime.

That'd render it pretty useless for extensions, though.

I like the idea of being able to introspect state for particular kinds of
contexts, and not have to generate strings that 99.99% of the time won't
get get looked at.

Function pointers instead of char* ? It adds a significant potential
stability risk to MemoryContextStats() calls, but a great deal of
flexibility.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Vladimir Sitnikov <sitnikov.vladimir@gmail.com> writes:

It takes just a single pass to compute "total" (and it takes no memory), so
it would be much better if "TopMemoryContext: ..." was replaced with
"Total memory used by all contexts is XXX bytes"
Current TopMemoryContext row is extremely misleading.

This may or may not be a good thing to do, but in any case it's well
outside the scope of this patch, whose ambition is only to get additional
identification info attached to contexts for which that's useful.
Moreover, seeing how late we are in the v11 cycle, it's hard to justify
doing more; that smells like a new feature and the time for that is past
for this year. The only reason I'm considering this patch now at all
is that it rethinks some API changes we made earlier in v11, and it'd be
nice to avoid an additional round of churn there in v12.

PS. SQL text might involve sensitive information (e.g. logins, passwords,
personal IDs), so there might be security issues with printing SQL by
default.

Indeed, that's something that extensions would need to think about.
I do not believe it's an issue for MemoryContextStats though; the
postmaster log can already contain sensitive data.

regards, tom lane

Here's an updated patch that adjusts the output format per discussion:

- context identifier at the end of the line, so it's easier to see the
numbers

- identifiers truncated at 100 bytes, control characters replaced by
spaces

Also, I hacked things so that dynahash hash tables continue to print
the way they did before, since the hash table name is really what
you want to see there.

Sample output is same test case as last time (dump at end of plpgsql.sql
regression test script).

Barring objection I plan to push this shortly.

regards, tom lane