Re: [HACKERS] PATCH: tracking temp files in pg_stat_database

2012-01-26 Thread Magnus Hagander 
On Sat, Jan 21, 2012 at 20:12, Tomas Vondra t...@fuzzy.cz wrote:
 On 21 Leden 2012, 18:13, Magnus Hagander wrote:
 On Sat, Jan 21, 2012 at 18:02, Tomas Vondra t...@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/

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: tracking temp files in pg_stat_database

2012-01-26 Thread Tomas Vondra 
On 26 Leden 2012, 14:44, Magnus Hagander wrote:
 On Sat, Jan 21, 2012 at 20:12, Tomas Vondra t...@fuzzy.cz wrote:
 On 21 Leden 2012, 18:13, Magnus Hagander wrote:
 On Sat, Jan 21, 2012 at 18:02, Tomas Vondra t...@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



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: tracking temp files in pg_stat_database

2012-01-21 Thread Tomas Vondra 
On 20 Leden 2012, 13:23, Magnus Hagander wrote:
 On Tue, Jan 17, 2012 at 21:39, Tomas Vondra t...@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 t...@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.01% performance difference because temporary
files are quite expensive.

Tomas


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: tracking temp files in pg_stat_database

2012-01-21 Thread Magnus Hagander 
On Sat, Jan 21, 2012 at 18:02, Tomas Vondra t...@fuzzy.cz wrote:
 On 20 Leden 2012, 13:23, Magnus Hagander wrote:
 On Tue, Jan 17, 2012 at 21:39, Tomas Vondra t...@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 t...@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.01% 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/

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: tracking temp files in pg_stat_database

2012-01-21 Thread Tomas Vondra 
On 21 Leden 2012, 18:13, Magnus Hagander wrote:
 On Sat, Jan 21, 2012 at 18:02, Tomas Vondra t...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: tracking temp files in pg_stat_database

2012-01-20 Thread Magnus Hagander 
On Tue, Jan 17, 2012 at 21:39, Tomas Vondra t...@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 t...@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/

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: tracking temp files in pg_stat_database

2012-01-17 Thread Tomas Vondra 
On 20.12.2011 19:59, Tomas Vondra wrote:
 On 20.12.2011 11:20, Magnus Hagander wrote:
 2011/12/20 Tomas Vondra t...@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
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index a12a9a2..3635c3f 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -691,6 +691,26 @@ postgres: replaceableuser/ replaceabledatabase/ 
replaceablehost/ re
  /row
 
  row
+  
entryliteralfunctionpg_stat_get_db_temp_files/function(typeoid/type)/literal/entry
+  entrytypebigint/type/entry
+  entry
+   Nuber of temporary files written for the database. All temporary files 
are
+   counted, regardless of why the temporary file was created (sorting or 
hash
+   join) or file size (log_temp_file does not affect this).
+  /entry
+ /row
+
+ row
+  
entryliteralfunctionpg_stat_get_db_temp_bytes/function(typeoid/type)/literal/entry
+  entrytypebigint/type/entry
+  entry
+   Amount of data written to temporary files for the database. All 
temporary
+   files are counted, regardless of why the temporary file was created 
(sorting
+   or hash join) or file size (log_temp_file does not affect this).
+  /entry
+ /row
+
+ row
   
entryliteralfunctionpg_stat_get_numscans/function(typeoid/type)/literal/entry
   entrytypebigint/type/entry
   entry
diff --git a/src/backend/catalog/system_views.sql 
b/src/backend/catalog/system_views.sql
index 50ba20c..3a0dca3 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -574,6 +574,8 @@ CREATE VIEW pg_stat_database AS
 pg_stat_get_db_tuples_updated(D.oid) AS tup_updated,
 pg_stat_get_db_tuples_deleted(D.oid) AS tup_deleted,
 pg_stat_get_db_conflict_all(D.oid) AS conflicts,
+pg_stat_get_db_temp_files(D.oid) AS temp_files,
+pg_stat_get_db_temp_bytes(D.oid) AS temp_bytes,
 pg_stat_get_db_stat_reset_time(D.oid) AS stats_reset
 FROM pg_database D;
 
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 323d42b..6a2ff1d 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -286,7 +286,7 @@ static void pgstat_recv_bgwriter(PgStat_MsgBgWriter *msg, 
int len);
 static void pgstat_recv_funcstat(PgStat_MsgFuncstat *msg, int len);
 static void pgstat_recv_funcpurge(PgStat_MsgFuncpurge *msg, int len);
 static void pgstat_recv_recoveryconflict(PgStat_MsgRecoveryConflict *msg, int 
len);
-
+static void pgstat_recv_tempfile(PgStat_MsgTempFile *msg, int len);
 
 /* 
  * Public functions called from postmaster follow
@@ -1339,6 +1339,29 @@ pgstat_report_recovery_conflict(int reason)
pgstat_send(msg, sizeof(msg));
 }
 
+
+/* 
+ * pgstat_report_tempfile() -
+ *
+ * Tell the collector about a temporary file.
+ * 
+ */
+void
+pgstat_report_tempfile(size_t filesize)
+{
+   PgStat_MsgTempFile msg;
+
+   if (pgStatSock == PGINVALID_SOCKET || !pgstat_track_counts)
+   return;
+
+   pgstat_setheader(msg.m_hdr, PGSTAT_MTYPE_TEMPFILE);
+   msg.m_databaseid = MyDatabaseId;
+   msg.m_filesize = filesize;
+   pgstat_send(msg, sizeof(msg));
+}
+
+;
+
 /* --
  * pgstat_ping() -
  *
@@ -3185,6 +3208,10 @@ PgstatCollectorMain(int argc, char *argv[])

pgstat_recv_recoveryconflict((PgStat_MsgRecoveryConflict *) msg, len);
break;
 
+   case PGSTAT_MTYPE_TEMPFILE:
+   
pgstat_recv_tempfile((PgStat_MsgTempFile *) msg, len);
+   break;
+
default:
break;
}
@@ -3266,6 +3293,8 @@ pgstat_get_db_entry(Oid databaseid, bool create)
result-n_conflict_snapshot = 0;
result-n_conflict_bufferpin = 0;
result-n_conflict_startup_deadlock = 0;
+   result-n_temp_files = 0;
+   result-n_temp_bytes = 0;
 
result-stat_reset_timestamp = GetCurrentTimestamp();
 
@@ -4177,6 +4206,8 @@ pgstat_recv_resetcounter(PgStat_MsgResetcounter *msg, int 
len)
dbentry-n_tuples_updated = 0;
dbentry-n_tuples_deleted = 0;
dbentry-last_autovac_time = 0;
+   dbentry-n_temp_bytes = 0;
+   dbentry-n_temp_files = 0;
 
dbentry-stat_reset_timestamp =

Re: [HACKERS] PATCH: tracking temp files in pg_stat_database

2011-12-20 Thread Tomas Vondra 
On 20.12.2011 11:20, Magnus Hagander wrote:
 2011/12/20 Tomas Vondra t...@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
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index a12a9a2..3635c3f 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -691,6 +691,26 @@ postgres: replaceableuser/ replaceabledatabase/ 
replaceablehost/ re
  /row
 
  row
+  
entryliteralfunctionpg_stat_get_db_temp_files/function(typeoid/type)/literal/entry
+  entrytypebigint/type/entry
+  entry
+   Nuber of temporary files written for the database. All temporary files 
are
+   counted, regardless of why the temporary file was created (sorting or 
hash
+   join) or file size (log_temp_file does not affect this).
+  /entry
+ /row
+
+ row
+  
entryliteralfunctionpg_stat_get_db_temp_bytes/function(typeoid/type)/literal/entry
+  entrytypebigint/type/entry
+  entry
+   Amount of data written to temporary files for the database. All 
temporary
+   files are counted, regardless of why the temporary file was created 
(sorting
+   or hash join) or file size (log_temp_file does not affect this).
+  /entry
+ /row
+
+ row
   
entryliteralfunctionpg_stat_get_numscans/function(typeoid/type)/literal/entry
   entrytypebigint/type/entry
   entry
diff --git a/src/backend/catalog/system_views.sql 
b/src/backend/catalog/system_views.sql
index 2253ca8..55d20dc 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -574,6 +574,8 @@ CREATE VIEW pg_stat_database AS
 pg_stat_get_db_tuples_updated(D.oid) AS tup_updated,
 pg_stat_get_db_tuples_deleted(D.oid) AS tup_deleted,
 pg_stat_get_db_conflict_all(D.oid) AS conflicts,
+pg_stat_get_db_temp_files(D.oid) AS temp_files,
+pg_stat_get_db_temp_bytes(D.oid) AS temp_bytes,
 pg_stat_get_db_stat_reset_time(D.oid) AS stats_reset
 FROM pg_database D;
 
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 24f4cde..97c7004 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -286,7 +286,7 @@ static void pgstat_recv_bgwriter(PgStat_MsgBgWriter *msg, 
int len);
 static void pgstat_recv_funcstat(PgStat_MsgFuncstat *msg, int len);
 static void pgstat_recv_funcpurge(PgStat_MsgFuncpurge *msg, int len);
 static void pgstat_recv_recoveryconflict(PgStat_MsgRecoveryConflict *msg, int 
len);
-
+static void pgstat_recv_tempfile(PgStat_MsgTempFile *msg, int len);
 
 /* 
  * Public functions called from postmaster follow
@@ -1339,6 +1339,29 @@ pgstat_report_recovery_conflict(int reason)
pgstat_send(msg, sizeof(msg));
 }
 
+
+/* 
+ * pgstat_report_tempfile() -
+ *
+ * Tell the collector about a temporary file.
+ * 
+ */
+void
+pgstat_report_tempfile(size_t filesize)
+{
+   PgStat_MsgTempFile msg;
+
+   if (pgStatSock == PGINVALID_SOCKET || !pgstat_track_counts)
+   return;
+
+   pgstat_setheader(msg.m_hdr, PGSTAT_MTYPE_TEMPFILE);
+   msg.m_databaseid = MyDatabaseId;
+   msg.m_filesize = filesize;
+   pgstat_send(msg, sizeof(msg));
+}
+
+;
+
 /* --
  * pgstat_ping() -
  *
@@ -3185,6 +3208,10 @@ PgstatCollectorMain(int argc, char *argv[])

pgstat_recv_recoveryconflict((PgStat_MsgRecoveryConflict *) msg, len);
break;
 
+   case PGSTAT_MTYPE_TEMPFILE:
+   
pgstat_recv_tempfile((PgStat_MsgTempFile *) msg, len);
+   break;
+
default:
break;
}
@@ -3266,6 +3293,8 @@ pgstat_get_db_entry(Oid databaseid, bool create)
result-n_conflict_snapshot = 0;
result-n_conflict_bufferpin = 0;
result-n_conflict_startup_deadlock = 0;
+   result-n_temp_files = 0;
+   result-n_temp_bytes = 0;
 
result-stat_reset_timestamp = GetCurrentTimestamp();
 
@@ -4177,6 +4206,8 @@ pgstat_recv_resetcounter(PgStat_MsgResetcounter *msg, int 
len)
dbentry-n_tuples_updated = 0;
dbentry-n_tuples_deleted = 0;
dbentry-last_autovac_time = 0;
+   dbentry-n_temp_bytes = 0;
+   dbentry-n_temp_files = 0;
 
dbentry-stat_reset_timestamp = GetCurrentTimestamp();
 
@@ -4403,6 +4434,24 @@ pgstat_recv_recoveryconflict(PgStat_MsgRecoveryConflict 
*msg, int len)
 }
 
 /*

Re: [HACKERS] PATCH: tracking temp files in pg_stat_database

2011-12-20 Thread Magnus Hagander 
2011/12/20 Tomas Vondra t...@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/

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: tracking temp files in pg_stat_database

2011-12-20 Thread Tomas Vondra 
On 20 Prosinec 2011, 11:20, Magnus Hagander wrote:
 2011/12/20 Tomas Vondra t...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: tracking temp files in pg_stat_database

2011-12-20 Thread Magnus Hagander 
On Tue, Dec 20, 2011 at 11:45, Tomas Vondra t...@fuzzy.cz wrote:
 On 20 Prosinec 2011, 11:20, Magnus Hagander wrote:
 2011/12/20 Tomas Vondra t...@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/

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] PATCH: tracking temp files in pg_stat_database

2011-12-19 Thread Tomas Vondra 
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
diff --git a/src/backend/catalog/system_views.sql 
b/src/backend/catalog/system_views.sql
index 2253ca8..55d20dc 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -574,6 +574,8 @@ CREATE VIEW pg_stat_database AS
 pg_stat_get_db_tuples_updated(D.oid) AS tup_updated,
 pg_stat_get_db_tuples_deleted(D.oid) AS tup_deleted,
 pg_stat_get_db_conflict_all(D.oid) AS conflicts,
+pg_stat_get_db_temp_files(D.oid) AS temp_files,
+pg_stat_get_db_temp_bytes(D.oid) AS temp_bytes,
 pg_stat_get_db_stat_reset_time(D.oid) AS stats_reset
 FROM pg_database D;
 
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 24f4cde..97c7004 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -286,7 +286,7 @@ static void pgstat_recv_bgwriter(PgStat_MsgBgWriter *msg, 
int len);
 static void pgstat_recv_funcstat(PgStat_MsgFuncstat *msg, int len);
 static void pgstat_recv_funcpurge(PgStat_MsgFuncpurge *msg, int len);
 static void pgstat_recv_recoveryconflict(PgStat_MsgRecoveryConflict *msg, int 
len);
-
+static void pgstat_recv_tempfile(PgStat_MsgTempFile *msg, int len);
 
 /* 
  * Public functions called from postmaster follow
@@ -1339,6 +1339,29 @@ pgstat_report_recovery_conflict(int reason)
pgstat_send(msg, sizeof(msg));
 }
 
+
+/* 
+ * pgstat_report_tempfile() -
+ *
+ * Tell the collector about a temporary file.
+ * 
+ */
+void
+pgstat_report_tempfile(size_t filesize)
+{
+   PgStat_MsgTempFile msg;
+
+   if (pgStatSock == PGINVALID_SOCKET || !pgstat_track_counts)
+   return;
+
+   pgstat_setheader(msg.m_hdr, PGSTAT_MTYPE_TEMPFILE);
+   msg.m_databaseid = MyDatabaseId;
+   msg.m_filesize = filesize;
+   pgstat_send(msg, sizeof(msg));
+}
+
+;
+
 /* --
  * pgstat_ping() -
  *
@@ -3185,6 +3208,10 @@ PgstatCollectorMain(int argc, char *argv[])

pgstat_recv_recoveryconflict((PgStat_MsgRecoveryConflict *) msg, len);
break;
 
+   case PGSTAT_MTYPE_TEMPFILE:
+   
pgstat_recv_tempfile((PgStat_MsgTempFile *) msg, len);
+   break;
+
default:
break;
}
@@ -3266,6 +3293,8 @@ pgstat_get_db_entry(Oid databaseid, bool create)
result-n_conflict_snapshot = 0;
result-n_conflict_bufferpin = 0;
result-n_conflict_startup_deadlock = 0;
+   result-n_temp_files = 0;
+   result-n_temp_bytes = 0;
 
result-stat_reset_timestamp = GetCurrentTimestamp();
 
@@ -4177,6 +4206,8 @@ pgstat_recv_resetcounter(PgStat_MsgResetcounter *msg, int 
len)
dbentry-n_tuples_updated = 0;
dbentry-n_tuples_deleted = 0;
dbentry-last_autovac_time = 0;
+   dbentry-n_temp_bytes = 0;
+   dbentry-n_temp_files = 0;
 
dbentry-stat_reset_timestamp = GetCurrentTimestamp();
 
@@ -4403,6 +4434,24 @@ pgstat_recv_recoveryconflict(PgStat_MsgRecoveryConflict 
*msg, int len)
 }
 
 /* --
+ * pgstat_recv_tempfile() -
+ *
+ * Process as PGSTAT_MTYPE_TEMPFILE message.
+ * --
+ */
+static void
+pgstat_recv_tempfile(PgStat_MsgTempFile *msg, int len)
+{
+   PgStat_StatDBEntry *dbentry;
+
+   dbentry = pgstat_get_db_entry(msg-m_databaseid, true);
+
+   dbentry-n_temp_bytes += msg-m_filesize;
+   dbentry-n_temp_files += 1;
+
+}
+
+/* --
  * pgstat_recv_funcstat() -
  *
  * Count what the backend has done.
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 9540279..5e40d12 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@