Re: [pgsql-patches] Patch to avoid gprof profiling overwrites

> And the patch is where?

You caught me; guess I'd better make something up fast, huh?

Here it is, thanks.

-- Korry

>
> ---------------------------------------------------------------------------
>
> korryd(at)enterprisedb(dot)com wrote:
> > It's difficult to profile a backend server process (using gprof)
> > because each process overwrites any earlier profile as it exits.
> >
> > It is especially tricky to nab a useful profile if you happen to have
> > autovacuum enabled.
> >
> > This patch reduces the problem by forcing the backend to 'cd' to a new
> > directory ($PGDATA/gprof/pid) just before calling exit(), but only if
> > the backend was compiled with -DLINUX_PROFILE.
> >
> > I've tested this with Linux, but not with other host architectures.
> >
> > -- Korry
> >
> >
> >
> > --
> > Korry Douglas korryd(at)enterprisedb(dot)com
> > EnterpriseDB http://www.enterprisedb.com
>

--
Korry Douglas korryd(at)enterprisedb(dot)com
EnterpriseDB http://www.enterprisedb.com

korryd(at)enterprisedb(dot)com wrote:
>> And the patch is where?
>
> You caught me; guess I'd better make something up fast, huh?
>
> Here it is, thanks.
>
> -- Korry
>
>>
>> ---------------------------------------------------------------------------
>>
>> korryd(at)enterprisedb(dot)com <mailto:korryd(at)enterprisedb(dot)com> wrote:
>> > It's difficult to profile a backend server process (using gprof)
>> > because each process overwrites any earlier profile as it exits.
>> >
>> > It is especially tricky to nab a useful profile if you happen to have
>> > autovacuum enabled.
>> >
>> > This patch reduces the problem by forcing the backend to 'cd' to a new
>> > directory ($PGDATA/gprof/pid) just before calling exit(), but only if
>> > the backend was compiled with -DLINUX_PROFILE.
>> >

Nice addition - tested on FreeBSD 6.2, works great!

The name for the define variable could perhaps be better - feels silly
adding -DLINUX_PROFILE on Freebsd! (maybe just PROFILE or GPROF_PROFILE?).

Cheers

Mark

> Nice addition - tested on FreeBSD 6.2, works great!

Cool - thanks for the quick feedback.

> The name for the define variable could perhaps be better - feels silly
> adding -DLINUX_PROFILE on Freebsd! (maybe just PROFILE or GPROF_PROFILE?).

That wasn't my choice, there is other code elsewhere that depends on
that symbol, I just added a little bit more.

-- Korry

korryd(at)enterprisedb(dot)com wrote:
>> The name for the define variable could perhaps be better - feels silly
>> adding -DLINUX_PROFILE on Freebsd! (maybe just PROFILE or GPROF_PROFILE?).
>
> That wasn't my choice, there is other code elsewhere that depends on
> that symbol, I just added a little bit more.
>

Right - but LINUX_PROFILE was added to correct Linux specific oddities
with the time counter accumulation, whereas your patch is not Linux
specific at all. So I think a more representative symbol is required.

Cheers

Mark

Mark Kirkwood wrote:
> korryd(at)enterprisedb(dot)com wrote:
>>> The name for the define variable could perhaps be better - feels
>>> silly adding -DLINUX_PROFILE on Freebsd! (maybe just PROFILE or
>>> GPROF_PROFILE?).
>>
>> That wasn't my choice, there is other code elsewhere that depends on
>> that symbol, I just added a little bit more.
>>
>
> Right - but LINUX_PROFILE was added to correct Linux specific oddities
> with the time counter accumulation, whereas your patch is not Linux
> specific at all. So I think a more representative symbol is required.
>

In fact - thinking about this a bit more, probably a construction like:

#if defined(LINUX_PROFILE) || defined(PROFILE)

or similar would work - because I think those of us not on Linux do
*not* want to define LINUX_PROFILE, as our timer accumulation for forked
process works fine as it is...but we don't want to make Linux guys have
to define LINUX_PROFILE *and* PROFILE...

Mark Kirkwood <markir(at)paradise(dot)net(dot)nz> writes:
> Right - but LINUX_PROFILE was added to correct Linux specific oddities
> with the time counter accumulation, whereas your patch is not Linux
> specific at all. So I think a more representative symbol is required.

Yeah, that was my problem with the patch too. The issue that it's
addressing isn't Linux-specific in the least.

Is there a way to detect via #if that profiling is enabled? I wouldn't
expect a really portable answer, but maybe there's something that works
for gcc?

regards, tom lane

On Thu, 2007-02-01 at 00:59 -0500, Tom Lane wrote:
> Is there a way to detect via #if that profiling is enabled? I wouldn't
> expect a really portable answer, but maybe there's something that works
> for gcc?

What about a "--enable-gprof" (or "--enable-profiling"?) configure flag?
This could add the appropriate compiler flags to CFLAGS, enable
LINUX_PROFILE if on Linux, and enable the "gprof/pid" mkdir().

-Neil

Neil Conway wrote:
> What about a "--enable-gprof" (or "--enable-profiling"?) configure
> flag? This could add the appropriate compiler flags to CFLAGS, enable
> LINUX_PROFILE if on Linux, and enable the "gprof/pid" mkdir().

That would really only work for GCC, wouldn't it?

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

> > Right - but LINUX_PROFILE was added to correct Linux specific oddities
> > with the time counter accumulation, whereas your patch is not Linux
> > specific at all. So I think a more representative symbol is required.
>
> Yeah, that was my problem with the patch too. The issue that it's
> addressing isn't Linux-specific in the least.
>
> Is there a way to detect via #if that profiling is enabled? I wouldn't
> expect a really portable answer, but maybe there's something that works
> for gcc?

You're right - I hadn't really looked beyond the problem that I was
trying to solve.

The technique should work for gprof on other platforms, but, as you
point out, LINUX_PROFILE is unique to Linux (no, I hadn't noticed that,
but it seems pretty obvious in retrospect).

I like Neil's idea of a using --enable-profiling configure flag. Every
time I need to profile, I have to go search the archives for 'gprof' -
it would be nice to see --enable-profiling when I configure --help.

-- Korry

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Neil Conway wrote:
>> What about a "--enable-gprof" (or "--enable-profiling"?) configure
>> flag? This could add the appropriate compiler flags to CFLAGS, enable
>> LINUX_PROFILE if on Linux, and enable the "gprof/pid" mkdir().

> That would really only work for GCC, wouldn't it?

Well, yeah, but that's what many of us use anyway. I would envision it
as adding $(PROFILE) to CFLAGS, and then there would be one place
to adjust "-pg" to something else for another compiler --- perhaps the
template files could be given a chance to change PROFILE to something
else.

regards, tom lane

> >> What about a "--enable-gprof" (or "--enable-profiling"?) configure
> >> flag? This could add the appropriate compiler flags to CFLAGS, enable
> >> LINUX_PROFILE if on Linux, and enable the "gprof/pid" mkdir().
>
> > That would really only work for GCC, wouldn't it?
>
> Well, yeah, but that's what many of us use anyway. I would envision it
> as adding $(PROFILE) to CFLAGS, and then there would be one place
> to adjust "-pg" to something else for another compiler --- perhaps the
> template files could be given a chance to change PROFILE to something
> else.

I don't feel competent to muck around with configure.in (sorry, I'm not
tying to shirk the work, I've just never had any success in writing
configure/automake/autoconf stuff - I have the "leaping goats" book, but
I need a small meaningful example to start with).

Can someone else volunteer to make this change? And then forward the
patch to me so I can learn something useful about how to change
configure.in without breaking it?

-- Korry

korryd(at)enterprisedb(dot)com wrote:
> > >> What about a "--enable-gprof" (or "--enable-profiling"?) configure
> > >> flag? This could add the appropriate compiler flags to CFLAGS, enable
> > >> LINUX_PROFILE if on Linux, and enable the "gprof/pid" mkdir().
> >
> > > That would really only work for GCC, wouldn't it?
> >
> > Well, yeah, but that's what many of us use anyway. I would envision it
> > as adding $(PROFILE) to CFLAGS, and then there would be one place
> > to adjust "-pg" to something else for another compiler --- perhaps the
> > template files could be given a chance to change PROFILE to something
> > else.
>
>
> I don't feel competent to muck around with configure.in (sorry, I'm not
> tying to shirk the work, I've just never had any success in writing
> configure/automake/autoconf stuff - I have the "leaping goats" book, but
> I need a small meaningful example to start with).
>
> Can someone else volunteer to make this change? And then forward the
> patch to me so I can learn something useful about how to change
> configure.in without breaking it?

I can work on this.

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +