Horton

Thread

Commits

  1. Add memory context identifier to portal context

  2. Rename MemoryContextCopySetIdentifier() for clarity

  3. Allow memory contexts to have both fixed and variable ident strings.

  4. Rethink MemoryContext creation to improve performance.

  1. Backend memory dump analysis

    Vladimir Sitnikov <sitnikov.vladimir@gmail.com> — 2018-03-23T16:18:52Z

    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?

Vladimir

  2. Re: Backend memory dump analysis

    Andres Freund <andres@anarazel.de> — 2018-03-23T17:37:59Z

    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

  3. Re: Backend memory dump analysis

    Vladimir Sitnikov <sitnikov.vladimir@gmail.com> — 2018-03-23T18:05:38Z

    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

  4. Re: Backend memory dump analysis

    Andres Freund <andres@anarazel.de> — 2018-03-23T18:11:57Z

    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

  5. Re: Backend memory dump analysis

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-23T18:33:25Z

    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

  6. Re: Backend memory dump analysis

    Andres Freund <andres@anarazel.de> — 2018-03-23T19:01:41Z

    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

  7. Re: Backend memory dump analysis

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-23T19:12:43Z

    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

  8. Re: Backend memory dump analysis

    Andres Freund <andres@anarazel.de> — 2018-03-23T19:20:50Z

    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

  9. Re: Backend memory dump analysis

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-23T19:41:03Z

    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

  10. Re: Backend memory dump analysis

    Teodor Sigaev <teodor@sigaev.ru> — 2018-03-23T20:51:41Z

    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/

  11. Re: Backend memory dump analysis

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-24T17:10:19Z

    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

  12. Re: Backend memory dump analysis

    Vladimir Sitnikov <sitnikov.vladimir@gmail.com> — 2018-03-25T17:11:39Z

    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

  13. Re: Backend memory dump analysis

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-25T17:31:05Z

    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

  14. Re: Backend memory dump analysis

    Vladimir Sitnikov <sitnikov.vladimir@gmail.com> — 2018-03-25T17:50:08Z

    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

  15. Re: Backend memory dump analysis

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-25T18:05:26Z

    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

  16. Re: Backend memory dump analysis

    Vladimir Sitnikov <sitnikov.vladimir@gmail.com> — 2018-03-25T21:16:08Z

    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:
https://www.postgresql.org/message-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:
https://www.postgresql.org/message-id/55D7F9CE.3040904%402ndquadrant.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

  17. Re: Backend memory dump analysis

    Craig Ringer <craig@2ndquadrant.com> — 2018-03-26T03:14:41Z

    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

  18. Re: Backend memory dump analysis

    Craig Ringer <craig@2ndquadrant.com> — 2018-03-26T03:16:33Z

    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

  19. Re: Backend memory dump analysis

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-26T22:12:09Z

    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

  20. Re: Backend memory dump analysis

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-27T16:47:12Z

    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

  21. Re: Backend memory dump analysis

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-03-27T17:07:47Z

    Great stuff.

My only gripe is the pattern where the identifier needs to be
re-installed when resetting the context.  I don't think we need to hold
push for that reason alone, but I bet we'll be revisiting that.

I suppose this infrastructure can be used to implement the idea in
https://www.postgresql.org/message-id/CAMsr+YHii-BCC7ddpbb8fpCgzt0wMRt5GYZ0W_kD_Ft8rwWPiQ@mail.gmail.com
in some more acceptable manner.  I'm not proposing it for now, just
parking the idea for a future patch.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  22. Re: Backend memory dump analysis

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-27T17:29:14Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> My only gripe is the pattern where the identifier needs to be
> re-installed when resetting the context.  I don't think we need to hold
> push for that reason alone, but I bet we'll be revisiting that.

Yeah, that's slightly annoying; if I'd found more than one case of that,
I'd want a better answer.  But it seems like contexts that are long-lived
enough to warrant labeling typically don't get reset during their
lifespans, so it shouldn't be a huge problem.

I considered having MemoryContextReset either Assert that context->ident
is NULL, or just forcibly reset it to NULL, thus preventing a dangling
pointer if someone gets this wrong.  But that would lock out a perfectly
valid coding pattern where the identifier is in the parent context, so
I'm not convinced it's a good idea.

> I suppose this infrastructure can be used to implement the idea in
> https://www.postgresql.org/message-id/CAMsr+YHii-BCC7ddpbb8fpCgzt0wMRt5GYZ0W_kD_Ft8rwWPiQ@mail.gmail.com
> in some more acceptable manner.  I'm not proposing it for now, just
> parking the idea for a future patch.

Ah, I thought I remembered the callback idea from some previous
discussion, but I'd not located this one.  I think I've got a nicer
API for the per-context-type stats functions than what Craig
proposes there, but we could imagine doing this API or something
close to it for MemoryContextStatsInternal.  Or, as I mentioned
before, an external caller could just implement the scan over the
context tree for itself, and format the data however it wants.

			regards, tom lane

  23. Re: Backend memory dump analysis

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-03-27T23:32:54Z

    On 3/27/18 12:47, Tom Lane wrote:
> 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.

Cool.

How about this one as well:

diff --git a/src/backend/utils/mmgr/portalmem.c
b/src/backend/utils/mmgr/portalmem.c
index 75a6dde32b..c08dc260e2 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -200,6 +200,7 @@ CreatePortal(const char *name, bool allowDup, bool
dupSilent)
    portal->portalContext = AllocSetContextCreate(TopPortalContext,
                                                  "PortalContext",
                                                  ALLOCSET_SMALL_SIZES);
+   MemoryContextCopySetIdentifier(portal->portalContext, name);

    /* create a resource owner for the portal */
    portal->resowner = ResourceOwnerCreate(CurTransactionResourceOwner,


The term CopySetIdentifier has confused me a bit.  (What's a "set
identifier"?)  Maybe use CopyAndSetIdentifier?  (We similarly have
MemoryContextResetAndDeleteChildren.)

I'm also not clear why this doesn't undo the previous optimization that
preferred making the identifier a compile time-constant.  Aren't we now
just going back to doing a pstrdup() every time?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  24. Re: Backend memory dump analysis

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-27T23:55:49Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> How about this one as well:

>     portal->portalContext = AllocSetContextCreate(TopPortalContext,
>                                                   "PortalContext",
>                                                   ALLOCSET_SMALL_SIZES);
> +   MemoryContextCopySetIdentifier(portal->portalContext, name);

Seems reasonable, although I think if you were to delay setting the
name till the end of that function, you could point to portal->name
and avoid the extra pstrdup.  Maybe that's useless microoptimization.

> The term CopySetIdentifier has confused me a bit.  (What's a "set
> identifier"?)  Maybe use CopyAndSetIdentifier?  (We similarly have
> MemoryContextResetAndDeleteChildren.)

No objection, do you want to make the change?

> I'm also not clear why this doesn't undo the previous optimization that
> preferred making the identifier a compile time-constant.  Aren't we now
> just going back to doing a pstrdup() every time?

Huh?  It's not undoing that, it's doubling down on it; the "name" now
*has* to be a compile-time constant.  Only for contexts that seem worthy
of carrying extra ID information, which is a small minority, do we bother
setting the ident field.  Even for those, in the majority of cases we can
avoid an extra strcpy because the identity info is being carried somewhere
inside the context already.

			regards, tom lane

  25. Re: Backend memory dump analysis

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-04-06T16:41:04Z

    On 3/27/18 19:55, Tom Lane wrote:
> Seems reasonable, although I think if you were to delay setting the
> name till the end of that function, you could point to portal->name
> and avoid the extra pstrdup.  Maybe that's useless microoptimization.

done

>> The term CopySetIdentifier has confused me a bit.  (What's a "set
>> identifier"?)  Maybe use CopyAndSetIdentifier?  (We similarly have
>> MemoryContextResetAndDeleteChildren.)
> 
> No objection, do you want to make the change?

and done

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services