Re: pg_stat_statements: calls under-estimation propagation

On Tue, Nov 5, 2013 at 5:30 AM, Sameer Thakur <samthakur74(at)gmail(dot)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