RE: Planning counters in pg_stat_statements (using pgss_store)

On Thu, Mar 12, 2020 at 6:31 AM, Julien Rouhaud wrote:
> On Thu, Mar 12, 2020 at 05:28:38AM +0000, imai(dot)yoshikazu(at)fujitsu(dot)com wrote:
> > Hi Julien,
> >
> > On Mon, Mar 9, 2020 at 10:32 AM, Julien Rouhaud wrote:
> > > On Thu, Mar 05, 2020 at 01:26:19PM -0700, legrand legrand wrote:
> > > > Please consider PG13 shortest path ;o)
> > > >
> > > > My one is parse->queryId != UINT64CONST(0) in pgss_planner_hook().
> > > > It fixes IVM problem (verified),
> > > > and keep CTAS equal to pgss without planning counters (verified too).
> > >
> > > I still disagree that hiding this problem is the right fix, but since no one
> > > objected here's a v5 with that behavior. Hopefully this will be fixed in v14.
> >
> > Is there any case that query_text will be NULL when executing pg_plan_query?
>
> With current sources, there are no cases where the query text isn't provided
> AFAICS.
>
> > If query_text will be NULL, we need to add codes to avoid errors in
> > pgss_store like as current patch. If query_text will not be NULL, we should
> > add Assert in pg_plan_query so that other developers can notice that they
> > would not mistakenly set query_text as NULL even without using pgss_planning
> > counter.
>
> I totally agree. I already had such assert locally, and regression tests pass
> without any problem. I'm attaching a v6 with that extra assert. If the
> first patch is committed, it'll now be a requirement to provide it. Or if
> people think it's not, it'll make sure that we'll discuss it.

I see. I also don't come up with any case of query_text is NULL. Now we need
other people's opinion about here.

I'll summary code review of this thread.

[Performance]

If track_planning is not enabled, performance will drop 0.2-0.6% which can be
ignored. If track_planning is enabled, performance will drop 0-2.2%. 2.2% is a
bit large but I think it is still acceptable because people using this feature
might take account that some overhead will happen for additional calling of a
gettime function.

https://www.postgresql.org/message-id/CY4PR20MB12227E5CE199FFBB90C68A13BCB40%40CY4PR20MB1222.namprd20.prod.outlook.com

[Values in each row]

* Rows for planner time are added as {total/min/max/mean/stddev}_plan_time.

These are enough statistics for users who want to investigate the
planning time.

* Rows for executor time are changed from {total/min/max/mean/stddev}_time
to {total/min/max/mean/stddev}_exec_time.

Because of changing the name of the rows, there's no backward compatibility.
Thus some users needs to modify scripts which using previous version of the
pg_stat_statements. I believe it is not expensive to rewrite scripts along
this change and it would be better to give an appropriate name to a row
for future users.
I also haven't seen big opposition about losing backward compatibility so
far.

* We don't provide {total/min/max/mean/stddev}_time.

Users can calculate total_time as total_plan_time + total_exec_time on their
own. About {min/max/mean/stddev}_time, it will not make much sense
because it is not ensured that executor follows planner and each counter
value will be different largely between planner and executor.

* bufusage still only counts the buffer usage during executor.

Now we have the ability to count the buffer usage during planner but we keep
the bufusage count the buffer usage during executor for now.

[Coding]

* We add Assert in pg_plan_query so that query_text will not be NULL when
executing planner.

There's no case query_text will be NULL with current sources. It is not
ensured there will be any case query_text will be possibly NULL in the
future though. Some considerations are needed by other people about this.

I don't have any other comments for now. After looking patches over again and
if there are no other comments about this patch, I'll set this patch as ready
for committer for getting more opinion.

--
Yoshikazu Imai