Small proposal to improve out-of-memory messages

On 29 March 2018 at 09:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I was looking at
/messages/by-id/CAG_=8kAYKjhQX3FmAWQBC95Evh3+
qszOQxkNMm1Q4W1QO7+c4Q@mail.gmail.com

in which the most salient issue is

2018-03-28 19:20:33.264 UTC [10580] cory@match ERROR: out of memory
2018-03-28 19:20:33.264 UTC [10580] cory@match DETAIL: Failed on

request of size 1610612736.

and it suddenly struck me that this'd be noticeably more useful, at least
for experts, if the errdetail included the name of the memory context
we were trying to allocate in. In this case it'd be nice to know right
off the bat whether the failure occurred in MessageContext, which looked
bloated already, or someplace else.

In the wake of commit 442accc3f one might think that the message should
also include the context "identifier" if any. But I'm specifically not
proposing that, because of the point made in the discussion of that patch
that some identifier strings might contain security-sensitive query
excerpts. Now that that commit has required all context "names" to be
compile-time constants, it's hard to see how there could be any security
downside to showing them in a user-visible message.

Of course, by the same token, maybe this change wouldn't be as useful
as I'm thinking. But I can think of past cases where being able to
distinguish, say, allocation failures in a query's global ExecutorState
versus ones in an ExprState would save some effort.

This would have been significantly useful to me in the past, so +1 from me.

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

On Thu, Mar 29, 2018 at 09:29:59AM +0800, Craig Ringer wrote:

This would have been significantly useful to me in the past, so +1 from me.

As long as that does not cost more memory, +1.
--
Michael

Tom Lane wrote:

In the wake of commit 442accc3f one might think that the message should
also include the context "identifier" if any. But I'm specifically not
proposing that, because of the point made in the discussion of that patch
that some identifier strings might contain security-sensitive query
excerpts. Now that that commit has required all context "names" to be
compile-time constants, it's hard to see how there could be any security
downside to showing them in a user-visible message.

How about using errdetail_log() to include the context identifier?

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

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

Tom Lane wrote:

In the wake of commit 442accc3f one might think that the message should
also include the context "identifier" if any. But I'm specifically not
proposing that, because of the point made in the discussion of that patch
that some identifier strings might contain security-sensitive query
excerpts. Now that that commit has required all context "names" to be
compile-time constants, it's hard to see how there could be any security
downside to showing them in a user-visible message.

How about using errdetail_log() to include the context identifier?

Not really excited about that; we have no field experience to say it'd
be useful. If we start finding ourselves asking "exactly which ExprState
was that", we could revisit the question perhaps.

Furthermore, because elog.c constructs the whole detail string in
ErrorContext, doing this would create a significant OOM hazard anytime
the context ID didn't fit into the preallocated ErrorContext space.
Context names are generally short enough that they're not adding to
our risk there, but the ID strings could be very long in some cases.
(mcxt.c avoids this hazard by writing directly to stderr, but elog.c
can't do that.)

regards, tom lane