Re: New SQL counter statistics view (pg_stat_sql) - Mailing list pgsql-hackers
From | Andres Freund |
---|---|
Subject | Re: New SQL counter statistics view (pg_stat_sql) |
Date | |
Msg-id | 20170405191757.4q23duppyl6cywk7@alap3.anarazel.de Whole thread Raw |
In response to | Re: [HACKERS] New SQL counter statistics view (pg_stat_sql) (Haribabu Kommi <kommi.haribabu@gmail.com>) |
Responses |
RE: New SQL counter statistics view (pg_stat_sql)
|
List | pgsql-hackers |
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.paquier@gmail.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
pgsql-hackers by date: