Re: Planning counters in pg_stat_statements (using pgss_store)
От | Julien Rouhaud |
---|---|
Тема | Re: Planning counters in pg_stat_statements (using pgss_store) |
Дата | |
Msg-id | 20200327100001.qarktrnxldmoxzgd@nol обсуждение исходный текст |
Ответ на | Re: Planning counters in pg_stat_statements (using pgss_store) (Julien Rouhaud <rjuju123@gmail.com>) |
Ответы |
Re: Planning counters in pg_stat_statements (using pgss_store)
Re: Planning counters in pg_stat_statements (using pgss_store) |
Список | pgsql-hackers |
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.
Вложения
В списке pgsql-hackers по дате отправления: