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-owner(at)postgresql(dot)org <pgsql-hackers-owner(at)postgresql(dot)org> On Behalf Of Andres Freund
Sent: Thursday, 6 April 2017 5:18 AM
To: Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
Cc: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>; Dilip Kumar <dilipbalaut(at)gmail(dot)com>; vinayak <Pokale_Vinayak_q3(at)lab(dot)ntt(dot)co(dot)jp>; pgsql-hackers(at)postgresql(dot)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
> <michael(dot)paquier(at)gmail(dot)com>

> + <varlistentry id="guc-track-sql" xreflabel="track_sql">
> + <term><varname>track_sql</varname> (<type>boolean</type>)
> + <indexterm>
> + <primary><varname>track_sql</> configuration parameter</primary>
> + </indexterm>
> + </term>
> + <listitem>
> + <para>
> + 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.
> + </para>
> + </listitem>
> + </varlistentry>
> +

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?

> + <table id="pg-stat-sql-view" xreflabel="pg_stat_sql">
> + <title><structname>pg_stat_sql</structname> View</title>
> + <tgroup cols="3">
> + <thead>
> + <row>
> + <entry>Column</entry>
> + <entry>Type</entry>
> + <entry>Description</entry>
> + </row>
> + </thead>
> +
> + <tbody>
> + <row>
> + <entry><structfield>tag</></entry>
> + <entry><type>text</></entry>
> + <entry>Name of the SQL statement</entry>
> + </row>

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(&msg.m_hdr, PGSTAT_MTYPE_SQLSTMT);
> + msg.m_nentries = 0;
> +
> + hash_seq_init(&hstat, pgstat_backend_sql);
> + while ((entry = (PgStat_SqlstmtEntry *) hash_seq_search(&hstat)) != 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 = &msg.m_entry[msg.m_nentries];
> + memcpy(m_ent, entry, sizeof(PgStat_SqlstmtEntry));
> +
> + if (++msg.m_nentries >= PGSTAT_NUM_SQLSTMTS)
> + {
> + pgstat_send(&msg, 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(&msg, 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;
> + bool found;
> +
> + if (!pgstat_backend_sql)
> + {
> + /* First time through - initialize SQL statement stat table */
> + HASHCTL hash_ctl;
> +
> + memset(&hash_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,
> + &hash_ctl,
> + HASH_ELEM | HASH_BLOBS);
> + }
> +
> + /* Get the stats entry for this SQL statement, create if necessary */
> + htabent = hash_search(pgstat_backend_sql, commandTag,
> + HASH_ENTER, &found);
> + if (!found)
> + htabent->count = 1;
> + else
> + htabent->count++;
> +}

That's not really something in this patch, but a lot of this would be better if we didn't internally have command tags as strings, but as an enum. We should really only convert to a string with needed. That we're doing string comparisons on Portal->commandTag is just plain bad.

If so, we could also make this whole patch a lot cheaper - have a fixed size array that has an entry for every possible tag (possibly even in shared memory, and then use atomics there).

> +++ b/src/backend/tcop/postgres.c
> @@ -1109,6 +1109,12 @@ exec_simple_query(const char *query_string)
>
> PortalDrop(portal, false);
>
> + /*
> + * Count SQL statement for pg_stat_sql view
> + */
> + if (pgstat_track_sql)
> + pgstat_count_sqlstmt(commandTag);
> +

Isn't doing this at the message level quite wrong? What about statements executed from functions and such? Shouldn't this integrate at the executor level instead?

> if (IsA(parsetree->stmt, TransactionStmt))
> {
> /*
> @@ -1991,6 +1997,29 @@ exec_execute_message(const char *portal_name,
> long max_rows)
>
> (*receiver->rDestroy) (receiver);
>
> + /*
> + * Count SQL Statement for pgx_stat_sql
> + */
> + if (pgstat_track_sql)
> + {
> + CachedPlanSource *psrc = NULL;
> +
> + if (portal->prepStmtName)
> + {
> + PreparedStatement *pstmt;
> +
> + pstmt = FetchPreparedStatement(portal->prepStmtName, false);
> + if (pstmt)
> + psrc = pstmt->plansource;
> + }
> + else
> + psrc = unnamed_stmt_psrc;
> +
> + /* psrc should not be NULL here */
> + if (psrc && psrc->commandTag && !execute_is_fetch && pgstat_track_sql)
> + pgstat_count_sqlstmt(psrc->commandTag);

Wait, we're re-fetching the statement here? That doesn't sound alright.

> +Datum
> +pg_stat_sql(PG_FUNCTION_ARGS)
> +{
> + TupleDesc tupdesc;
> + Datum values[2];
> + bool nulls[2];
> + ReturnSetInfo *rsi;
> + MemoryContext old_cxt;
> + Tuplestorestate *tuple_store;
> + PgStat_SqlstmtEntry *sqlstmtentry;
> + HASH_SEQ_STATUS hstat;
> +
> + /* Function call to let the backend read the stats file */
> + pgstat_fetch_global();
> +
> + /* Initialize values and nulls arrays */
> + MemSet(values, 0, sizeof(values));
> + MemSet(nulls, 0, sizeof(nulls));

why not instead declare values[2] = {0}? Obviously not important.

> + tuple_store =
> + tuplestore_begin_heap(rsi->allowedModes & SFRM_Materialize_Random,
> + false, work_mem);

Huh, why do we need random?

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org) To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers