Thanks to Simon Riggs and Bruce for input that helped me put this together.

--
Bill Moran
Collaborative Fusion Inc.



---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

Bill Moran wrote:
>
> Thanks to Simon Riggs and Bruce for input that helped me put this together.

Please change things to save the stat() syscall when the feature is not
in use.

Nitpick: also note our brace placement convention (though this would be
fixed by pgindent, but still).

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

Bill Moran wrote:
> + if (stat(vfdP->fileName, &filestats) == 0) {
> + if (trace_temp_files)
>
Shouldn't these tests be the other way around?

cheers

andrew

---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate

In response to Alvaro Herrera <alvherre@commandprompt.com>:

> Bill Moran wrote:
> >
> > Thanks to Simon Riggs and Bruce for input that helped me put this together.
>
> Please change things to save the stat() syscall when the feature is not
> in use.

Do you have a suggestion on how to do that and still have the PG_TRACE1()
work? That was specifically requested by Simon Riggs.

I'm not at all familiar with how the PG_TRACE probes work, so I'd be
interested to hear suggestions on how to wrap that in an if. If I remove
the PG_TRACE, it becomes a simple matter to skip the stat() if the feature
is disabled.

> Nitpick: also note our brace placement convention (though this would be
> fixed by pgindent, but still).

Sorry, I thought I _was_ following the convention. Must have missed
something. Is there a written style guide somewhere? Might drive things
home a little better than just looking at other folks code.

--
Bill Moran
Collaborative Fusion Inc.

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

Bill Moran <wmoran@collaborativefusion.com> writes:
> In response to Alvaro Herrera <alvherre@commandprompt.com>:
>> Please change things to save the stat() syscall when the feature is not
>> in use.

> Do you have a suggestion on how to do that and still have the PG_TRACE1()
> work? That was specifically requested by Simon Riggs.

Well, we are NOT paying a stat() call on every single file close,
whether Simon wants it or not. PG_TRACE1 doesn't even do anything
on non-Solaris platforms, for pete's sake.

Perhaps it would be reasonable to define trace_temp_files as the minimum
file size to log; then you could do something like

if (trace_temp_files > 0)
{
if (stat(vfdP->fileName, &filestats) < 0)
elog(LOG, ...);
else
{
if (filestats.st_size / BLCKSZ >= trace_temp_files)
ereport(LOG, ...);
PG_TRACE1(temp__file__cleanup, filestats.st_size);
}
}

Note that elog(ERROR) is quite inappropriate here.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

On Tue, 2007-01-02 at 18:20 -0500, Tom Lane wrote:
> Bill Moran <wmoran@collaborativefusion.com> writes:
> > In response to Alvaro Herrera <alvherre@commandprompt.com>:
> >> Please change things to save the stat() syscall when the feature is not
> >> in use.
>
> > Do you have a suggestion on how to do that and still have the PG_TRACE1()
> > work? That was specifically requested by Simon Riggs.
>
> Well, we are NOT paying a stat() call on every single file close,
> whether Simon wants it or not.

Simon doesn't/wouldn't want the stat() call on each file close.

If you put the PG_TRACE macro outside of the if test, yet prior to the
file close, you can pass the filename through like this

PG_TRACE1(temp__file__cleanup, vfdP->fileName);

That way DTrace can make its own call to find out filesize, if it would
like to... and we don't need to stat() before each temp file close.
That's much more flexible and useful, as well as better performance.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com



---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

In response to "Simon Riggs" <simon@2ndquadrant.com>:

> On Tue, 2007-01-02 at 18:20 -0500, Tom Lane wrote:
> > Bill Moran <wmoran@collaborativefusion.com> writes:
> > > In response to Alvaro Herrera <alvherre@commandprompt.com>:
> > >> Please change things to save the stat() syscall when the feature is not
> > >> in use.
> >
> > > Do you have a suggestion on how to do that and still have the PG_TRACE1()
> > > work? That was specifically requested by Simon Riggs.
> >
> > Well, we are NOT paying a stat() call on every single file close,
> > whether Simon wants it or not.
>
> Simon doesn't/wouldn't want the stat() call on each file close.
>
> If you put the PG_TRACE macro outside of the if test, yet prior to the
> file close, you can pass the filename through like this
>
> PG_TRACE1(temp__file__cleanup, vfdP->fileName);
>
> That way DTrace can make its own call to find out filesize, if it would
> like to... and we don't need to stat() before each temp file close.
> That's much more flexible and useful, as well as better performance.

OK, I think I've managed to adjust this patch so that everyone is happy
and it's better as well.

* PG_TRACE will work whether the GUC var is enabled or not, it sends the
fileName, as suggested by Simon
* stat() call is not made if trace_temp_files is disabled
* trace_temp_files is now an int: -1 disables, 0 and up equate to "log if
the file is this size or larger"
* Cleaned things up a bit -- should be more in line with PostgreSQL
coding standards
* failed stat is reported as LOG instead of ERROR

Done a bit of testing here, and everything seems to be in order.

--
Bill Moran
Collaborative Fusion Inc.


---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

Bill Moran wrote:
> + if (trace_temp_files != -1)
>

Might be more robust to say

if (trace_temp_files >= 0)

cheers

andrew

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

Andrew Dunstan <andrew@dunslane.net> wrote:
>
> Bill Moran wrote:
> > + if (trace_temp_files != -1)
> >
>
> Might be more robust to say
>
> if (trace_temp_files >= 0)

Because it would allow for the easy addition of more negative numbers
with magic value?

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

Bill Moran wrote:
> Andrew Dunstan <andrew@dunslane.net> wrote:
>>
>> Bill Moran wrote:
>> > + if (trace_temp_files != -1)
>> >
>>
>> Might be more robust to say
>>
>> if (trace_temp_files >= 0)
>
> Because it would allow for the easy addition of more negative numbers
> with magic value?
>


because ISTM any negative number here should mean no action is to be
taken. Otherwise how else is it different from 0?

cheers

andrew


---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate