Re: New SQL counter statistics view (pg_stat_sql)
On Tue, Mar 03, 2020 at 11:50:04PM -0300, Alvaro Herrera wrote: > So, who is updating this patch for the new cmdtaglist.h stuff? This patch had no activity for months, so I am marking it as returned with feedback. -- Michael signature.asc Description: PGP signature
Re: New SQL counter statistics view (pg_stat_sql)
> On Mar 3, 2020, at 6:50 PM, Alvaro Herrera wrote: > > On 2019-Nov-13, Smith, Peter wrote: > >> From: Thomas Munro Sent: Monday, 4 November 2019 >> 1:43 PM >> >>> No comment on the patch but I noticed that the documentation changes don't >>> build. Please make sure you can "make docs" successfully, having installed >>> the documentation tools[1]. >> >> Thanks for the feedback. An updated patch which fixes the docs issue is >> attached. > > So, who is updating this patch for the new cmdtaglist.h stuff? I have a much altered (competing?) patch inspired by pg_stat_sql. I am working to integrate the changes you made to my commandtag patch into my commandstats patch before posting it. It might be good if somebody else tackled the pg_stat_sql patch in case my version is rejected. I don't want to highjack this thread to talk in detail about the other patch, so I'll just give an overview that might be useful for anybody thinking about putting effort into committing this patch first. The motivation for the commandtag patch that you've committed (thanks again!) was to make the commandstats patch possible. I didn't like the way pg_stat_sql stored the tags as strings, but I also still don't like the way it needs to take a lock every time a backend increments the counter for a command. That is bad, I think, because the lock overhead for an otherwise fast command (like moving a cursor) may be high enough to discourage users from turning this feature on in production. I solve that by trading off speed and memory. For most commandtags, there is a single uint64 counter, and incrementing the counter requires taking a lock. This is very similar to how the other patch works. For the few commandtags that we deem worthy of the special treatment, there is an additional uint32 counter *per backend* reserved in shared memory. Backends update this counter using atomics, but do not require locks. When the counter is in danger of overflow, locks are taken to roll the count into the uint64 counters and zero out the backend-specific uint32 counter. I have roughly 20 command tags receiving this special treatment, but the design of the patch doesn't make it too hard to change the exact list of tags worthy of it, so I don't plan to stress too much about the exact set of tags. I'm sure you'll all tell me which ones you think do or do not deserve the treatment. If it isn't obvious, the logic of having this treatment for only a subset of tags is that 192 tags * 4 bytes per counter * MaxBackends gets expensive for folks running a high number of backends. Taking that from 192 tags down to one or two dozen seems worthwhile. I expect to post on a separate thread before the day is over. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: New SQL counter statistics view (pg_stat_sql)
On 2019-Nov-13, Smith, Peter wrote: > From: Thomas Munro Sent: Monday, 4 November 2019 > 1:43 PM > > > No comment on the patch but I noticed that the documentation changes don't > > build. Please make sure you can "make docs" successfully, having installed > > the documentation tools[1]. > > Thanks for the feedback. An updated patch which fixes the docs issue is > attached. So, who is updating this patch for the new cmdtaglist.h stuff? -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
RE: New SQL counter statistics view (pg_stat_sql)
From: Thomas Munro Sent: Monday, 4 November 2019 1:43 PM > No comment on the patch but I noticed that the documentation changes don't > build. Please make sure you can "make docs" successfully, having installed > the documentation tools[1]. Thanks for the feedback. An updated patch which fixes the docs issue is attached. Kind Regards. Peter Smith --- Fujitsu Australia 0001-pg_stat_sql.patch Description: 0001-pg_stat_sql.patch
Re: New SQL counter statistics view (pg_stat_sql)
On Thu, Oct 17, 2019 at 3:22 PM Smith, Peter wrote: > We have resurrected this 2 year old "SQL statements statistics counter" > proposal from Hari. > > The attached patch addresses the previous review issues. Hi, No comment on the patch but I noticed that the documentation changes don't build. Please make sure you can "make docs" successfully, having installed the documentation tools[1]. [1] https://www.postgresql.org/docs/devel/docguide-toolsets.html
RE: New SQL counter statistics view (pg_stat_sql)
Dear Hackers,
We have resurrected this 2 year old "SQL statements statistics counter"
proposal from Hari.
The attached patch addresses the previous review issues.
The commit-fest entry has been moved forward to the next commit-fest
https://commitfest.postgresql.org/25/790/
Please review again, and consider if this is OK for "Ready for Committer"
status.
Kind Regards
--
Peter Smith
Fujitsu Australia
-Original Message-
From: pgsql-hackers-ow...@postgresql.org
On Behalf Of Andres Freund
Sent: Thursday, 6 April 2017 5:18 AM
To: Haribabu Kommi
Cc: Michael Paquier ; Dilip Kumar
; vinayak ;
pgsql-hack...@postgresql.org
Subject: Re: New SQL counter statistics view (pg_stat_sql)
Hi,
I'm somewhat inclined to think that this really would be better done in an
extension like pg_stat_statements.
On 2017-03-08 14:39:23 +1100, Haribabu Kommi wrote:
> On Wed, Feb 1, 2017 at 3:13 PM, Michael Paquier
>
> +
> + track_sql (boolean)
> +
> + track_sql configuration parameter
> +
> +
> +
> +
> +Enables collection of different SQL statement statistics that are
> +executed on the instance. This parameter is off by default. Only
> +superusers can change this setting.
> +
> +
> +
> +
I don't like this name much, seems a bit too generic to me. 'track_activities',
'track_io_timings' are at least a bit clearer.
How about track_statement_statistics + corresponding view/function renaming?
> +
> + pg_stat_sql View
> +
> +
> +
> + Column
> + Type
> + Description
> +
> +
> +
> +
> +
> + tag
> + text
> + Name of the SQL statement
> +
It's not really the "name" of a statement. Internally and IIRC elsewhere in the
docs we describe something like this as tag?
> +/* --
> + * pgstat_send_sqlstmt(void)
> + *
> + * Send SQL statement statistics to the collector
> + * --
> + */
> +static void
> +pgstat_send_sqlstmt(void)
> +{
> + PgStat_MsgSqlstmt msg;
> + PgStat_SqlstmtEntry *entry;
> + HASH_SEQ_STATUS hstat;
> +
> + if (pgstat_backend_sql == NULL)
> + return;
> +
> + pgstat_setheader(_hdr, PGSTAT_MTYPE_SQLSTMT);
> + msg.m_nentries = 0;
> +
> + hash_seq_init(, pgstat_backend_sql);
> + while ((entry = (PgStat_SqlstmtEntry *) hash_seq_search()) !=
> NULL)
> + {
> + PgStat_SqlstmtEntry *m_ent;
> +
> + /* Skip it if no counts accumulated since last time */
> + if (entry->count == 0)
> + continue;
> +
> + /* need to convert format of time accumulators */
> + m_ent = _entry[msg.m_nentries];
> + memcpy(m_ent, entry, sizeof(PgStat_SqlstmtEntry));
> +
> + if (++msg.m_nentries >= PGSTAT_NUM_SQLSTMTS)
> + {
> + pgstat_send(, offsetof(PgStat_MsgSqlstmt,
> m_entry[0]) +
> + msg.m_nentries *
> sizeof(PgStat_SqlstmtEntry));
> + msg.m_nentries = 0;
> + }
> +
> + /* reset the entry's count */
> + entry->count = 0;
> + }
> +
> + if (msg.m_nentries > 0)
> + pgstat_send(, offsetof(PgStat_MsgSqlstmt, m_entry[0]) +
> + msg.m_nentries *
> sizeof(PgStat_SqlstmtEntry));
Hm. So pgstat_backend_sql is never deleted, which'll mean that if a backend
lives long we'll continually iterate over a lot of 0 entries. I think
performance evaluation of this feature should take that into account.
> +}
> +
> +/*
> + * Count SQL statement for pg_stat_sql view */ void
> +pgstat_count_sqlstmt(const char *commandTag) {
> + PgStat_SqlstmtEntry *htabent;
> + boolfound;
> +
> + if (!pgstat_backend_sql)
> + {
> + /* First time through - initialize SQL statement stat table */
> + HASHCTL hash_ctl;
> +
> + memset(_ctl, 0, sizeof(hash_ctl));
> + hash_ctl.keysize = NAMEDATALEN;
> + hash_ctl.entrysize = sizeof(PgStat_SqlstmtEntry);
> + pgstat_backend_sql = hash_create("SQL statement stat entries",
> +
> PGSTAT_SQLSTMT_HASH_SIZE,
> +
> _ctl,
> +
> HASH_ELEM | HASH_BLOBS);
> + }
> +
> + /* Get the stats entry for this SQL sta
