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

Hi,

My apologies for the last grumpy message I wrote; I sent it out of
surprise that my patch was marked "closed" on the current commitfest.
I hope I haven't discouraged you from reviewing this updated version.

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.

Here is version 3 of the patch.

On Wed, Oct 5, 2011 at 02:36, gabrielle <gorthx(at)gmail(dot)com> wrote:
> - The doc comment 'pgstat_get_backend_current_activity' doesn't match
> the function name 'pgstat_get_crashed_backend_activity'.

This is now fixed in the patch.

> Project coding guidelines:
> - There are some formatting problems, such as all newlines at the same
> indent level need to line up.
> - Wayward tabs, line 2725 in pgstat.c specifically

I wasn't entirely sure which line you're referring to (what's on line
2725 in the current git master wasn't touched by me)
But I presume it's this statement:
if (activity < BackendActivityBuffer || ...

I have rearranged this if statement using a temporary variable so that
there's no need to right-align long expressions anymore.

I also tweaked the wording of comments here and there.

For the points not addressed in this version, see my response here:
http://archives.postgresql.org/pgsql-hackers/2011-10/msg00202.php

Regards,
Marti

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!

gabrielle

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

Hi, here's version 4 of the patch.

On Wed, Oct 19, 2011 at 19:34, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> 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

Good catch about the memory leak; I always assumed that the caller
takes care of cleaning the memory context. But looking at the code,
that doesn't seem to happen in postmaster.

Using a global buffer would waste memory in every backend, but this is
needed rarely only in postmaster. So instead I'm allocating the buffer
on stack in LogChildExit(), and pass that to
pgstat_get_crashed_backend_activity() in arguments.

I use a character array of 1024 bytes in LogChildExit() since
'track_activity_query_size' is unknown at compile time (1024 is the
default). I could have used alloca(), but doesn't seem portable or
robust with arbitrary inputs coming from GUC.

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

Sure, I added a BackendActivityBufferSize global to pgstat.c

>                                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.

Originally I wanted to use exact same messages as
pg_stat_get_backend_activity; but you're right, we should be as
accurate as possible. I think "<command string empty>" is better,
since it means the PID was found, but it had a zero-length activity
string.

> It's almost making me cry
> thinking about how much time this would have saved me

Thanks for your review and the generous words. :)

Regards,
Marti

On Fri, Oct 21, 2011 at 11:45 AM, Marti Raudsepp <marti(at)juffo(dot)org> wrote:
>> It's almost making me cry
>> thinking about how much time this would have saved me
>
> Thanks for your review and the generous words. :)

I have committed this version. I'm expecting Tom to try to find a
scenario in which it's unfixably broken, so we'll see how that turns
out; but there seems to be significant support for this feature and
I'm hopeful that this will pass (or can be made to pass) muster.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I have committed this version. I'm expecting Tom to try to find a
> scenario in which it's unfixably broken, so we'll see how that turns
> out; but there seems to be significant support for this feature and
> I'm hopeful that this will pass (or can be made to pass) muster.

I found some problems with it, but with the changes I just committed
it seems like it should be fairly bulletproof.

regards, tom lane

On Fri, Oct 21, 2011 at 4:37 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I have committed this version.  I'm expecting Tom to try to find a
>> scenario in which it's unfixably broken, so we'll see how that turns
>> out; but there seems to be significant support for this feature and
>> I'm hopeful that this will pass (or can be made to pass) muster.
>
> I found some problems with it, but with the changes I just committed
> it seems like it should be fairly bulletproof.

Cool.

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