Re: Planning counters in pg_stat_statements (using pgss_store)

On Tue, Mar 31, 2020 at 04:10:47PM +0900, Fujii Masao wrote:
> 
> 
> On 2020/03/31 15:03, Julien Rouhaud wrote:
> > On Tue, Mar 31, 2020 at 12:21:43PM +0900, Fujii Masao wrote:
> > > 
> > > On 2020/03/31 3:16, Julien Rouhaud wrote:
> > > > On Mon, Mar 30, 2020 at 6:36 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> > > > > 
> > > > > While testing the patched pgss, I found that the patched version
> > > > > may track the statements that the original version doesn't.
> > > > > Please imagine the case where the following queries are executed,
> > > > > with pgss.track = top.
> > > > > 
> > > > >        PREPARE hoge AS SELECT * FROM t;
> > > > >        EXPLAIN EXECUTE hoge;
> > > > > 
> > > > > The pgss view returned "PREPARE hoge AS SELECT * FROM t"
> > > > > in the patched version, but not in the orignal version.
> > > > > 
> > > > > Is this problematic?
> > > > 
> > > > Oh indeed. That's a side effect of having different the executed query
> > > > and the planned query being different.
> > > > 
> > > > I guess the question is to chose if the top level executed query of a
> > > > utilty statement containing an optimisable query, should the top level
> > > > planner call of that optimisable statement be considered at top level
> > > > or not.  I tend to think that's the correct behavior here, as this is
> > > > also what would happen if a regular DML was provided.  What do you
> > > > think?
> > > 
> > > TBH, not sure if that's ok yet...
> > > 
> > > I'm now just wondering if both plan_nested_level and
> > > exec_nested_level should be incremented in pgss_ProcessUtility().
> > > This is just a guess, so I need more investigation about this.
> > 
> > Yeah, after a second thought I realize that my comparison was wrong.  Allowing
> > *any* top-level planner call when pgss.track = top would mean that we should
> > also consider all planner calls from queries executed for FK checks and such,
> > which is definitely not the intended behavior.
> 
> Yes. So, basically any planner activity that happens during
> the execution phase of the statement is not tracked.
> 
> > FTR with this patch such calls still don't get tracked, but only because those
> > query don't get a queryid assigned, not because the nesting level says so.
> > 
> > How about simply passing (plan_nested_level + exec_nested_level) for
> > pgss_enabled call in pgss_planner_hook?
> 
> Looks good to me! The comment about why this treatment is necessary only in
> pgss_planner() should be added.


Yes of course.  It also requires some changes in the macro to make it safe when
called with an expression.

v12 attached!