Re: Backend memory dump analysis
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
Re: Backend memory dump analysis
Peter Eisentraut 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
Re: Backend memory dump analysis
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
Re: Backend memory dump analysis
Alvaro Herrera 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_ft8rww...@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
Re: Backend memory dump analysis
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_ft8rww...@mail.gmail.com in some more acceptable manner. I'm not proposing it for now, just parking the idea for a future patch. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Backend memory dump analysis
Vladimir Sitnikov 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
Re: Backend memory dump analysis
On 24 March 2018 at 03:01, Andres Freund 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
Re: Backend memory dump analysis
On 24 March 2018 at 02:33, Tom Lane wrote: > Andres Freund writes: > > On 2018-03-23 18:05:38 +, 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
Re: Backend memory dump analysis
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.9000...@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
Re: Backend memory dump analysis
Vladimir Sitnikov 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
Re: Backend memory dump analysis
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
Re: Backend memory dump analysis
Vladimir Sitnikov 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
Re: Backend memory dump analysis
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
Re: Backend memory dump analysis
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: teo...@sigaev.ru WWW: http://www.sigaev.ru/
Re: Backend memory dump analysis
Andres Freund 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
Re: Backend memory dump analysis
On 2018-03-23 15:12:43 -0400, Tom Lane wrote: > Andres Freund 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
Re: Backend memory dump analysis
Andres Freund 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
Re: Backend memory dump analysis
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
Re: Backend memory dump analysis
Andres Freund writes: > On 2018-03-23 18:05:38 +, 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
Re: Backend memory dump analysis
On 2018-03-23 18:05:38 +, 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
Re: Backend memory dump analysis
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
Re: Backend memory dump analysis
Hi, On 2018-03-23 16:18:52 +, 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
