Re: Planning counters in pg_stat_statements (using pgss_store)

On 2020/03/31 16:33, Julien Rouhaud wrote:
> 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(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.
>
>
> Yes of course. It also requires some changes in the macro to make it safe when
> called with an expression.
>
> v12 attached!

Thanks for updating the patch! The patch looks good to me.

I applied minor and cosmetic changes into the patch. Attached is
the updated version of the patch. Barring any objection, I'd like to
commit this version.

BTW, the minor and cosmetic changes that I applied are, for example,

- Rename pgss_planner_hook to pgss_planner for the sake of consistency.
Other function using hook in pgss doesn't use "_hook" in their names, too.
- Make pgss_planner use PG_FINALLY() instead of PG_CATCH().
- Make PGSS_NUMKIND as the last value in enum pgssStoreKind.
- Update the sample output in the document.
etc

Regards,

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