Re: [HACKERS] New SQL counter statistics view (pg_stat_sql) - Mailing list pgsql-hackers

Hi,


I'm somewhat inclined to think that this really would be better done in
an extension like pg_stat_statements.

> +}
> +
> +/*
> + * 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?