On 21 December 2017 at 14:58, Andres Freund <[email protected]> wrote:
> Hi,
>
> On 2017-12-21 14:49:28 +0800, Craig Ringer wrote:
> > +/*
> > + * Accumulate writes into the buffer in diag_request_buf,
> > + * for use with functions that expect a printf-like callback.
> > + */
> > +static void
> > +printwrapper_stringinfo(void *extra, const char * fmt, ...)
> > +{
> > + StringInfo out = extra;
> > + for (;;)
> > + {
> > + va_list args;
> > + int needed;
> > + va_start(args, fmt);
> > + needed = appendStringInfoVA(out, fmt, args);
> > + va_end(args);
> > + if (needed == 0)
> > + break;
> > + enlargeStringInfo(out, needed);
> > + }
> > }
>
> Hm, so I'm not entirely sure it's ok to use something that ERRORs on
> OOM. There's plenty of scenarios with thousands of memory contexts,
> making this output fairly large. If we want to make this usable in
> production, I'm not sure it's ok to introduce additional ERRORs. I
> wonder if we can change this to emit a static message if collecting the
> output exhausted memory.
There tons of callers to enlargeStringInfo, so a 'noerror' parameter would
be viable.
But I'm not convinced it's worth it personally. If we OOM in response to a
ProcSignal request for memory context output, we're having pretty bad luck.
The output is 8k in my test. But even if it were a couple of hundred kb,
happening to hit OOM just then isn't great luck on modern systems with many
gigabytes of RAM.
If that *does* happen, repalloc(...) will call
MemoryContextStats(TopMemoryContext) before returning NULL. So we'll get
our memory context dump anyway, albeit to stderr.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
