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