Re: Planning counters in pg_stat_statements (using pgss_store)

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(dot)fujii(at)oss(dot)nttdata(dot)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.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters