Re: [PATCH] Log crashed backend's query v3

On Thu, Oct 6, 2011 at 10:15 PM, gabrielle <gorthx(at)gmail(dot)com> wrote:
> On Wed, Oct 5, 2011 at 5:14 PM, Marti Raudsepp <marti(at)juffo(dot)org> wrote:
>> I think you intended to use the "Waiting on Author" status -- that
>> leaves the commitfest entry open. I will re-open the commitfest entry
>> myself, I hope that's OK.
>
> No worries, and yeah, I picked the wrong checkbox. :)
>
>> Here is version 3 of the patch.
>
> Looks good, and performs as advertised.  Thanks!

I think it would be safer to write this so that
pgstat_get_crashed_backend_activity writes its answer into a
statically allocated buffer and returns a pointer to that buffer,
rather than using palloc. I think the current coding might lead to a
memory leak in the postmaster, but even if it doesn't, it seems better
to make this as simple as possible.

Also, how about having CreateSharedBackendStatus store the length of
the backend activity buffer in a global somewhere, instead of
repeating the calculation here?

For this:

if (*(activity) == '\0')
return "<command string not enabled>";

I'd suggest that we instead return <command string not found>, and
avoid making judgements about how things got that way.

Other than that, it looks good to me. It's almost making me cry
thinking about how much time this would have saved me debugging server
crashes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company