Re: pg_stat_statements oddity with track = all

Hi,

On Tue, Jan 19, 2021 at 4:55 PM Masahiro Ikeda <ikedamsh(at)oss(dot)nttdata(dot)com> wrote:
>
> Thanks for making the patch to add "toplevel" column in
> pg_stat_statements.
> This is a review comment.

Thanks a lot for the thorough review!

> I tested the "update" command can work.
> postgres=# ALTER EXTENSION pg_stat_statements UPDATE TO '1.10';
>
> Although the "toplevel" column of all queries which already stored is
> 'false',
> we have to decide the default. I think 'false' is ok.

Mmm, I'm not sure that I understand this result. The "toplevel" value
is tracked by the C code loaded at startup, so it should have a
correct value even if you used to have the extension in a previous
version, it's just that you can't access the toplevel field before
doing the ALTER EXTENSION pg_stat_statements UPDATE. There's also a
change in the magic number, so you can't use the stored stat file from
a version before this patch.

I also fail to reproduce this behavior. I did the following:

- start postgres with pg_stat_statements v1.10 (so with this patch) in
shared_preload_libraries
- CREATE EXTENSION pg_stat_statements VERSION '1.9';
- execute a few queries
- ALTER EXTENSION pg_stat_statements UPDATE;
- SELECT * FROM pg_stat_statements reports the query with toplvel to TRUE

Can you share a way to reproduce your findings?

> 2. usability review
> ====================
> [...]
> Although one concern is whether only top-level is enough or not,
> I couldn't come up with any use-case to use nested level, so I think
> it's ok.

I agree, I don't see how tracking statistics per nesting level would
really help. The only additional use case I see would tracking
triggers/FK query execution, but the nesting level won't help with
that.

> 3. coding review
> =================
> [...]
> B. add a argument of the pg_stat_statements_reset().
>
> Now, pg_stat_statements supports a flexible reset feature.
> > pg_stat_statements_reset(userid Oid, dbid Oid, queryid bigint)
>
> Although I wondered whether we need to add a top-level flag to the
> arguments,
> I couldn't come up with any use-case to reset only top-level queries or
> not top-level queries.

Ah, I didn't think of the reset function. I also fail to see a
reasonable use case to provide a switch for the reset function.

> 4. others
> ==========
>
> These comments are not related to this patch.
>
> A. about the topic of commitfests
>
> Since I think this feature is for monitoring,
> it's better to change the topic from "System Administration"
> to "Monitoring & Control".

I agree, thanks for the change!

> B. check logic whether a query is top-level or not in pg_stat_kcache
>
> This patch uses the only exec_nested_level to check whether a query is
> top-level or not.
> The reason is nested_level is less useful and I agree.

Do you mean plan_nest_level is less useful?

> But, pg_stat_kcache uses plan_nested_level too.
> Although the check result is the same, it's better to change it
> corresponding to this patch after it's committed.

I agree, we should be consistent here. I'll take care of the needed
changes if and when this patch is commited!