Thread: Patch to send transaction commit/rollback stats to the stats collector unconditionally.

Gurjeet Singh <gurjeet@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

Gurjeet Singh wrote:
> On Wed, Mar 19, 2014 at 4:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> > Gurjeet Singh <gurjeet@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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre@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

On Wed, Mar 19, 2014 at 7:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alvaro Herrera <alvherre@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

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 <gurjeet@singh.im> wrote:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Alvaro Herrera <alvherre@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 Company

Kevin Grittner <kgrittn@ymail.com> writes:
> Gurjeet Singh <gurjeet@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

Tom Lane <tgl@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

On Sun, Jun 29, 2014 at 11:58 AM, Kevin Grittner <kgrittn@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. Although the discussion
is a bit old, but I'm sure I would've tested the patch before
submitting.

> I have attached a suggested patch which I think
> would work.  Gurjeet, could you take a look at it?

The patch, when considered together with Tom's suggestion upthread,
looks good to me.

Best regards,
-- 
Gurjeet Singh http://gurjeet.singh.im/

EDB : www.EnterpriseDB.com : The Enterprise PostgreSQL Company

On Tue, Jul 1, 2014 at 10:05 AM, Kevin Grittner <kgrittn@ymail.com> wrote:
> Tom Lane <tgl@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.

Attached is the patch to save you a few key strokes :)

> 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?

Considering this as a bug-fix, I'd vote for it to be applied to all
supported releases. But since this may cause unforeseen performance
penalty, I think it should be applied only as far back as the
introduction of PGSTAT_STAT_INTERVAL throttle.

The throttle was implemeted in 641912b, which AFAICT was part of 8.3.
So I guess all the supported releases it is.

Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/

EDB : www.EnterpriseDB.com :  The Enterprise PostgreSQL Company

On Tue, Jul 1, 2014 at 7:32 PM, Gurjeet Singh <gurjeet@singh.im> wrote:
> On Tue, Jul 1, 2014 at 10:05 AM, Kevin Grittner <kgrittn@ymail.com> wrote:
>> Tom Lane <tgl@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.
>
> Attached is the patch to save you a few key strokes :)
>
>> 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?
>
> Considering this as a bug-fix, I'd vote for it to be applied to all
> supported releases. But since this may cause unforeseen performance
> penalty, I think it should be applied only as far back as the
> introduction of PGSTAT_STAT_INTERVAL throttle.
>
> The throttle was implemeted in 641912b, which AFAICT was part of 8.3.
> So I guess all the supported releases it is.

I'll vote for master-only.  I view this as a behavior change, and it
isn't nice to spring those on people in minor releases.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Jul 1, 2014 at 7:32 PM, Gurjeet Singh <gurjeet@singh.im> wrote:

>> Considering this as a bug-fix, I'd vote for it to be applied to
>> all supported releases.

I initially considered that position, but balanced that against
this:

> I view this as a behavior change, and it isn't nice to spring
> those on people in minor releases.

I tend to be very conservative on what should go into a minor
release.  In this case the user impact is pretty minimal --
distortion of numbers in monitoring software.  That doesn't strike
me as a serious enough problem to risk even a trivial behavior
change.

> I'll vote for master-only.

Well, it seems tame enough to me to apply to the release still in
beta testing.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Jul 2, 2014 at 11:45 AM, Kevin Grittner <kgrittn@ymail.com> wrote:
>> I'll vote for master-only.
>
> Well, it seems tame enough to me to apply to the release still in
> beta testing.

It didn't seem important enough to me to justify a beta-to-release
behavior change, but I won't complain too much if you feel otherwise.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Jul 2, 2014 at 11:45 AM, Kevin Grittner <kgrittn@ymail.com> wrote:
>>> I'll vote for master-only.

>> Well, it seems tame enough to me to apply to the release still in
>> beta testing.

> It didn't seem important enough to me to justify a beta-to-release
> behavior change, but I won't complain too much if you feel otherwise.

FWIW, master + 9.4 seems like a reasonable compromise to me too.
        regards, tom lane

In preparing to push the patch, I noticed I hadn't responded to this:

Gurjeet Singh <gurjeet@singh.im> wrote:
> Kevin Grittner <kgrittn@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(®ular_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

Tom Lane <tgl@sss.pgh.pa.us> wrote:


> FWIW, master + 9.4 seems like a reasonable compromise to me too.

Pushed to those branches.  Marked in CF.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Jul 2, 2014 at 3:49 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
> In preparing to push the patch, I noticed I hadn't responded to this:
>
> Gurjeet Singh <gurjeet@singh.im> wrote:
>> Kevin Grittner <kgrittn@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(®ular_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.
    /*
-     * 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(®ular_msg);    if
(shared_msg.m_nentries> 0)        pgstat_send_tabstat(&shared_msg);
 

Best regards,
-- 
Gurjeet Singh http://gurjeet.singh.im/

EDB : www.EnterpriseDB.com : The Enterprise PostgreSQL Company

Gurjeet Singh <gurjeet@singh.im> wrote:
> On Wed, Jul 2, 2014 at 3:49 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
>> Gurjeet Singh <gurjeet@singh.im> wrote:
>>> Kevin Grittner <kgrittn@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(®ular_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