Re: pg_stat_statements: calls under-estimation propagation

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

The thought was queryid should have a different value for the same
query across PG versions, to ensure that clients using
the view,do not assume otherwise.

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

There is #define PGSS_TUP_LATEST PGSS_TUP_V1_2
So if an update has to be done, this is the one place to do it.

> 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?
Will set it back to the original value and also test for max value = 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.
Will convert to using elogs

> Please take a look at this, for future reference:
>
> https://wiki.postgresql.org/wiki/Creating_Clean_Patches
>
> The whitespace changes are distracting.

Thanks! Still learning the art of clean patch submission.

> 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?
I think its a carry over from Daniel's code. I understand the thought.
Will keep patch strictly restricted to functionality implemented
> @@ -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).
Will make the change.
> --
> Peter Geoghegan

Thank you for the review
Sameer

--
View this message in context: http://postgresql.1045698.n5.nabble.com/pg-stat-statements-calls-under-estimation-propagation-tp5738128p5778472.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.