Re: pg_stat_statements: calls under-estimation propagation - Mailing list pgsql-hackers

> I took a quick look. Observations: <br />> <br />> + /* Making query ID dependent on PG version */ <br
/>>+ query->queryId |= PG_VERSION_NUM << 16; <br />> <br />> If you want to do something like this,
makethe value of <br />> PGSS_FILE_HEADER incorporate (PG_VERSION_NUM / 100) or something. <br />> <br />> Why
areyou doing this? <br /><br />The thought was queryid should have a different value for the same <br />query across PG
versions,to ensure that clients using <br />the view,do not assume otherwise. <br /><div class="shrinkable-quote"><br
/>>@@ -128,6 +146,7 @@ typedef struct pgssEntry <br />>   pgssHashKey key; /* hash key of entry - MUST BE FIRST
*/<br />>   Counters counters; /* the statistics for this query */ <br />>   int query_len; /* # of valid bytes
inquery string */ <br />> + uint32 query_id; /* jumble value for this entry */ <br />> <br />> query_id is
alreadyin "key". <br />> <br />> Not sure I like the idea of the new enum at all, but in any case you <br />>
shouldn'thave a PGSS_TUP_LATEST constant - should someone go update <br />> all usage of that constant only when
yourversion isn't the latest? <br />> Like here: <br />> <br />> + if (detected_version >= PGSS_TUP_LATEST)
</div><br/>There is #define PGSS_TUP_LATEST PGSS_TUP_V1_2 <br />So if an update has to be done, this is the one place
todo it. <br /><br />> I forget why Daniel originally altered the min value of <br />> pg_stat_statements.max to
1(I just remember that he did), but I don't <br />> think it holds that you should keep it there. Have you
consideredthe <br />> failure modes when it is actually set to 1? <br />Will set it back to the original value and
alsotest for max value = 1 <br /><div class="shrinkable-quote"><br />> This is what I call a "can't happen" error,
ora defensive one: <br />> <br />> + else <br />> + { <br />> + /* <br />> + * Couldn't identify the
tupleformat.  Raise error. <br />> + * <br />> + * This is an exceptional case that may only happen in bizarre
<br/>> + * situations, since it is thought that every released version <br />> + * of pg_stat_statements has a
matchingschema. <br />> + */ <br />> + ereport(ERROR, <br />> +
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),<br />> + errmsg("pg_stat_statements schema is not supported "
<br/>> + "by its installed binary"))); <br />> + } <br />> <br />> I'll generally make these simple
elogs(),which are more terse. No one <br />> is going to find all that dressing useful. </div> Will convert to using
elogs<br /><br />> Please take a look at this, for future reference: <br />> <br />> <a
href="https://wiki.postgresql.org/wiki/Creating_Clean_Patches"link="external" rel="nofollow"
target="_top">https://wiki.postgresql.org/wiki/Creating_Clean_Patches</a><br/>> <br />> The whitespace changes
aredistracting. <br /><br />Thanks! Still learning the art of clean patch submission. <br /><div
class="shrinkable-quote"><br/>> It probably isn't useful to comment random, unaffected code that isn't <br />>
affectedby your patch - I don't find this new refactoring useful, and <br />> am surprised to see it in your patch:
<br/>> <br />> + /* Check header existence and magic number match. */ <br />>   if (fread(&header,
sizeof(uint32),1, file) != 1 || <br />> - header != PGSS_FILE_HEADER || <br />> - fread(&num, sizeof(int32),
1,file) != 1) <br />> + header != PGSS_FILE_HEADER) <br />> + goto error; <br />> + <br />> + /* Read how
manytable entries there are. */ <br />> + if (fread(&num, sizeof(int32), 1, file) != 1) <br />>   goto error;
<br/>> <br />> Did you mean to add all this, or is it left over from Daniel's patch? </div>I think its a carry
overfrom Daniel's code. I understand the thought. <br />Will keep patch strictly restricted to functionality
implemented<div class="shrinkable-quote"><br />> @@ -43,6 +43,7 @@ <br />>   */ <br />>  #include "postgres.h"
<br/>> <br />> +#include <time.h> <br />>  #include <unistd.h> <br />> <br />>  #include
"access/hash.h"<br />> @@ -59,15 +60,18 @@ <br />>  #include "storage/spin.h" <br />>  #include
"tcop/utility.h"<br />>  #include "utils/builtins.h" <br />> +#include "utils/timestamp.h" <br />> <br />>
Finalthought: I think the order in the pg_stat_statements view is <br />> wrong. It ought to be like a composite
primarykey - (userid, dbid, <br />> query_id). </div>Will make the change. <br />> -- <br />> Peter Geoghegan
<br/><br />Thank you for the review <br />Sameer <br /><br /><hr align="left" width="300" /> View this message in
context:<a
href="http://postgresql.1045698.n5.nabble.com/pg-stat-statements-calls-under-estimation-propagation-tp5738128p5778472.html">Re:
pg_stat_statements:calls under-estimation propagation</a><br /> Sent from the <a
href="http://postgresql.1045698.n5.nabble.com/PostgreSQL-hackers-f1928748.html">PostgreSQL- hackers mailing list
archive</a>at Nabble.com.<br />