From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Julien Rouhaud <rjuju123@gmail.com>
Cc: Sergei Kornilov <sk@zsrv.org>
Cc: imai.yoshikazu@fujitsu.com <imai.yoshikazu@fujitsu.com>
Cc: legrand legrand <legrand_legrand@hotmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: Planning counters in pg_stat_statements (using pgss_store)
Date: Fri, 27 Mar 2020 20:02:15 +0900
Message-ID: <a2383854-5b94-9bd4-cd1d-d4352991fb15@oss.nttdata.com> (raw)
In-Reply-To: <20200327100001.qarktrnxldmoxzgd@nol>
References: <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>
<bdfee4e0-a304-2498-8da5-3cb52c0a193e@oss.nttdata.com>
<20200325134553.GA14054@nol>
<c20a2e2f-527d-cffd-a2c7-dde2bee23b3f@oss.nttdata.com>
<20200326132242.GA80836@nol>
<20200327100001.qarktrnxldmoxzgd@nol>
On 2020/03/27 19:00, Julien Rouhaud wrote:
> On Thu, Mar 26, 2020 at 02:22:47PM +0100, Julien Rouhaud wrote:
>> On Thu, Mar 26, 2020 at 08:08:35PM +0900, Fujii Masao wrote:
>>>
>>> Here are other comments.
>>>
>>> - if (jstate)
>>> + if (kind == PGSS_JUMBLE)
>>>
>>> Why is PGSS_JUMBLE necessary? ISTM that we can still use jstate here, instead.
>>>
>>> If it's ok to remove PGSS_JUMBLE, we can define PGSS_NUM_KIND(=2) instead
>>> and replace 2 in, e.g., total_time[2] with PGSS_NUM_KIND. Thought?
>>
>> Yes, we could be using jstate here. I originally used that to avoid passing
>> PGSS_EXEC (or the other one) as a way to say "ignore this information as
>> there's the jstate which says it's yet another meaning". If that's not an
>> issue, I can change that as PGSS_NUM_KIND will clearly improve the explicit "2"
>> all over the place.
>
> Done, passing PGSS_PLAN when jumble is intended, with a comment saying that the
> pgss_kind is ignored in that case.
>
>>> + <entry><structfield>total_time</structfield></entry>
>>> + <entry><type>double precision</type></entry>
>>> + <entry></entry>
>>> + <entry>
>>> + Total time spend planning and executing the statement, in milliseconds
>>> + </entry>
>>> + </row>
>>>
>>> pg_stat_statements view has this column but the function not.
>>> We should make both have the column or not at all, for consistency?
>>> I'm not sure if it's good thing to expose the sum of total_plan_time
>>> and total_exec_time as total_time. If some users want that, they can
>>> easily calculate it from total_plan_time and total_exec_time by using
>>> their own logic.
>>
>> I think we originally added it as a way to avoid too much compatibility break,
>> and also because it seems like a field most users will be interested in anyway.
>> Now that I'm thinking about it again, I indeed think it was a mistake to have
>> that in view part only. Not mainly for consistency, but for users who would be
>> interested in the total_time field while not wanting to pay the overhead of
>> retrieving the query text if they don't need it. So I'll change that!
>
> Done
>
>>> + nested_level++;
>>> + PG_TRY();
>>>
>>> In old thread [1], Tom Lane commented the usage of nested_level
>>> in the planner hook. There seems no reply to that so far. What's
>>> your opinion about that comment?
>>>
>>> [1] https://www.postgresql.org/message-id/28980.1515803777@sss.pgh.pa.us
>>
>> Oh thanks, I didn't noticed this part of the discussion. I agree with Tom's
>> concern, and I think that having a specific nesting level variable for the
>> planner is the best workaround, so I'll implement that.
>
> Done.
>
> I also exported BufferUsageAccumDiff as mentioned previously, as it seems
> clearner and will avoid future useless code churn, and run pgindent.
>
> v10 attached.
Thanks for updating the patches!
Regarding 0001 patch, I have one nitpicking comment;
- result = standard_planner(parse, cursorOptions, boundParams);
+ result = standard_planner(parse, query_text, cursorOptions, boundParams);
-standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams)
+standard_planner(Query *parse, const char *querytext, int cursorOptions,
+ ParamListInfo boundParams)
-pg_plan_query(Query *querytree, int cursorOptions, ParamListInfo boundParams)
+pg_plan_query(Query *querytree, const char *query_text, int cursorOptions,
+ ParamListInfo boundParams)
The patch uses "query_text" and "querytext" as the name of newly-added
argument. They should be unified? IMO "query_string" looks better name
because it's used in other functions like pg_analyze_and_rewrite(),
pg_parse_query() for the sake of consistency. Thought?
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: masao.fujii@oss.nttdata.com, rjuju123@gmail.com, sk@zsrv.org, imai.yoshikazu@fujitsu.com, legrand_legrand@hotmail.com
Subject: Re: Planning counters in pg_stat_statements (using pgss_store)
In-Reply-To: <a2383854-5b94-9bd4-cd1d-d4352991fb15@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox