Horton

Thread

Commits

  1. Improve out-of-memory error reports by including memory context name.

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

  1. Small proposal to improve out-of-memory messages

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-29T01:07:08Z

    I was looking at
https://www.postgresql.org/message-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.

Thoughts?

			regards, tom lane

  2. Re: Small proposal to improve out-of-memory messages

    Craig Ringer <craig@2ndquadrant.com> — 2018-03-29T01:29:59Z

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

> I was looking at
> https://www.postgresql.org/message-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

  3. Re: Small proposal to improve out-of-memory messages

    Michael Paquier <michael@paquier.xyz> — 2018-03-29T02:33:24Z

    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

  4. Re: Small proposal to improve out-of-memory messages

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-03-31T13:36:07Z

    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

  5. Re: Small proposal to improve out-of-memory messages

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-31T15:08:38Z

    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