Subject: Re: Planning counters in pg_stat_statements (using pgss_store)
To: Julien Rouhaud <rjuju123@gmail.com>, Sergei Kornilov <sk@zsrv.org>
Cc: "imai.yoshikazu@fujitsu.com" <imai.yoshikazu@fujitsu.com>,
 legrand legrand <legrand_legrand@hotmail.com>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
References: 
 <OSBPR01MB461661BDBBF8C5CB477AE47494FD0@OSBPR01MB4616.jpnprd01.prod.outlook.com>
 <CAOBaU_Y1M7TFHDWV1wkQpES3qsK-BatsizPjxur1BMBoVE095Q@mail.gmail.com>
 <CAFMSG9HJQr=H8doWJOp=wqyKbVqxMLkk_Qu2KfpmkKvS-Xn7qQ@mail.gmail.com>
 <CAOBaU_YekXNsQ819w=eBTM_aQ+BYPcVWLX0sDO-5xRNhqipbRA@mail.gmail.com>
 <OSAPR01MB46099E7D03547A2A7292716494FA0@OSAPR01MB4609.jpnprd01.prod.outlook.com>
 <1584180240397-0.post@n3.nabble.com> <20200314172733.mg7qpyumlyythm25@nol>
 <TY2PR01MB46172D9531F2D3CE4C036C1A94F90@TY2PR01MB4617.jpnprd01.prod.outlook.com>
 <20200316214912.iakenhp7vyd37hmg@nol>
 <6300601584711975@vla4-87a00c2d2b1b.qloud-c.yandex.net>
 <20200320193004.rqgf3iim4fugq3sm@nol>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <bdfee4e0-a304-2498-8da5-3cb52c0a193e@oss.nttdata.com>
Date: Wed, 25 Mar 2020 22:09:37 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0)
 Gecko/20100101 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <20200320193004.rqgf3iim4fugq3sm@nol>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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