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.1515803...@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
