Hi, here's version 4 of the patch.
On Wed, Oct 19, 2011 at 19:34, Robert Haas <robertmhaas@gmail.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