Re: pg_stat_statements: calls under-estimation propagation - Mailing list pgsql-hackers

On Tue, Nov 5, 2013 at 5:30 AM, Sameer Thakur <samthakur74@gmail.com> wrote:
> Hello,
> Please find attached pg_stat_statements-identification-v9.patch.

I took a quick look. Observations:

+    /* Making query ID dependent on PG version */
+    query->queryId |= PG_VERSION_NUM << 16;

If you want to do something like this, make the value of
PGSS_FILE_HEADER incorporate (PG_VERSION_NUM / 100) or something.

Why are you doing this?

@@ -128,6 +146,7 @@ typedef struct pgssEntry    pgssHashKey key;            /* hash key of entry - MUST BE FIRST */
Counters   counters;        /* the statistics for this query */    int            query_len;        /* # of valid bytes
inquery string */
 
+    uint32        query_id;        /* jumble value for this entry */

query_id is already in "key".

Not sure I like the idea of the new enum at all, but in any case you
shouldn't have a PGSS_TUP_LATEST constant - should someone go update
all usage of that constant only when your version isn't the latest?
Like here:

+        if (detected_version >= PGSS_TUP_LATEST)

I forget why Daniel originally altered the min value of
pg_stat_statements.max to 1 (I just remember that he did), but I don't
think it holds that you should keep it there. Have you considered the
failure modes when it is actually set to 1?

This is what I call a "can't happen" error, or a defensive one:

+    else
+    {
+        /*
+         * Couldn't identify the tuple format.  Raise error.
+         *
+         * This is an exceptional case that may only happen in bizarre
+         * situations, since it is thought that every released version
+         * of pg_stat_statements has a matching schema.
+         */
+        ereport(ERROR,
+                (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                 errmsg("pg_stat_statements schema is not supported "
+                        "by its installed binary")));
+    }

I'll generally make these simple elogs(), which are more terse. No one
is going to find all that dressing useful.

Please take a look at this, for future reference:

https://wiki.postgresql.org/wiki/Creating_Clean_Patches

The whitespace changes are distracting.

It probably isn't useful to comment random, unaffected code that isn't
affected by your patch - I don't find this new refactoring useful, and
am surprised to see it in your patch:

+    /* Check header existence and magic number match. */    if (fread(&header, sizeof(uint32), 1, file) != 1 ||
-        header != PGSS_FILE_HEADER ||
-        fread(&num, sizeof(int32), 1, file) != 1)
+        header != PGSS_FILE_HEADER)
+        goto error;
+
+    /* Read how many table entries there are. */
+    if (fread(&num, sizeof(int32), 1, file) != 1)        goto error;

Did you mean to add all this, or is it left over from Daniel's patch?

@@ -43,6 +43,7 @@ */#include "postgres.h"

+#include <time.h>#include <unistd.h>
#include "access/hash.h"
@@ -59,15 +60,18 @@#include "storage/spin.h"#include "tcop/utility.h"#include "utils/builtins.h"
+#include "utils/timestamp.h"

Final thought: I think the order in the pg_stat_statements view is
wrong. It ought to be like a composite primary key - (userid, dbid,
query_id).

-- 
Peter Geoghegan