Horton

Thread

  1. PATCH: tracking temp files in pg_stat_database

    Tomas Vondra <tv@fuzzy.cz> — 2011-12-20T01:00:00Z

    Hello everybody,

this patch adds two counters to pg_stat_database to track temporary
files - number of temp files and number of bytes. I see this as a useful
feature, as temporary files often cause a lot of IO (because of low
work_mem etc.). The log_temp_files is useful, but you have to parse the
log and only temp files exceeding a size limit are logged so the actual
amount of I/O is unknown.

The patch is rather simple:

1) two new fields in PgStat_StatDBEntry (n_temp_files, n_temp_bytes)
2) report/recv function in pgstat.c
3) FileClose modified to log stats for all temp files (see below)
4) two new fields added to pg_stat_database (temp_files, temp_bytes)

I had to modify FileClose to call stat() on each temp file as this
should log all temp files (not just when log_temp_file >= 0). But the
impact of this change should be negligible, considering that the user is
already using temp files.

I haven't updated the docs yet - let's see if the patch is acceptable at
all first.

Tomas

  2. Re: PATCH: tracking temp files in pg_stat_database

    Magnus Hagander <magnus@hagander.net> — 2011-12-20T10:20:41Z

    2011/12/20 Tomas Vondra <tv@fuzzy.cz>:
> Hello everybody,
>
> this patch adds two counters to pg_stat_database to track temporary
> files - number of temp files and number of bytes. I see this as a useful
> feature, as temporary files often cause a lot of IO (because of low
> work_mem etc.). The log_temp_files is useful, but you have to parse the
> log and only temp files exceeding a size limit are logged so the actual
> amount of I/O is unknown.

Hey, cool, that was on my personal TODO list :-)


> The patch is rather simple:
>
> 1) two new fields in PgStat_StatDBEntry (n_temp_files, n_temp_bytes)
> 2) report/recv function in pgstat.c
> 3) FileClose modified to log stats for all temp files (see below)
> 4) two new fields added to pg_stat_database (temp_files, temp_bytes)

I haven't reviewed the code itself yet, but that seems like a
reasonable approach.


> I had to modify FileClose to call stat() on each temp file as this
> should log all temp files (not just when log_temp_file >= 0). But the
> impact of this change should be negligible, considering that the user is
> already using temp files.
>
> I haven't updated the docs yet - let's see if the patch is acceptable at
> all first.

Again, without having reviewed the code, this looks like a feature
we'd want, so please add some docs, and then submit it for the next
commitfest!

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

  3. Re: PATCH: tracking temp files in pg_stat_database

    Tomas Vondra <tv@fuzzy.cz> — 2011-12-20T10:45:08Z

    On 20 Prosinec 2011, 11:20, Magnus Hagander wrote:
> 2011/12/20 Tomas Vondra <tv@fuzzy.cz>:
>>
>> I haven't updated the docs yet - let's see if the patch is acceptable at
>> all first.
>
> Again, without having reviewed the code, this looks like a feature
> we'd want, so please add some docs, and then submit it for the next
> commitfest!

Hm, I added it to the current commit fest - I should probably move it to
the next one (2012-01), right? I'll add the docs at the evening.

Tomas

  4. Re: PATCH: tracking temp files in pg_stat_database

    Magnus Hagander <magnus@hagander.net> — 2011-12-20T11:30:28Z

    On Tue, Dec 20, 2011 at 11:45, Tomas Vondra <tv@fuzzy.cz> wrote:
> On 20 Prosinec 2011, 11:20, Magnus Hagander wrote:
>> 2011/12/20 Tomas Vondra <tv@fuzzy.cz>:
>>>
>>> I haven't updated the docs yet - let's see if the patch is acceptable at
>>> all first.
>>
>> Again, without having reviewed the code, this looks like a feature
>> we'd want, so please add some docs, and then submit it for the next
>> commitfest!
>
> Hm, I added it to the current commit fest - I should probably move it to
> the next one (2012-01), right? I'll add the docs at the evening.

Yes, all new patches should always go on the one that's labeled
"Open". If we don't do it that way, we will never finish a CF...

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

  5. Re: PATCH: tracking temp files in pg_stat_database

    Tomas Vondra <tv@fuzzy.cz> — 2011-12-20T18:59:04Z

    On 20.12.2011 11:20, Magnus Hagander wrote:
> 2011/12/20 Tomas Vondra <tv@fuzzy.cz>:
>>
>> I haven't updated the docs yet - let's see if the patch is acceptable at
>> all first.
> 
> Again, without having reviewed the code, this looks like a feature
> we'd want, so please add some docs, and then submit it for the next
> commitfest!

I've added the docs (see the attachment) and rebased to current head.

Tomas

  6. Re: PATCH: tracking temp files in pg_stat_database

    Tomas Vondra <tv@fuzzy.cz> — 2012-01-17T20:39:23Z

    On 20.12.2011 19:59, Tomas Vondra wrote:
> On 20.12.2011 11:20, Magnus Hagander wrote:
>> 2011/12/20 Tomas Vondra <tv@fuzzy.cz>:
>>>
>>> I haven't updated the docs yet - let's see if the patch is acceptable at
>>> all first.
>>
>> Again, without having reviewed the code, this looks like a feature
>> we'd want, so please add some docs, and then submit it for the next
>> commitfest!
> 
> I've added the docs (see the attachment) and rebased to current head.
> 
> Tomas

Fixed a failing regression test (check of pg_stat_database structure).

Tomas

  7. Re: PATCH: tracking temp files in pg_stat_database

    Magnus Hagander <magnus@hagander.net> — 2012-01-20T12:23:55Z

    On Tue, Jan 17, 2012 at 21:39, Tomas Vondra <tv@fuzzy.cz> wrote:
> On 20.12.2011 19:59, Tomas Vondra wrote:
>> On 20.12.2011 11:20, Magnus Hagander wrote:
>>> 2011/12/20 Tomas Vondra <tv@fuzzy.cz>:
>>>>
>>>> I haven't updated the docs yet - let's see if the patch is acceptable at
>>>> all first.
>>>
>>> Again, without having reviewed the code, this looks like a feature
>>> we'd want, so please add some docs, and then submit it for the next
>>> commitfest!
>>
>> I've added the docs (see the attachment) and rebased to current head.
>>
>> Tomas
>
> Fixed a failing regression test (check of pg_stat_database structure).

I'm wondering if this (and also my deadlocks stats patch that's int he
queue) should instead of inventing new pgstats messages, add fields to
the tabstat message. It sounds like that one is just for tables, but
it's already the one collecting info about commits and rollbacks, and
it's already sent on every commit.

Adding two fields to that one would add some extra bytes on every
send, but I wonder if that woudl ever affect performance, given the
total size of the packet? And it would certainly be lower overhead in
the cases that there *have* been temp tables used.

Thoughts?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

  8. Re: PATCH: tracking temp files in pg_stat_database

    Tomas Vondra <tv@fuzzy.cz> — 2012-01-21T17:02:01Z

    On 20 Leden 2012, 13:23, Magnus Hagander wrote:
> On Tue, Jan 17, 2012 at 21:39, Tomas Vondra <tv@fuzzy.cz> wrote:
>> On 20.12.2011 19:59, Tomas Vondra wrote:
>>> On 20.12.2011 11:20, Magnus Hagander wrote:
>>>> 2011/12/20 Tomas Vondra <tv@fuzzy.cz>:
>>>>>
>>>>> I haven't updated the docs yet - let's see if the patch is acceptable
>>>>> at
>>>>> all first.
>>>>
>>>> Again, without having reviewed the code, this looks like a feature
>>>> we'd want, so please add some docs, and then submit it for the next
>>>> commitfest!
>>>
>>> I've added the docs (see the attachment) and rebased to current head.
>>>
>>> Tomas
>>
>> Fixed a failing regression test (check of pg_stat_database structure).
>
> I'm wondering if this (and also my deadlocks stats patch that's int he
> queue) should instead of inventing new pgstats messages, add fields to
> the tabstat message. It sounds like that one is just for tables, but
> it's already the one collecting info about commits and rollbacks, and
> it's already sent on every commit.

Hmmm, I'm not against that, but I'd recommend changing the message name to
something that reflects the reality. If it's not just about table
statistics, it should not be named 'tabstats' IMHO. Or maybe split that
into two messages, both sent at the commit time.

I do like the idea of not sending the message for each temp file, though.
One thing that worries me are long running transactions (think about a
batch process that runs for several hours within a single transaction). By
sending the data only at the commit, such transactions would not be
accounted properly. So I'd suggest sending the message either at commit
time or after collecting enough data (increment a counter whenever the
struct is updated and send a message when the counter >= N for a
reasonable value of N, say 20). But maybe it already works that way - I
haven't checked the current 'tabstat' implementation.

> Adding two fields to that one would add some extra bytes on every
> send, but I wonder if that woudl ever affect performance, given the
> total size of the packet? And it would certainly be lower overhead in
> the cases that there *have* been temp tables used.

It's not about temp tables, it's about temp files. Which IMHO implies that
there would be exactly 0.000001% performance difference because temporary
files are quite expensive.

Tomas

  9. Re: PATCH: tracking temp files in pg_stat_database

    Magnus Hagander <magnus@hagander.net> — 2012-01-21T17:13:23Z

    On Sat, Jan 21, 2012 at 18:02, Tomas Vondra <tv@fuzzy.cz> wrote:
> On 20 Leden 2012, 13:23, Magnus Hagander wrote:
>> On Tue, Jan 17, 2012 at 21:39, Tomas Vondra <tv@fuzzy.cz> wrote:
>>> On 20.12.2011 19:59, Tomas Vondra wrote:
>>>> On 20.12.2011 11:20, Magnus Hagander wrote:
>>>>> 2011/12/20 Tomas Vondra <tv@fuzzy.cz>:
>>>>>>
>>>>>> I haven't updated the docs yet - let's see if the patch is acceptable
>>>>>> at
>>>>>> all first.
>>>>>
>>>>> Again, without having reviewed the code, this looks like a feature
>>>>> we'd want, so please add some docs, and then submit it for the next
>>>>> commitfest!
>>>>
>>>> I've added the docs (see the attachment) and rebased to current head.
>>>>
>>>> Tomas
>>>
>>> Fixed a failing regression test (check of pg_stat_database structure).
>>
>> I'm wondering if this (and also my deadlocks stats patch that's int he
>> queue) should instead of inventing new pgstats messages, add fields to
>> the tabstat message. It sounds like that one is just for tables, but
>> it's already the one collecting info about commits and rollbacks, and
>> it's already sent on every commit.
>
> Hmmm, I'm not against that, but I'd recommend changing the message name to
> something that reflects the reality. If it's not just about table
> statistics, it should not be named 'tabstats' IMHO. Or maybe split that
> into two messages, both sent at the commit time.

Yes, renaming it might be a good idea. If you split it into two
messages that would defeat much of the point.

Though I just looked at the tabstat code again, and we already split
that message up at regular intervals. Which would make it quite weird
to have global counters in it as well.

But instead of there, perhaps we need a general "non table, but more
than one type of data" message sent out at the same time. There is
other stuff in the queue for it.

I'm not convinced either way - I'm not against the original way in
your patch either. I just wanted to throw the idea out there, and was
hoping somebody else would have an opinion too :-)


> I do like the idea of not sending the message for each temp file, though.
> One thing that worries me are long running transactions (think about a
> batch process that runs for several hours within a single transaction). By
> sending the data only at the commit, such transactions would not be
> accounted properly. So I'd suggest sending the message either at commit

By that argument they are *already* not accounted properly, because
the "number of rows" and those counters are wrong. By sending the temp
file data in the middle of the transaction, you won't be able to
correlate those numbers with the temp file usage.

I'm not saying the other usecase isn't more common, but whichever way
you do it, it's going to get inconsistent with *something*.


> time or after collecting enough data (increment a counter whenever the
> struct is updated and send a message when the counter >= N for a
> reasonable value of N, say 20). But maybe it already works that way - I
> haven't checked the current 'tabstat' implementation.

No, tabstat is sent at transaction end only.

>
>> Adding two fields to that one would add some extra bytes on every
>> send, but I wonder if that woudl ever affect performance, given the
>> total size of the packet? And it would certainly be lower overhead in
>> the cases that there *have* been temp tables used.
>
> It's not about temp tables, it's about temp files. Which IMHO implies that
> there would be exactly 0.000001% performance difference because temporary
> files are quite expensive.

Bad choice of words, I meant temp files, and was thinking temp files
the whole way :-)


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

  10. Re: PATCH: tracking temp files in pg_stat_database

    Tomas Vondra <tv@fuzzy.cz> — 2012-01-21T19:12:39Z

    On 21 Leden 2012, 18:13, Magnus Hagander wrote:
> On Sat, Jan 21, 2012 at 18:02, Tomas Vondra <tv@fuzzy.cz> wrote:
>> On 20 Leden 2012, 13:23, Magnus Hagander wrote:
>>> I'm wondering if this (and also my deadlocks stats patch that's int he
>>> queue) should instead of inventing new pgstats messages, add fields to
>>> the tabstat message. It sounds like that one is just for tables, but
>>> it's already the one collecting info about commits and rollbacks, and
>>> it's already sent on every commit.
>>
>> Hmmm, I'm not against that, but I'd recommend changing the message name
>> to
>> something that reflects the reality. If it's not just about table
>> statistics, it should not be named 'tabstats' IMHO. Or maybe split that
>> into two messages, both sent at the commit time.
>
> Yes, renaming it might be a good idea. If you split it into two
> messages that would defeat much of the point.

Not really, if combined with the 'send after each N updates or at the
transaction end' because then those parts might be sent independently
(thus not transmitting the whole message).

> Though I just looked at the tabstat code again, and we already split
> that message up at regular intervals. Which would make it quite weird
> to have global counters in it as well.
>
> But instead of there, perhaps we need a general "non table, but more
> than one type of data" message sent out at the same time. There is
> other stuff in the queue for it.
>
> I'm not convinced either way - I'm not against the original way in
> your patch either. I just wanted to throw the idea out there, and was
> hoping somebody else would have an opinion too :-)

Let's make that in a separate patch. It seems like a good thing to do, but
I don't think it should be mixed with this patch.

>> I do like the idea of not sending the message for each temp file,
>> though.
>> One thing that worries me are long running transactions (think about a
>> batch process that runs for several hours within a single transaction).
>> By
>> sending the data only at the commit, such transactions would not be
>> accounted properly. So I'd suggest sending the message either at commit
>
> By that argument they are *already* not accounted properly, because
> the "number of rows" and those counters are wrong. By sending the temp
> file data in the middle of the transaction, you won't be able to
> correlate those numbers with the temp file usage.
>
> I'm not saying the other usecase isn't more common, but whichever way
> you do it, it's going to get inconsistent with *something*.

The question is whether the patch should be modified to send the message
only at commit (i.e. just like tabstats) or if it can remain the way it is
now. And maybe we could change the way all those messages are sent (e.g.
to the regular submits I've proposed in the previous post) to make them
more consistent.

I'm not asking for 100% consistency. What I don't like is a batch process
consuming resources but not reflected in the stats until the final commit.
With a huge spike in the stats right after that, although the activity was
actually spread over several hours.

Tomas

  11. Re: PATCH: tracking temp files in pg_stat_database

    Magnus Hagander <magnus@hagander.net> — 2012-01-26T13:44:17Z

    On Sat, Jan 21, 2012 at 20:12, Tomas Vondra <tv@fuzzy.cz> wrote:
> On 21 Leden 2012, 18:13, Magnus Hagander wrote:
>> On Sat, Jan 21, 2012 at 18:02, Tomas Vondra <tv@fuzzy.cz> wrote:
>> Though I just looked at the tabstat code again, and we already split
>> that message up at regular intervals. Which would make it quite weird
>> to have global counters in it as well.
>>
>> But instead of there, perhaps we need a general "non table, but more
>> than one type of data" message sent out at the same time. There is
>> other stuff in the queue for it.
>>
>> I'm not convinced either way - I'm not against the original way in
>> your patch either. I just wanted to throw the idea out there, and was
>> hoping somebody else would have an opinion too :-)
>
> Let's make that in a separate patch. It seems like a good thing to do, but
> I don't think it should be mixed with this patch.

Yeah, I'm not sure we even want to do that yet, when we go down this
road. We might eventually, of course.


>>> I do like the idea of not sending the message for each temp file,
>>> though.
>>> One thing that worries me are long running transactions (think about a
>>> batch process that runs for several hours within a single transaction).
>>> By
>>> sending the data only at the commit, such transactions would not be
>>> accounted properly. So I'd suggest sending the message either at commit
>>
>> By that argument they are *already* not accounted properly, because
>> the "number of rows" and those counters are wrong. By sending the temp
>> file data in the middle of the transaction, you won't be able to
>> correlate those numbers with the temp file usage.
>>
>> I'm not saying the other usecase isn't more common, but whichever way
>> you do it, it's going to get inconsistent with *something*.
>
> The question is whether the patch should be modified to send the message
> only at commit (i.e. just like tabstats) or if it can remain the way it is
> now. And maybe we could change the way all those messages are sent (e.g.
> to the regular submits I've proposed in the previous post) to make them
> more consistent.
>
> I'm not asking for 100% consistency. What I don't like is a batch process
> consuming resources but not reflected in the stats until the final commit.
> With a huge spike in the stats right after that, although the activity was
> actually spread over several hours.

Yeah, having thought about it some more, I think sending them when
they happen is a better idea. (It's obviously still only going to send
them when the file is closed, but that's as close as we can reasonably
get).

I've cleaned up the patch a bit and applied it - thanks!

Other than cosmetic, I changed the text in the description around a
bit (sol if that's worse now, that's purely my fault) and added docs
to the section about pg_stat_database that you'd forgotten.

Also, I'd like to point you in the direction of the
src/include/catalog/find_unused_oids and duplicate_oids scripts. The
oids you'd picked conflicted with the pg_extension catalog from a year
ago. Easy enough to fix, and there are often conflicts with more
recent oids that the committer has to deal with anyway, but cleaning
those up before submission always helps a little bit. For the next one
:-)

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

  12. Re: PATCH: tracking temp files in pg_stat_database

    Tomas Vondra <tv@fuzzy.cz> — 2012-01-26T13:54:28Z

    On 26 Leden 2012, 14:44, Magnus Hagander wrote:
> On Sat, Jan 21, 2012 at 20:12, Tomas Vondra <tv@fuzzy.cz> wrote:
>> On 21 Leden 2012, 18:13, Magnus Hagander wrote:
>>> On Sat, Jan 21, 2012 at 18:02, Tomas Vondra <tv@fuzzy.cz> wrote:
>>> Though I just looked at the tabstat code again, and we already split
>>> that message up at regular intervals. Which would make it quite weird
>>> to have global counters in it as well.
>>>
>>> But instead of there, perhaps we need a general "non table, but more
>>> than one type of data" message sent out at the same time. There is
>>> other stuff in the queue for it.
>>>
>>> I'm not convinced either way - I'm not against the original way in
>>> your patch either. I just wanted to throw the idea out there, and was
>>> hoping somebody else would have an opinion too :-)
>>
>> Let's make that in a separate patch. It seems like a good thing to do,
>> but
>> I don't think it should be mixed with this patch.
>
> Yeah, I'm not sure we even want to do that yet, when we go down this
> road. We might eventually, of course.

Yes, that's one of the reasons why I suggested to do that in a separate
patch (that might be rejected if we find it's a bad idea).

> I've cleaned up the patch a bit and applied it - thanks!
>
> Other than cosmetic, I changed the text in the description around a
> bit (sol if that's worse now, that's purely my fault) and added docs
> to the section about pg_stat_database that you'd forgotten.

Great. Thanks for the fixes.

> Also, I'd like to point you in the direction of the
> src/include/catalog/find_unused_oids and duplicate_oids scripts. The
> oids you'd picked conflicted with the pg_extension catalog from a year
> ago. Easy enough to fix, and there are often conflicts with more
> recent oids that the committer has to deal with anyway, but cleaning
> those up before submission always helps a little bit. For the next one
> :-)

Aha! I've been wondering how the commiters identify duplicate oids etc.
but I was unaware of those scripts.

Tomas