Re: [HACKERS] Patch to send transaction commit/rollback stats to the stats collector unconditionally.

2014-07-02 Thread Kevin Grittner
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.

2014-07-02 Thread Kevin Grittner
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.

2014-07-01 Thread Kevin Grittner
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.

2014-06-29 Thread Kevin Grittner
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.

2014-06-29 Thread Tom Lane
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


Re: [HACKERS] Patch to send transaction commit/rollback stats to the stats collector unconditionally.

2014-03-19 Thread Tom Lane
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.

2014-03-19 Thread Gurjeet Singh
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.

2014-03-19 Thread Alvaro Herrera
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.

2014-03-19 Thread Tom Lane
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.

2014-03-19 Thread Robert Haas
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.

2014-03-19 Thread Gurjeet Singh
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