Thread: PATCH: tracking temp files in pg_stat_database
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
Attachment
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/
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
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/
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
Attachment
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
Attachment
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/
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
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/
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
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/
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