Re: [HACKERS] Patch to send transaction commit/rollback stats to the stats collector unconditionally.
In preparing to push the patch, I noticed I hadn't responded to this: Gurjeet Singh gurj...@singh.im wrote: Kevin Grittner kgri...@ymail.com wrote: I have reviewed this patch, and think we should do what the patch is trying to do, but I don't think the submitted patch would actually work. Just curious, why do you think it won't work. Because you didn't touch this part of the function: /* * Send partial messages. If force is true, make sure that any pending * xact commit/abort gets counted, even if no table stats to send. */ if (regular_msg.m_nentries 0 || (force (pgStatXactCommit 0 || pgStatXactRollback 0))) pgstat_send_tabstat(regular_msg); The statistics would not actually be sent unless a table had been accessed or it was forced because the connection was closing. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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 to send transaction commit/rollback stats to the stats collector unconditionally.
Gurjeet Singh gurj...@singh.im wrote: On Wed, Jul 2, 2014 at 3:49 PM, Kevin Grittner kgri...@ymail.com wrote: Gurjeet Singh gurj...@singh.im wrote: Kevin Grittner kgri...@ymail.com wrote: I have reviewed this patch, and think we should do what the patch is trying to do, but I don't think the submitted patch would actually work. Just curious, why do you think it won't work. Because you didn't touch this part of the function: /* * Send partial messages. If force is true, make sure that any pending * xact commit/abort gets counted, even if no table stats to send. */ if (regular_msg.m_nentries 0 || (force (pgStatXactCommit 0 || pgStatXactRollback 0))) pgstat_send_tabstat(regular_msg); The statistics would not actually be sent unless a table had been accessed or it was forced because the connection was closing. I sure did! In fact that was the one and only line of code that was changed. It effectively bypassed the 'force' check if there were any transaction stats to report. Sorry, I had too many versions of things sitting around and looked at the wrong one. It was the test at the top that you might not get past to be able to report things below: /* Don't expend a clock check if nothing to do */ if ((pgStatTabList == NULL || pgStatTabList-tsa_used == 0) !have_function_stats !force) return; The function stats might have gotten you past it for the particular cases you were testing, but the problem could be caused without that. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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 to send transaction commit/rollback stats to the stats collector unconditionally.
Tom Lane t...@sss.pgh.pa.us wrote: If we're going to do it like this, then I think the force flag should be considered to do nothing except override the clock check, which probably means it shouldn't be tested in the initial if() at all. That makes sense, and is easily done. The only question left is how far back to take the patch. I'm inclined to only apply it to master and 9.4. Does anyone think otherwise? -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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 to send transaction commit/rollback stats to the stats collector unconditionally.
I have reviewed this patch, and think we should do what the patch is trying to do, but I don't think the submitted patch would actually work. I have attached a suggested patch which I think would work. Gurjeet, could you take a look at it? My comments on prior discussion embedded below. Gurjeet Singh gurj...@singh.im wrote: Tom Lane t...@sss.pgh.pa.us wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: I'm not sure I understand the point of this whole thing. Realistically, how many transactions are there that do not access any database tables? I think that something like select * from pg_stat_activity might not bump any table-access counters, once the relevant syscache entries had gotten loaded. You could imagine that a monitoring app would do a long series of those and nothing else (whether any actually do or not is a different question). +1. This is exactly what I was doing; querying pg_stat_database from a psql session, to track transaction counters. +1 A monitoring application might very well do exactly that. Having the history graphs show large spikes might waste someone's time tracking down the cause. (In fact, that seems to be exactly what happened to Gurjeet, prompting this patch~.) I've been in similar situations -- enough to appreciate how user-unfriendly such behavior is. But still, it's a bit hard to credit that this patch is solving any real problem. Where's the user complaints about the existing behavior? Consider my original email a user complaint, albeit with a patch attached. I was trying to ascertain TPS on a customer's instance when I noticed this behaviour; it violated POLA. Had I not decided to dig through the code to find the source of this behaviour, as a regular user I would've reported this anomaly as a bug, or maybe turned a blind eye to it labeling it as a quirky behaviour. ... or assumed that it was real transaction load during that interval. There have probably been many bewildered users who thought they missed some activity storm from their own software, and run around trying to identify the source -- never realizing it was a measurement anomaly caused by surprising behavior of the statistics gathering. That is, even granting that anybody has a workload that acts like this, why would they care ... To avoid surprises! This behaviour, at least in my case, causes Postgres to misreport the very thing I am trying to calculate. Yup. What's the point of reporting these numbers if we're going to allow that kind of distortion? and are they prepared to take a performance hit to avoid the counter jump after the monitoring app exits? It doesn't look like we know how much of a performance hit this will cause. I don't see any reasons cited in the code, either. Could this be a case of premature optimization. The commit log shows that, during the overhaul (commit 77947c5), this check was put in place to emulate what the previous code did; code before that commit reported stats, including transaction stats, only if there were any regular or shared table stats to report. More than that, this only causes new activity for a connection which, within a single PGSTAT_STAT_INTERVAL, ran queries -- but none of the queries run accessed any tables. That should be a pretty small number of connections, since there only special-purpose connections (e.g., monitoring) are likely to do that. And when it does happen, the new activity is just the same as what any connection which does access a table would generate. There's nothing special or more intense about this. And an idle connection won't generate any new activity. This concern seem like much ado about nothing (or about as close to nothing as you can get). That said, if you *did* actually have a workload where you were using the database engine as a calculator, running such queries at volume on a lot of connections, wouldn't you want the statistics to represent that accurately? Besides, there's already a throttle built in using the PGSTAT_STAT_INTERVAL limit. This is a key point; neither the original patch nor my proposed alternative bypasses that throttling. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Companydiff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index 3ab1428..6e6f7fe 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -753,6 +753,7 @@ pgstat_report_stat(bool force) /* Don't expend a clock check if nothing to do */ if ((pgStatTabList == NULL || pgStatTabList-tsa_used == 0) + pgStatXactCommit == 0 pgStatXactRollback == 0 !have_function_stats !force) return; @@ -817,11 +818,11 @@ pgstat_report_stat(bool force) } /* - * Send partial messages. If force is true, make sure that any pending - * xact commit/abort gets counted, even if no table stats to send. + * Send partial messages. Make sure that any pending xact commit/abort + * gets counted, even
Re: [HACKERS] Patch to send transaction commit/rollback stats to the stats collector unconditionally.
Kevin Grittner kgri...@ymail.com writes: Gurjeet Singh gurj...@singh.im wrote: Besides, there's already a throttle built in using the PGSTAT_STAT_INTERVAL limit. This is a key point; neither the original patch nor my proposed alternative bypasses that throttling. That's a fair point that probably obviates my objection. I think the interval throttling is more recent than the code in question. If we're going to do it like this, then I think the force flag should be considered to do nothing except override the clock check, which probably means it shouldn't be tested in the initial if() at all. regards, tom lane -- 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 to send transaction commit/rollback stats to the stats collector unconditionally.
Please find attached the patch to send transaction commit/rollback stats to stats collector unconditionally. Without this patch, the transactions that do not read from/write to a database table do not get reported to the stats collector until the client disconnects. Hence the transaction counts in pg_stat_database do not increment gradually as one would expect them to. But when such a backend disconnects, the counts jump dramatically, giving the impression that the database processed a lot of transactions (potentially thousands) in an instant. Best regards, -- Gurjeet Singh http://gurjeet.singh.im/ EDB www.EnterpriseDB.com http://www.enterprisedb.com commit 500d11f96b21552872bad4e9655bd45db28efabd Author: Gurjeet Singh gurj...@singh.im Date: Wed Mar 19 13:41:55 2014 -0400 Send transaction stats to the collector even if no table stats to report. diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index 3dc280a..47008ed 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -825,11 +825,11 @@ pgstat_report_stat(bool force) } /* - * Send partial messages. If force is true, make sure that any pending - * xact commit/abort gets counted, even if no table stats to send. + * Send partial messages. Make sure that any pending xact commit/abort gets + * counted, even if no table stats to send. */ if (regular_msg.m_nentries 0 || - (force (pgStatXactCommit 0 || pgStatXactRollback 0))) + force || (pgStatXactCommit 0 || pgStatXactRollback 0)) pgstat_send_tabstat(regular_msg); if (shared_msg.m_nentries 0) pgstat_send_tabstat(shared_msg); -- 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 to send transaction commit/rollback stats to the stats collector unconditionally.
Gurjeet Singh gurj...@singh.im writes: Please find attached the patch to send transaction commit/rollback stats to stats collector unconditionally. That's intentional to reduce stats traffic. What kind of performance penalty does this patch impose? If the number of such transactions is large enough to create a noticeable jump in the counters, I would think that this would be a pretty darn expensive fix. regards, tom lane -- 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 to send transaction commit/rollback stats to the stats collector unconditionally.
On Wed, Mar 19, 2014 at 4:22 PM, Tom Lane t...@sss.pgh.pa.us wrote: Gurjeet Singh gurj...@singh.im writes: Please find attached the patch to send transaction commit/rollback stats to stats collector unconditionally. That's intentional to reduce stats traffic. What kind of performance penalty does this patch impose? If the number of such transactions is large enough to create a noticeable jump in the counters, I would think that this would be a pretty darn expensive fix. I can't speak to the performance impact of this patch, except that it would depend on the fraction of transactions that behave this way. Perhaps the people who develop and/or aggressively use monitoring can pitch in. Presumably, on heavily used systems these transactions would form a small fraction. On relatively idle systems these transactions may be a larger fraction but that wouldn't affect the users since the database is not under stress anyway. Best regards, -- Gurjeet Singh http://gurjeet.singh.im/ EDB www.EnterpriseDB.com http://www.enterprisedb.com
Re: [HACKERS] Patch to send transaction commit/rollback stats to the stats collector unconditionally.
Gurjeet Singh wrote: On Wed, Mar 19, 2014 at 4:22 PM, Tom Lane t...@sss.pgh.pa.us wrote: Gurjeet Singh gurj...@singh.im writes: Please find attached the patch to send transaction commit/rollback stats to stats collector unconditionally. That's intentional to reduce stats traffic. What kind of performance penalty does this patch impose? If the number of such transactions is large enough to create a noticeable jump in the counters, I would think that this would be a pretty darn expensive fix. Presumably, on heavily used systems these transactions would form a small fraction. On relatively idle systems these transactions may be a larger fraction but that wouldn't affect the users since the database is not under stress anyway. I'm not sure I understand the point of this whole thing. Realistically, how many transactions are there that do not access any database tables? If an application doesn't want to access stored data, why would it connect to the database in the first place? (I imagine you could use it to generate random numbers and such ...) -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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 to send transaction commit/rollback stats to the stats collector unconditionally.
Alvaro Herrera alvhe...@2ndquadrant.com writes: I'm not sure I understand the point of this whole thing. Realistically, how many transactions are there that do not access any database tables? I think that something like select * from pg_stat_activity might not bump any table-access counters, once the relevant syscache entries had gotten loaded. You could imagine that a monitoring app would do a long series of those and nothing else (whether any actually do or not is a different question). But still, it's a bit hard to credit that this patch is solving any real problem. Where's the user complaints about the existing behavior? That is, even granting that anybody has a workload that acts like this, why would they care ... and are they prepared to take a performance hit to avoid the counter jump after the monitoring app exits? regards, tom lane -- 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 to send transaction commit/rollback stats to the stats collector unconditionally.
On Wed, Mar 19, 2014 at 7:27 PM, Tom Lane t...@sss.pgh.pa.us wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: I'm not sure I understand the point of this whole thing. Realistically, how many transactions are there that do not access any database tables? I think that something like select * from pg_stat_activity might not bump any table-access counters, once the relevant syscache entries had gotten loaded. You could imagine that a monitoring app would do a long series of those and nothing else (whether any actually do or not is a different question). But still, it's a bit hard to credit that this patch is solving any real problem. Where's the user complaints about the existing behavior? That is, even granting that anybody has a workload that acts like this, why would they care ... and are they prepared to take a performance hit to avoid the counter jump after the monitoring app exits? Well, EnterpriseDB has a monitoring product called Postgres Enterprise Manager (PEM) that sits around and does stuff like periodically reading statistics views. I think you can probably configure it to read from regular tables too, but it's hardly insane that someone would have a long-running monitoring connection that only reads statistics and monitoring views. (This is not a vote for or against the patch, which I have not read. It's just an observation.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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 to send transaction commit/rollback stats to the stats collector unconditionally.
On Wed, Mar 19, 2014 at 7:27 PM, Tom Lane t...@sss.pgh.pa.us wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: I'm not sure I understand the point of this whole thing. Realistically, how many transactions are there that do not access any database tables? I think that something like select * from pg_stat_activity might not bump any table-access counters, once the relevant syscache entries had gotten loaded. You could imagine that a monitoring app would do a long series of those and nothing else (whether any actually do or not is a different question). +1. This is exactly what I was doing; querying pg_stat_database from a psql session, to track transaction counters. But still, it's a bit hard to credit that this patch is solving any real problem. Where's the user complaints about the existing behavior? Consider my original email a user complaint, albeit with a patch attached. I was trying to ascertain TPS on a customer's instance when I noticed this behaviour; it violated POLA. Had I not decided to dig through the code to find the source of this behaviour, as a regular user I would've reported this anomaly as a bug, or maybe turned a blind eye to it labeling it as a quirky behaviour. That is, even granting that anybody has a workload that acts like this, why would they care ... To avoid surprises! This behaviour, at least in my case, causes Postgres to misreport the very thing I am trying to calculate. and are they prepared to take a performance hit to avoid the counter jump after the monitoring app exits? It doesn't look like we know how much of a performance hit this will cause. I don't see any reasons cited in the code, either. Could this be a case of premature optimization. The commit log shows that, during the overhaul (commit 77947c5), this check was put in place to emulate what the previous code did; code before that commit reported stats, including transaction stats, only if there were any regular or shared table stats to report. Besides, there's already a throttle built in using the PGSTAT_STAT_INTERVAL limit. Best regards, -- Gurjeet Singh http://gurjeet.singh.im/ EDB www.EnterpriseDB.com http://www.enterprisedb.com
