On Thu, Oct 10, 2013 at 7:20 PM, Sameer Thakur <samthaku...@gmail.com> wrote: > Please find patch attached which adds documentation for session_start > and introduced fields and corrects documentation for queryid to be > query_id. session_start remains in the view as agreed.
Thanks for updating the document! I'm not clear about the use case of 'session_start' and 'introduced' yet. Could you elaborate it? Maybe we should add that explanation into the document? In my test, I found that pg_stat_statements.total_time always indicates a zero. I guess that the patch might handle pg_stat_statements.total_time wrongly. + values[i++] = DatumGetTimestamp( + instr_get_timestamptz(pgss->session_start)); + values[i++] = DatumGetTimestamp( + instr_get_timestamptz(entry->introduced)); These should be executed only when detected_version >= PGSS_TUP_LATEST? + <entry><structfield>session_start</structfield></entry> + <entry><type>text</type></entry> + <entry></entry> + <entry>Start time of a statistics session.</entry> + </row> + + <row> + <entry><structfield>introduced</structfield></entry> + <entry><type>text</type></entry> The data type of these columns is not text. + instr_time session_start; + uint64 private_stat_session_key; it's better to add the comments explaining these fields. + microsec = INSTR_TIME_GET_MICROSEC(t) - + ((POSTGRES_EPOCH_JDATE - UNIX_EPOCH_JDATE) * USECS_PER_DAY); HAVE_INT64_TIMESTAMP should be considered here. + if (log_cannot_read) + ereport(LOG, + (errcode_for_file_access(), + errmsg("could not read pg_stat_statement file \"%s\": %m", + PGSS_DUMP_FILE))); Is it better to use WARNING instead of LOG as the log level here? + /* + * The role calling this function is unable to see + * sensitive aspects of this tuple. + * + * Nullify everything except the "insufficient privilege" + * message for this entry + */ + memset(nulls, 1, sizeof nulls); + + nulls[i] = 0; + values[i] = CStringGetTextDatum("<insufficient privilege>"); Why do we need to hide *all* the fields in pg_stat_statements, when it's accessed by improper user? This is a big change of pg_stat_statements behavior, and it might break the compatibility. >> >This is not directly related to the patch itself, but why does the >> > queryid >> >need to be calculated based on also the "statistics session"? > > If we expose hash value of query tree, without using statistics session, > it is possible that users might make wrong assumption that this value > remains stable across version upgrades, when in reality it depends on > whether the version has make changes to query tree internals. So to > explicitly ensure that users do not make this wrong assumption, hash value > generation use statistics session id, which is newly created under > situations described above. So, ISTM that we can use, for example, the version number to calculate the query_id. Right? Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers