On Tue, Nov 5, 2013 at 5:30 AM, Sameer Thakur <samthaku...@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 in 
query 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


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

Reply via email to