Re: [HACKERS] Re: Patch to send transaction commit/rollback stats to the stats collector unconditionally.
On Tue, Jul 1, 2014 at 7:32 PM, Gurjeet Singh gurj...@singh.im wrote: On Tue, Jul 1, 2014 at 10:05 AM, Kevin Grittner kgri...@ymail.com wrote: 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. 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 -- 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] Re: Patch to send transaction commit/rollback stats to the stats collector unconditionally.
Robert Haas robertmh...@gmail.com wrote: On Tue, Jul 1, 2014 at 7:32 PM, Gurjeet Singh gurj...@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 -- 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] Re: Patch to send transaction commit/rollback stats to the stats collector unconditionally.
On Wed, Jul 2, 2014 at 11:45 AM, Kevin Grittner kgri...@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 -- 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] Re: Patch to send transaction commit/rollback stats to the stats collector unconditionally.
Robert Haas robertmh...@gmail.com writes: On Wed, Jul 2, 2014 at 11:45 AM, Kevin Grittner kgri...@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 -- 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] Re: Patch to send transaction commit/rollback stats to the stats collector unconditionally.
Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: Patch to send transaction commit/rollback stats to the stats collector unconditionally.
On Wed, Jul 2, 2014 at 3:49 PM, Kevin Grittner kgri...@ymail.com wrote: 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. 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(regular_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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: Patch to send transaction commit/rollback stats to the stats collector unconditionally.
On Sun, Jun 29, 2014 at 11:58 AM, 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. 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: Patch to send transaction commit/rollback stats to the stats collector unconditionally.
On Tue, Jul 1, 2014 at 10:05 AM, Kevin Grittner kgri...@ymail.com wrote: 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. 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 diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index 3ab1428..c7f41a5 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -753,7 +753,8 @@ pgstat_report_stat(bool force) /* Don't expend a clock check if nothing to do */ if ((pgStatTabList == NULL || pgStatTabList-tsa_used == 0) - !have_function_stats !force) + pgStatXactCommit == 0 pgStatXactRollback == 0 + !have_function_stats) 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 if there are no table stats to send. */ if (regular_msg.m_nentries 0 || - (force (pgStatXactCommit 0 || pgStatXactRollback 0))) + 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
