Re: pg_stat_statements: calls under-estimation propagation

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 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