Re: Query Jumbling for CALL and SET utility statements

Hi,

On 9/21/22 6:07 PM, Fujii Masao wrote:
> 
> 
> On 2022/09/19 15:29, Drouvot, Bertrand wrote:
>> Please find attached v6 taking care of the remarks mentioned above.
> 
> Thanks for updating the patch!
> 
> +SET pg_stat_statements.track_utility = TRUE;
> +
> +-- PL/pgSQL procedure and pg_stat_statements.track = all
> +-- we drop and recreate the procedures to avoid any caching funnies
> +SET pg_stat_statements.track_utility = FALSE;
> 
> Could you tell me why track_utility is enabled just before it's disabled?

Thanks for looking at the new version!

No real reason, I removed those useless SET in the new V7 attached.

> 
> Could you tell me what actually happens if we don't drop and
> recreate the procedures? I'd like to know what "any caching funnies" are.

Without the drop/recreate the procedure body does not appear normalized 
(while the CALL itself is) when switching from track = top to track = all.

I copy-pasted this comment from the already existing "function" section 
in the pg_stat_statements.sql file. This comment has been introduced for 
the function section in commit 83f2061dd0. Note that the behavior would 
be the same for the function case (function body does not appear 
normalized without the drop/recreate).

> 
> +SELECT pg_stat_statements_reset();
> +CALL MINUS_TWO(3);
> +CALL MINUS_TWO(7);
> +CALL SUM_TWO(3, 8);
> +CALL SUM_TWO(7, 5);
> +
> +SELECT query, calls, rows FROM pg_stat_statements ORDER BY query 
> COLLATE "C";
> 
> This test set for the procedures is executed with the following
> four conditions, respectively. Do we really need all of these tests?
> 
> track = top, track_utility = true
> track = top, track_utility = false
> track = all, track_utility = true
> track = all, track_utility = false

Oh right, the track_utility = false cases have been added when we 
decided up-thread to force track CALL.

But now that's probably not needed to test with track_utility = true. So 
I'm just keeping track_utility = off with track = top or all in the new 
V7 attached (like this is the case for the "function" section).

> 
> +begin;
> +prepare transaction 'tx1';
> +insert into test_tx values (1);
> +commit prepared 'tx1';
> 
> The test set of 2PC commands is also executed with track_utility = on
> and off, respectively. But why do we need to run that test when
> track_utility = off?

That's useless, thanks for pointing out. Removed in V7 attached.

> 
> -    if (query->utilityStmt)
> +    if (query->utilityStmt && !jstate)
>       {
>           if (pgss_track_utility && 
> !PGSS_HANDLED_UTILITY(query->utilityStmt))
> 
> "pgss_track_utility" should be
> "pgss_track_utility || FORCE_TRACK_UTILITY(parsetree)" theoretically?

Good catch! That's not needed (in practice) with the current code but 
that is "theoretically" needed indeed, let's add it in V7 attached 
(that's safer should the code change later on).


Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com