Re: Planning counters in pg_stat_statements (using pgss_store)

On 2020/03/21 4:30, Julien Rouhaud wrote:
> On Fri, Mar 20, 2020 at 05:09:05PM +0300, Sergei Kornilov wrote:
>> Hello
>>
>> Yet another is missed in docs: total_time
> 
> Oh good catch!  I rechecked many time the field, and totally missed that the
> documentation is referring to the view, which has an additional column, and not
> the function.  Attached v9 fixes that.

Thanks for the patch! Here are the review comments from me.

-    PGSS_V1_3
+    PGSS_V1_3,
+    PGSS_V1_8

WAL usage patch [1] increments this version to 1_4 instead of 1_8.
I *guess* that's because previously this version was maintained
independently from pg_stat_statements' version. For example,
pg_stat_statements 1.4 seems to have used PGSS_V1_3.

+    /*
+     * We can't process the query if no query_text is provided, as pgss_store
+     * needs it.  We also ignore query without queryid, as it would be treated
+     * as a utility statement, which may not be the case.
+     */

Could you tell me why the planning stats are not tracked when executing
utility statements? In some utility statements like REFRESH MATERIALIZED VIEW,
the planner would work.

+static BufferUsage
+compute_buffer_counters(BufferUsage start, BufferUsage stop)
+{
+    BufferUsage result;

BufferUsageAccumDiff() has very similar logic. Isn't it better to expose
and use that function rather than creating new similar function?

          values[i++] = Int64GetDatumFast(tmp.rows);
          values[i++] = Int64GetDatumFast(tmp.shared_blks_hit);
          values[i++] = Int64GetDatumFast(tmp.shared_blks_read);

Previously (without the patch) pg_stat_statements_1_3() reported
the buffer usage counters updated only in execution phase. But,
in the patched version, pg_stat_statements_1_3() reports the total
of buffer usage counters updated in both planning and execution
phases. Is this OK? I'm not sure how seriously we should ensure
the backward compatibility for pg_stat_statements....

+/* contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql */

ISTM it's good timing to have also pg_stat_statements--1.8.sql since
the definition of pg_stat_statements() is changed. Thought?

[1]
https://postgr.es/m/CAB-hujrP8ZfUkvL5OYETipQwA=e3n7oqHFU=4ZLxWS_Cza3kQQ@mail.gmail.com

Regards,

-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters