Horton

Thread

Commits

  1. Merge v1.10 of pg_stat_statements into v1.9

  2. Track identical top vs nested queries independently in pg_stat_statements

  1. pg_stat_statements oddity with track = all

    Julien Rouhaud <rjuju123@gmail.com> — 2020-12-02T04:05:16Z

    Hi,

Someone raised an interested point recently on pg_stat_kcache extension for
handling nested statements, which also applies to pg_stat_statements.

The root issue is that when pg_stat_statements tracks nested statements,
there's no way to really make sense of the counters, as top level statements
will also account for underlying statements.  Using a naive example:

=# CREATE FUNCTION f1() RETURNS VOID AS $$BEGIN PERFORM pg_sleep(5); END; $$ LANGUAGE plpgsql;
CREATE FUNCTION

=# SELECT pg_stat_statements_reset();
 pg_stat_statements_reset
--------------------------

(1 row)

=# SELECT f1();
 f1
----

(1 row)

=# select sum(total_exec_time) from pg_stat_statements;
     sum
--------------
 10004.403601
(1 row)

It looks like there was 10s total execution time overall all statements, which
doesn't really make sense.

It's of course possible to avoid that using track = top, but tracking all
nested statements is usually quite useful so it could be better to find a way
to better address that problem.

The only idea I have for that is to add a new field to entry key, for instance
is_toplevel.  The immediate cons is obviously that it could amplify quite a lot
the number of entries tracked, so people may need to increase
pg_stat_statements.max to avoid slowdown if that makes them reach frequent
entry eviction.

Should we address the problem, and in that case do you see a better way for
that, or should we just document this behavior?

  2. Re: pg_stat_statements oddity with track = all

    Nikolay Samokhvalov <samokhvalov@gmail.com> — 2020-12-02T06:08:06Z

    On Tue, Dec 1, 2020 at 8:05 PM Julien Rouhaud <rjuju123@gmail.com> wrote:

> Hi,
>
> Someone raised an interested point recently on pg_stat_kcache extension for
> handling nested statements, which also applies to pg_stat_statements.
>
...

> The only idea I have for that is to add a new field to entry key, for
> instance
> is_toplevel.


This particular problem often bothered me when dealing with
pg_stat_statements contents operating under "track = all" (especially when
performing the aggregated analysis, like you showed).

I think the idea of having a flag to distinguish the top-level entries is
great.


> The immediate cons is obviously that it could amplify quite a lot
> the number of entries tracked, so people may need to increase
> pg_stat_statements.max to avoid slowdown if that makes them reach frequent
> entry eviction.
>

If all top-level records in pg_stat_statements have "true" in the new
column (is_toplevel), how would this lead to the need to increase
pg_stat_statements.max? The number of records would remain the same, as
before extending pg_stat_statements.

  3. Re: pg_stat_statements oddity with track = all

    Julien Rouhaud <rjuju123@gmail.com> — 2020-12-02T06:32:41Z

    On Tue, Dec 01, 2020 at 10:08:06PM -0800, Nikolay Samokhvalov wrote:
> On Tue, Dec 1, 2020 at 8:05 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> 
> > Someone raised an interested point recently on pg_stat_kcache extension for
> > handling nested statements, which also applies to pg_stat_statements.
> >
> ...
> 
> > The only idea I have for that is to add a new field to entry key, for
> > instance
> > is_toplevel.
> 
> 
> This particular problem often bothered me when dealing with
> pg_stat_statements contents operating under "track = all" (especially when
> performing the aggregated analysis, like you showed).
> 
> I think the idea of having a flag to distinguish the top-level entries is
> great.
> 

Ok!

> > The immediate cons is obviously that it could amplify quite a lot
> > the number of entries tracked, so people may need to increase
> > pg_stat_statements.max to avoid slowdown if that makes them reach frequent
> > entry eviction.
> >
> 
> If all top-level records in pg_stat_statements have "true" in the new
> column (is_toplevel), how would this lead to the need to increase
> pg_stat_statements.max? The number of records would remain the same, as
> before extending pg_stat_statements.

If the same query is getting executed both at top level and as a nested
statement, two entries will then be created.  That's probably unlikely for
things like RI trigger queries, but I don't know what to expect for client
application queries.

  4. Re: pg_stat_statements oddity with track = all

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-12-02T06:52:37Z

    
On 2020/12/02 15:32, Julien Rouhaud wrote:
> On Tue, Dec 01, 2020 at 10:08:06PM -0800, Nikolay Samokhvalov wrote:
>> On Tue, Dec 1, 2020 at 8:05 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>>
>>> Someone raised an interested point recently on pg_stat_kcache extension for
>>> handling nested statements, which also applies to pg_stat_statements.
>>>
>> ...
>>
>>> The only idea I have for that is to add a new field to entry key, for
>>> instance
>>> is_toplevel.
>>
>>
>> This particular problem often bothered me when dealing with
>> pg_stat_statements contents operating under "track = all" (especially when
>> performing the aggregated analysis, like you showed).
>>
>> I think the idea of having a flag to distinguish the top-level entries is
>> great.
>>
> 
> Ok!
> 
>>> The immediate cons is obviously that it could amplify quite a lot
>>> the number of entries tracked, so people may need to increase
>>> pg_stat_statements.max to avoid slowdown if that makes them reach frequent
>>> entry eviction.
>>>
>>
>> If all top-level records in pg_stat_statements have "true" in the new
>> column (is_toplevel), how would this lead to the need to increase
>> pg_stat_statements.max? The number of records would remain the same, as
>> before extending pg_stat_statements.
> 
> If the same query is getting executed both at top level and as a nested
> statement, two entries will then be created.  That's probably unlikely for
> things like RI trigger queries, but I don't know what to expect for client
> application queries.

Just idea; instead of boolean is_toplevel flag, what about
counting the number of times when the statement is executed
in toplevel, and also in nested level?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  5. Re: pg_stat_statements oddity with track = all

    Julien Rouhaud <rjuju123@gmail.com> — 2020-12-02T08:24:02Z

    On Wed, Dec 02, 2020 at 03:52:37PM +0900, Fujii Masao wrote:
> 
> On 2020/12/02 15:32, Julien Rouhaud wrote:
> > On Tue, Dec 01, 2020 at 10:08:06PM -0800, Nikolay Samokhvalov wrote:
> > > On Tue, Dec 1, 2020 at 8:05 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > > 
> > > > Someone raised an interested point recently on pg_stat_kcache extension for
> > > > handling nested statements, which also applies to pg_stat_statements.
> > > > 
> > > ...
> > > 
> > > > The only idea I have for that is to add a new field to entry key, for
> > > > instance
> > > > is_toplevel.
> 
> [...]
> 
> Just idea; instead of boolean is_toplevel flag, what about
> counting the number of times when the statement is executed
> in toplevel, and also in nested level?

Ah, indeed that would avoid extraneous entries.  FTR we would also need that
for the planning part.

The cons I can see is that it'll make the counters harder to process (unless we
provide a specific view for the top-level statements only for instance), and
that it assumes that doing a simple division is representative enough for the
top level/nested repartition.  This might be quite off for in some cases, e.g.
big stored procedures due to lack of autovacuum, but that can't be worse than
what we currently have.

  6. Re: pg_stat_statements oddity with track = all

    legrand legrand <legrand_legrand@hotmail.com> — 2020-12-02T13:41:36Z

    Hi,

a crazy idea:
- add a parent_statement_id column that would be NULL for top level queries
- build statement_id for nested queries based on the merge of:
   a/ current_statement_id and parent one
or
   b/ current_statement_id and nested level.

this would offer the ability to track counters at any depth level ;o)
Regards
PAscal



--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

  7. Re: pg_stat_statements oddity with track = all

    Sergei Kornilov <sk@zsrv.org> — 2020-12-02T14:13:56Z

    Hello

> - add a parent_statement_id column that would be NULL for top level queries

Will generate too much entries... Every FK for each different delete/insert, for example.
But very useful for databases with a lot of stored procedures to find where this query is called. May be new mode track = tree? Use NULL to indicate a top-level query (same as with track=tree) and some constant for any nested queries when track = all.

Also, currently a top statement will account buffers usage for underlying statements?

regards, Sergei

  8. Re: pg_stat_statements oddity with track = all

    Nikolay Samokhvalov <samokhvalov@gmail.com> — 2020-12-02T14:23:54Z

    On Tue, Dec 1, 2020 at 10:32 PM Julien Rouhaud <rjuju123@gmail.com> wrote:

> On Tue, Dec 01, 2020 at 10:08:06PM -0800, Nikolay Samokhvalov wrote:
> > If all top-level records in pg_stat_statements have "true" in the new
> > column (is_toplevel), how would this lead to the need to increase
> > pg_stat_statements.max? The number of records would remain the same, as
> > before extending pg_stat_statements.
>
> If the same query is getting executed both at top level and as a nested
> statement, two entries will then be created.  That's probably unlikely for
> things like RI trigger queries, but I don't know what to expect for client
> application queries.
>

Right, but this is how things already work. The extra field you've proposed
won't increase the number of records so it shouldn't affect how users
choose pg_stat_statements.max.

  9. Re: pg_stat_statements oddity with track = all

    Julien Rouhaud <rjuju123@gmail.com> — 2020-12-03T01:00:51Z

    On Wed, Dec 02, 2020 at 06:23:54AM -0800, Nikolay Samokhvalov wrote:
> On Tue, Dec 1, 2020 at 10:32 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> 
> > On Tue, Dec 01, 2020 at 10:08:06PM -0800, Nikolay Samokhvalov wrote:
> > > If all top-level records in pg_stat_statements have "true" in the new
> > > column (is_toplevel), how would this lead to the need to increase
> > > pg_stat_statements.max? The number of records would remain the same, as
> > > before extending pg_stat_statements.
> >
> > If the same query is getting executed both at top level and as a nested
> > statement, two entries will then be created.  That's probably unlikely for
> > things like RI trigger queries, but I don't know what to expect for client
> > application queries.
> >
> 
> Right, but this is how things already work. The extra field you've proposed
> won't increase the number of records so it shouldn't affect how users
> choose pg_stat_statements.max.

The extra field I've proposed would increase the number of records, as it needs
to be a part of the key.  The only alternative would be what Fufi-san
mentioned, i.e. to split plans and calls (for instance plans_toplevel,
plans_nested, calls_toplevel, calls_nested) and let users compute an
approximate value for toplevel counters.

  10. Re: pg_stat_statements oddity with track = all

    legrand legrand <legrand_legrand@hotmail.com> — 2020-12-03T08:16:12Z

    Hi Julien,

> The extra field I've proposed would increase the number of records, as it
> needs
to be a part of the key. 

To get an increase in the number of records that means that the same
statement 
would appear at top level AND nested level. This seems a corner case with
very low 
(neglectible) occurence rate. Did I miss something ?

Regards
PAscal 



--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

  11. Re: pg_stat_statements oddity with track = all

    Sergei Kornilov <sk@zsrv.org> — 2020-12-03T08:40:22Z

    Hello

> To get an increase in the number of records that means that the same
> statement
> would appear at top level AND nested level. This seems a corner case with
> very low
> (neglectible) occurence rate.

+1
I think splitting fields into plans_toplevel / plans_nested will be less convenient. And more code with higher chance of copypaste errors

regards, Sergei

  12. Re: pg_stat_statements oddity with track = all

    Julien Rouhaud <rjuju123@gmail.com> — 2020-12-03T08:52:08Z

    On Wed, Dec 02, 2020 at 05:13:56PM +0300, Sergei Kornilov wrote:
> Hello
> 
> > - add a parent_statement_id column that would be NULL for top level queries
> 
> Will generate too much entries... Every FK for each different delete/insert, for example.
> But very useful for databases with a lot of stored procedures to find where this query is called. May be new mode track = tree? Use NULL to indicate a top-level query (same as with track=tree) and some constant for any nested queries when track = all.

Maybe pg_stat_statements isn't the best tool for that use case.  For the record
the profiler in plpgsql_check can now track queryid for each statements inside
a function, so you match pg_stat_statements entries.  That's clearly not
perfect as dynamic queries could generate different queryid, but that's a
start.

> Also, currently a top statement will account buffers usage for underlying statements?

I think so.

  13. Re: pg_stat_statements oddity with track = all

    Julien Rouhaud <rjuju123@gmail.com> — 2020-12-03T08:53:59Z

    On Thu, Dec 03, 2020 at 11:40:22AM +0300, Sergei Kornilov wrote:
> Hello
> 
> > To get an increase in the number of records that means that the same
> > statement
> > would appear at top level AND nested level. This seems a corner case with
> > very low
> > (neglectible) occurence rate.
> 
> +1
> I think splitting fields into plans_toplevel / plans_nested will be less convenient. And more code with higher chance of copypaste errors

As I mentioned in a previous message, I really have no idea if that would be a
corner case or not.  For instance with native partitioning, the odds to have
many different query executed both at top level and as a nested statement may
be quite higher.

  14. Re: pg_stat_statements oddity with track = all

    Julien Rouhaud <rjuju123@gmail.com> — 2020-12-04T08:15:48Z

    On Thu, Dec 03, 2020 at 04:53:59PM +0800, Julien Rouhaud wrote:
> On Thu, Dec 03, 2020 at 11:40:22AM +0300, Sergei Kornilov wrote:
> > Hello
> > 
> > > To get an increase in the number of records that means that the same
> > > statement
> > > would appear at top level AND nested level. This seems a corner case with
> > > very low
> > > (neglectible) occurence rate.
> > 
> > +1
> > I think splitting fields into plans_toplevel / plans_nested will be less convenient. And more code with higher chance of copypaste errors
> 
> As I mentioned in a previous message, I really have no idea if that would be a
> corner case or not.  For instance with native partitioning, the odds to have
> many different query executed both at top level and as a nested statement may
> be quite higher.

The consensus seems to be adding a new boolean toplevel flag in the entry key,
so PFA a patch implementing that.  Note that the key now has padding, so
memset() calls are required.

  15. Re: pg_stat_statements oddity with track = all

    Sergei Kornilov <sk@zsrv.org> — 2020-12-04T09:06:10Z

    Hello

Seems we need also change PGSS_FILE_HEADER.

regards, Sergei

  16. Re: pg_stat_statements oddity with track = all

    Julien Rouhaud <rjuju123@gmail.com> — 2020-12-04T10:09:13Z

    On Fri, Dec 04, 2020 at 12:06:10PM +0300, Sergei Kornilov wrote:
> Hello
> 
> Seems we need also change PGSS_FILE_HEADER.

Indeed, thanks!  v2 attached.

  17. Re: pg_stat_statements oddity with track = all

    Julien Rouhaud <rjuju123@gmail.com> — 2020-12-27T08:38:57Z

    On Fri, Dec 04, 2020 at 06:09:13PM +0800, Julien Rouhaud wrote:
> On Fri, Dec 04, 2020 at 12:06:10PM +0300, Sergei Kornilov wrote:
> > Hello
> > 
> > Seems we need also change PGSS_FILE_HEADER.
> 
> Indeed, thanks!  v2 attached.

There was a conflict on PGSS_FILE_HEADER since some recent commit, v3 attached.

  18. Re: pg_stat_statements oddity with track = all

    Masahiro Ikeda <ikedamsh@oss.nttdata.com> — 2021-01-19T08:55:18Z

    Hi,

Thanks for making the patch to add "toplevel" column in 
pg_stat_statements.
This is a review comment.

There hasn't been any discussion, at least that I've been able to find.
So, +1 to change the status to "Ready for Committer".


1. submission/feature review
=============================

This patch can be applied cleanly to the current master branch(ed4367).
I tested with `make check-world` and I checked there is no fail.

This patch has reasonable documents and tests.
A "toplevel" column of pg_stat_statements view is documented and
following tests are added.
- pg_stat_statements.track option : 'top' and 'all'
- query type: normal query and nested query(pl/pgsql)

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.


2. usability review
====================

This patch solves the problem we can't get to know
which query is top-level if pg_stat_statements.track = 'all'.

This leads that we can analyze with aggregated queries.

There is some use-case.
For example, we can know the sum of total_exec_time of queries
even if nested queries are executed.

We can know how efficiently a database can use CPU resource for queries
using the sum of the total_exec_time, and the exec_user_time and
exec_system_time in pg_stat_kcache which is the extension gathering
os resources.

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.


3. coding review
=================

Although I had two concerns, I think they are no problem.
So, this patch looks good to me.

Following were my concerns.

A. the risk of too many same queries is duplicate.

Since this patch adds a "top" member in the hash key,
it leads to store duplicated same query which "top" is false and true.

This concern is already discussed and I agreed to the consensus
it seems unlikely to have the same queries being executed both
at the top level and as nested statements.

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.


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".


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.

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.


Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION

  19. Re: pg_stat_statements oddity with track = all

    Julien Rouhaud <rjuju123@gmail.com> — 2021-01-20T09:14:54Z

    Hi,

On Tue, Jan 19, 2021 at 4:55 PM Masahiro Ikeda <ikedamsh@oss.nttdata.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!

  20. Re: pg_stat_statements oddity with track = all

    Masahiro Ikeda <ikedamsh@oss.nttdata.com> — 2021-01-20T10:39:18Z

    On 2021-01-20 18:14, Julien Rouhaud wrote:
> On Tue, Jan 19, 2021 at 4:55 PM Masahiro Ikeda 
> <ikedamsh@oss.nttdata.com> wrote:
>> 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?

Sorry for making you confused. I can't reproduce although I tried now.
I think my procedure was something wrong. So, please ignore this 
comment, sorry about that.


>> 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?

I think so. Anyway, it's important to correspond core's implementation
because pg_stat_statements and pg_stat_kcache are used at the same time.

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION

  21. Re: pg_stat_statements oddity with track = all

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-01-20T15:43:22Z

    On Wed, Jan 20, 2021 at 6:15 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> Hi,
>
> On Tue, Jan 19, 2021 at 4:55 PM Masahiro Ikeda <ikedamsh@oss.nttdata.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!

I've changed the topic accordingly.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  22. Re: pg_stat_statements oddity with track = all

    Magnus Hagander <magnus@hagander.net> — 2021-03-06T17:56:49Z

    On Sun, Dec 27, 2020 at 9:39 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Fri, Dec 04, 2020 at 06:09:13PM +0800, Julien Rouhaud wrote:
> > On Fri, Dec 04, 2020 at 12:06:10PM +0300, Sergei Kornilov wrote:
> > > Hello
> > >
> > > Seems we need also change PGSS_FILE_HEADER.
> >
> > Indeed, thanks!  v2 attached.
>
> There was a conflict on PGSS_FILE_HEADER since some recent commit, v3 attached.

- *
- * Right now, this structure contains no padding.  If you add any, make sure
- * to teach pgss_store() to zero the padding bytes.  Otherwise, things will
- * break, because pgss_hash is created using HASH_BLOBS, and thus tag_hash
- * is used to hash this.

I don't think removing this comment completely is a good idea. Right
now it ends up there, yes, but eventually it might reach the same
state again. I think it's better to reword it based on the current
situation while keeping the part about it having to be zeroed for
padding. And maybe along with a comment at the actual memset() sites
as well?

AFAICT, it's going to store two copies of the query in the query text
file (pgss_query_texts.stat)?  Can we find a way around that? Maybe by
looking up the entry with the flag set to the other value, and then
reusing that?

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

  23. Re: pg_stat_statements oddity with track = all

    Julien Rouhaud <rjuju123@gmail.com> — 2021-03-07T07:28:10Z

    On Thu, Jan 21, 2021 at 12:43:22AM +0900, Masahiko Sawada wrote:
> On Wed, Jan 20, 2021 at 6:15 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > I agree, thanks for the change!
> 
> I've changed the topic accordingly.

Thanks Sawada-san!  I thought that I took care of that but I somehow missed it.

  24. Re: pg_stat_statements oddity with track = all

    Julien Rouhaud <rjuju123@gmail.com> — 2021-03-07T07:39:36Z

    On Sat, Mar 06, 2021 at 06:56:49PM +0100, Magnus Hagander wrote:
> On Sun, Dec 27, 2020 at 9:39 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> - *
> - * Right now, this structure contains no padding.  If you add any, make sure
> - * to teach pgss_store() to zero the padding bytes.  Otherwise, things will
> - * break, because pgss_hash is created using HASH_BLOBS, and thus tag_hash
> - * is used to hash this.
> 
> I don't think removing this comment completely is a good idea. Right
> now it ends up there, yes, but eventually it might reach the same
> state again. I think it's better to reword it based on the current
> situation while keeping the part about it having to be zeroed for
> padding. And maybe along with a comment at the actual memset() sites
> as well?

Agreed, I'll take care of that.

> AFAICT, it's going to store two copies of the query in the query text
> file (pgss_query_texts.stat)?  Can we find a way around that? Maybe by
> looking up the entry with the flag set to the other value, and then
> reusing that?

Yes I was a bit worried about the possible extra text entry.  I kept things
simple until now as the general opinion was that entries existing for both top
level and nested level should be very rare so adding extra code and overhead to
spare a few query texts wouldn't be worth it.

I think that using a flag might be a bit expensive, as we would have to make
sure that at least one of the possible two entries has it.  So if there are 2
entries and the one with the flag is evicted, we would have to transfer the
flag to the other one, and check the existence of the flag when allocatin a new
entry.  And all of that has to be done holding an exclusive lock on pgss->lock.

Maybe having a new hash table (without the toplevel flag) for those query text
might be better, or maybe pgss performance is already so terrible when you have
to regularly evict entries that it wouldn't make any real difference.

Should I try to add some extra code to make sure that we only store the query
text once, or should I document that there might be duplicate, but we expect
that to be very rare?

  25. Re: pg_stat_statements oddity with track = all

    Magnus Hagander <magnus@hagander.net> — 2021-03-08T17:03:59Z

    On Sun, Mar 7, 2021 at 8:39 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Sat, Mar 06, 2021 at 06:56:49PM +0100, Magnus Hagander wrote:
> > On Sun, Dec 27, 2020 at 9:39 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > >
> > - *
> > - * Right now, this structure contains no padding.  If you add any, make sure
> > - * to teach pgss_store() to zero the padding bytes.  Otherwise, things will
> > - * break, because pgss_hash is created using HASH_BLOBS, and thus tag_hash
> > - * is used to hash this.
> >
> > I don't think removing this comment completely is a good idea. Right
> > now it ends up there, yes, but eventually it might reach the same
> > state again. I think it's better to reword it based on the current
> > situation while keeping the part about it having to be zeroed for
> > padding. And maybe along with a comment at the actual memset() sites
> > as well?
>
> Agreed, I'll take care of that.
>
> > AFAICT, it's going to store two copies of the query in the query text
> > file (pgss_query_texts.stat)?  Can we find a way around that? Maybe by
> > looking up the entry with the flag set to the other value, and then
> > reusing that?
>
> Yes I was a bit worried about the possible extra text entry.  I kept things
> simple until now as the general opinion was that entries existing for both top
> level and nested level should be very rare so adding extra code and overhead to
> spare a few query texts wouldn't be worth it.
>
> I think that using a flag might be a bit expensive, as we would have to make
> sure that at least one of the possible two entries has it.  So if there are 2
> entries and the one with the flag is evicted, we would have to transfer the
> flag to the other one, and check the existence of the flag when allocatin a new
> entry.  And all of that has to be done holding an exclusive lock on pgss->lock.

Yeah, we'd certainly want to minimize things. But what if they both
have the flag at that point? Then we wouldn't have to care on
eviction? But yes, for new allications we'd have to look up if the
query existed with the other value of the flag, and copy it over in
that case.

> Maybe having a new hash table (without the toplevel flag) for those query text
> might be better, or maybe pgss performance is already so terrible when you have
> to regularly evict entries that it wouldn't make any real difference.
>
> Should I try to add some extra code to make sure that we only store the query
> text once, or should I document that there might be duplicate, but we expect
> that to be very rare?

If we expect it to be rare, I think it might be reasonable to just
document that. But do we really have a strong argument for it being
rare?

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

  26. Re: pg_stat_statements oddity with track = all

    Julien Rouhaud <rjuju123@gmail.com> — 2021-03-09T02:39:54Z

    On Mon, Mar 08, 2021 at 06:03:59PM +0100, Magnus Hagander wrote:
> On Sun, Mar 7, 2021 at 8:39 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > Yes I was a bit worried about the possible extra text entry.  I kept things
> > simple until now as the general opinion was that entries existing for both top
> > level and nested level should be very rare so adding extra code and overhead to
> > spare a few query texts wouldn't be worth it.
> >
> > I think that using a flag might be a bit expensive, as we would have to make
> > sure that at least one of the possible two entries has it.  So if there are 2
> > entries and the one with the flag is evicted, we would have to transfer the
> > flag to the other one, and check the existence of the flag when allocatin a new
> > entry.  And all of that has to be done holding an exclusive lock on pgss->lock.
> 
> Yeah, we'd certainly want to minimize things. But what if they both
> have the flag at that point? Then we wouldn't have to care on
> eviction? But yes, for new allications we'd have to look up if the
> query existed with the other value of the flag, and copy it over in
> that case.

I think that we might be able to handle that without a flag.  The only thing
that would need to be done is when creating an entry, look for an existing
entry with the opposite flag, and if there's simply use the same
(query_offset, query_len) info.  This doesn't sound that expensive.

The real pain point will be that the garbage collection phase
will become way more expensive as it will now have to somehow maintain that
knowledge, which will require additional lookups for each entry.  I'm a bit
concerned about that, especially with the current heuristic to schedule garbage
collection.  For now, need_qc_qtext says that we have to do it if the extent is
more than 512 (B) * pgss_max.  This probably doesn't work well for people using
ORM as they tend to generate gigantic SQL queries.

If we implement query text deduplication, should we add another GUC for that
"512" magic value so that people can minimize the gc overhead if they know they
have gigantic queries, or simply don't mind bigger qtext file?

> > Maybe having a new hash table (without the toplevel flag) for those query text
> > might be better, or maybe pgss performance is already so terrible when you have
> > to regularly evict entries that it wouldn't make any real difference.
> >
> > Should I try to add some extra code to make sure that we only store the query
> > text once, or should I document that there might be duplicate, but we expect
> > that to be very rare?
> 
> If we expect it to be rare, I think it might be reasonable to just
> document that. But do we really have a strong argument for it being
> rare?

I don't that think that anyone really had a strong argument, mostly gut
feeling.  Note that pg_stat_kcache already implemented that toplevel flags, so
if people are using that extension in a recent version they might have some
figures to show.  I'll ping some people that I know are using it.

One good argument would be that gigantic queries generated by ORM should always
be executed as top level statements.

I previously tried with the postgres regression tests, which clearly isn't a
representative workload, and as far as I can see the vast majority of queries
executed bost as top level and nested level are DDL implying recursion (e.g. a
CREATE TABLE with underlying index creation).

  27. Re: pg_stat_statements oddity with track = all

    Magnus Hagander <magnus@hagander.net> — 2021-03-16T11:55:45Z

    On Tue, Mar 9, 2021 at 3:39 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Mon, Mar 08, 2021 at 06:03:59PM +0100, Magnus Hagander wrote:
> > On Sun, Mar 7, 2021 at 8:39 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > >
> > > Yes I was a bit worried about the possible extra text entry.  I kept things
> > > simple until now as the general opinion was that entries existing for both top
> > > level and nested level should be very rare so adding extra code and overhead to
> > > spare a few query texts wouldn't be worth it.
> > >
> > > I think that using a flag might be a bit expensive, as we would have to make
> > > sure that at least one of the possible two entries has it.  So if there are 2
> > > entries and the one with the flag is evicted, we would have to transfer the
> > > flag to the other one, and check the existence of the flag when allocatin a new
> > > entry.  And all of that has to be done holding an exclusive lock on pgss->lock.
> >
> > Yeah, we'd certainly want to minimize things. But what if they both
> > have the flag at that point? Then we wouldn't have to care on
> > eviction? But yes, for new allications we'd have to look up if the
> > query existed with the other value of the flag, and copy it over in
> > that case.
>
> I think that we might be able to handle that without a flag.  The only thing
> that would need to be done is when creating an entry, look for an existing
> entry with the opposite flag, and if there's simply use the same
> (query_offset, query_len) info.  This doesn't sound that expensive.

That's basically what I was trying to say :)


> The real pain point will be that the garbage collection phase
> will become way more expensive as it will now have to somehow maintain that
> knowledge, which will require additional lookups for each entry.  I'm a bit
> concerned about that, especially with the current heuristic to schedule garbage
> collection.  For now, need_qc_qtext says that we have to do it if the extent is
> more than 512 (B) * pgss_max.  This probably doesn't work well for people using
> ORM as they tend to generate gigantic SQL queries.

Right, the cost would be mostly on the GC  side. I've never done any
profiling to see how big of a thing that is in systems today -- have
you?


> If we implement query text deduplication, should we add another GUC for that
> "512" magic value so that people can minimize the gc overhead if they know they
> have gigantic queries, or simply don't mind bigger qtext file?
>
> > > Maybe having a new hash table (without the toplevel flag) for those query text
> > > might be better, or maybe pgss performance is already so terrible when you have
> > > to regularly evict entries that it wouldn't make any real difference.
> > >
> > > Should I try to add some extra code to make sure that we only store the query
> > > text once, or should I document that there might be duplicate, but we expect
> > > that to be very rare?
> >
> > If we expect it to be rare, I think it might be reasonable to just
> > document that. But do we really have a strong argument for it being
> > rare?
>
> I don't that think that anyone really had a strong argument, mostly gut
> feeling.  Note that pg_stat_kcache already implemented that toplevel flags, so
> if people are using that extension in a recent version they might have some
> figures to show.  I'll ping some people that I know are using it.

Great -- data always wins over gut feelings :)


> One good argument would be that gigantic queries generated by ORM should always
> be executed as top level statements.

Yes, that's true. And it probably holds as a more generic case as
well, that is the queries that are likely to show up both top-level
and lower-level are more likely to be relatively simple ones. (Except
for example during the development of functions/procs where they're
often executed top level as well to test etc, but that's not the most
important case to optimize for)


> I previously tried with the postgres regression tests, which clearly isn't a
> representative workload, and as far as I can see the vast majority of queries
> executed bost as top level and nested level are DDL implying recursion (e.g. a
> CREATE TABLE with underlying index creation).

I think the PostgreSQL regression tests are so far from a real world
workload that the input in this case has a value of exactly zero.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

  28. Re: pg_stat_statements oddity with track = all

    Julien Rouhaud <rjuju123@gmail.com> — 2021-03-16T15:35:30Z

    On Tue, Mar 16, 2021 at 12:55:45PM +0100, Magnus Hagander wrote:
> On Tue, Mar 9, 2021 at 3:39 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > I think that we might be able to handle that without a flag.  The only thing
> > that would need to be done is when creating an entry, look for an existing
> > entry with the opposite flag, and if there's simply use the same
> > (query_offset, query_len) info.  This doesn't sound that expensive.
> 
> That's basically what I was trying to say :)

Oh ok sorry :)

> > The real pain point will be that the garbage collection phase
> > will become way more expensive as it will now have to somehow maintain that
> > knowledge, which will require additional lookups for each entry.  I'm a bit
> > concerned about that, especially with the current heuristic to schedule garbage
> > collection.  For now, need_qc_qtext says that we have to do it if the extent is
> > more than 512 (B) * pgss_max.  This probably doesn't work well for people using
> > ORM as they tend to generate gigantic SQL queries.
> 
> Right, the cost would be mostly on the GC  side. I've never done any
> profiling to see how big of a thing that is in systems today -- have
> you?

I didn't, but I don't see how it could be anything but ridiculously impacting.
it's basically preventing any query from being planned or executed on the whole
instance the time needed to read the previous qtext file, and write all entries
still needed.

> > I don't that think that anyone really had a strong argument, mostly gut
> > feeling.  Note that pg_stat_kcache already implemented that toplevel flags, so
> > if people are using that extension in a recent version they might have some
> > figures to show.  I'll ping some people that I know are using it.
> 
> Great -- data always wins over gut feelings :)

So I asked some friends that have latest pg_stat_kcache installed on some
preproduction environment configured to track nested queries.  There isn't a
high throughput but the activity should still be representative of the
production queries.  There are a lot of applications plugged there, around 20
databases and quite a lot of PL code.

After a few days, here are the statistics:

- total of ~ 9500 entries
- ~ 900 entries for nested statements
- ~ 35 entries existing for both top level and nested statements

So the duplicates account for less than 4% of the nested statements, and less
than 0.5% of the whole entries.

I wish I had more reports, but if this one is representative enough then it
seems that trying to avoid storing duplicated queries wouldn't be worth it.

> > One good argument would be that gigantic queries generated by ORM should always
> > be executed as top level statements.
> 
> Yes, that's true. And it probably holds as a more generic case as
> well, that is the queries that are likely to show up both top-level
> and lower-level are more likely to be relatively simple ones. (Except
> for example during the development of functions/procs where they're
> often executed top level as well to test etc, but that's not the most
> important case to optimize for)

Agreed.

> > I previously tried with the postgres regression tests, which clearly isn't a
> > representative workload, and as far as I can see the vast majority of queries
> > executed bost as top level and nested level are DDL implying recursion (e.g. a
> > CREATE TABLE with underlying index creation).
> 
> I think the PostgreSQL regression tests are so far from a real world
> workload that the input in this case has a value of exactly zero.

I totally agree, but that's the only one I had at that time :)  Still it wasn't
entirely useless as I didn't realize before that that some DDL would lead to
duplicated entries.

  29. Re: pg_stat_statements oddity with track = all

    Magnus Hagander <magnus@hagander.net> — 2021-04-08T08:30:53Z

    On Tue, Mar 16, 2021 at 4:34 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Tue, Mar 16, 2021 at 12:55:45PM +0100, Magnus Hagander wrote:
> > On Tue, Mar 9, 2021 at 3:39 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > >
> > > I think that we might be able to handle that without a flag.  The only thing
> > > that would need to be done is when creating an entry, look for an existing
> > > entry with the opposite flag, and if there's simply use the same
> > > (query_offset, query_len) info.  This doesn't sound that expensive.
> >
> > That's basically what I was trying to say :)
>
> Oh ok sorry :)
>
> > > The real pain point will be that the garbage collection phase
> > > will become way more expensive as it will now have to somehow maintain that
> > > knowledge, which will require additional lookups for each entry.  I'm a bit
> > > concerned about that, especially with the current heuristic to schedule garbage
> > > collection.  For now, need_qc_qtext says that we have to do it if the extent is
> > > more than 512 (B) * pgss_max.  This probably doesn't work well for people using
> > > ORM as they tend to generate gigantic SQL queries.
> >
> > Right, the cost would be mostly on the GC  side. I've never done any
> > profiling to see how big of a thing that is in systems today -- have
> > you?
>
> I didn't, but I don't see how it could be anything but ridiculously impacting.
> it's basically preventing any query from being planned or executed on the whole
> instance the time needed to read the previous qtext file, and write all entries
> still needed.
>
> > > I don't that think that anyone really had a strong argument, mostly gut
> > > feeling.  Note that pg_stat_kcache already implemented that toplevel flags, so
> > > if people are using that extension in a recent version they might have some
> > > figures to show.  I'll ping some people that I know are using it.
> >
> > Great -- data always wins over gut feelings :)
>
> So I asked some friends that have latest pg_stat_kcache installed on some
> preproduction environment configured to track nested queries.  There isn't a
> high throughput but the activity should still be representative of the
> production queries.  There are a lot of applications plugged there, around 20
> databases and quite a lot of PL code.
>
> After a few days, here are the statistics:
>
> - total of ~ 9500 entries
> - ~ 900 entries for nested statements
> - ~ 35 entries existing for both top level and nested statements
>
> So the duplicates account for less than 4% of the nested statements, and less
> than 0.5% of the whole entries.
>
> I wish I had more reports, but if this one is representative enough then it
> seems that trying to avoid storing duplicated queries wouldn't be worth it.

I agree. If those numbers are indeed representable, it seems like
better to pay that overhead than to pay the overhead of trying to
de-dupe it.

Let's hope they are :)


Looking through ti again my feeling said the toplevel column should go
after the queryid and not before, but I'm not going to open up a
bikeshed over that.

I've added in a comment to cover that one that you removed (if you did
send an updated patch as you said, then I missed it -- sorry), and
applied the rest.

Thanks!

--
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

  30. Re: pg_stat_statements oddity with track = all

    Julien Rouhaud <rjuju123@gmail.com> — 2021-04-08T12:05:05Z

    On Thu, Apr 08, 2021 at 10:30:53AM +0200, Magnus Hagander wrote:
> 
> I agree. If those numbers are indeed representable, it seems like
> better to pay that overhead than to pay the overhead of trying to
> de-dupe it.
> 
> Let's hope they are :)

:)

> Looking through ti again my feeling said the toplevel column should go
> after the queryid and not before, but I'm not going to open up a
> bikeshed over that.
> 
> I've added in a comment to cover that one that you removed (if you did
> send an updated patch as you said, then I missed it -- sorry), and
> applied the rest.

Oops, somehow I totally forgot to send the new patch, sorry :(

While looking at the patch, I unfortunately just realize that I unnecessarily
bumped the version to 1.10, as 1.9 was already new as of pg14.  Honestly I have
no idea why I used 1.10 at that time.  Version numbers are not a scarce
resource but maybe it would be better to keep 1.10 for a future major postgres
version?

If yes, PFA a patch to merge 1.10 in 1.9.

  31. Re: pg_stat_statements oddity with track = all

    Magnus Hagander <magnus@hagander.net> — 2021-04-08T13:18:09Z

    On Thu, Apr 8, 2021 at 2:04 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Thu, Apr 08, 2021 at 10:30:53AM +0200, Magnus Hagander wrote:
> >
> > I agree. If those numbers are indeed representable, it seems like
> > better to pay that overhead than to pay the overhead of trying to
> > de-dupe it.
> >
> > Let's hope they are :)
>
> :)
>
> > Looking through ti again my feeling said the toplevel column should go
> > after the queryid and not before, but I'm not going to open up a
> > bikeshed over that.
> >
> > I've added in a comment to cover that one that you removed (if you did
> > send an updated patch as you said, then I missed it -- sorry), and
> > applied the rest.
>
> Oops, somehow I totally forgot to send the new patch, sorry :(
>
> While looking at the patch, I unfortunately just realize that I unnecessarily
> bumped the version to 1.10, as 1.9 was already new as of pg14.  Honestly I have
> no idea why I used 1.10 at that time.  Version numbers are not a scarce
> resource but maybe it would be better to keep 1.10 for a future major postgres
> version?
>
> If yes, PFA a patch to merge 1.10 in 1.9.

I actually thought I looked at that, but clearly I was confused one
way or another.

I think you're right, it's cleaner to merge it into 1.9, so applied and pushed.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

  32. Re: pg_stat_statements oddity with track = all

    Julien Rouhaud <rjuju123@gmail.com> — 2021-04-08T14:54:09Z

    On Thu, Apr 08, 2021 at 03:18:09PM +0200, Magnus Hagander wrote:
> On Thu, Apr 8, 2021 at 2:04 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > If yes, PFA a patch to merge 1.10 in 1.9.
> 
> I actually thought I looked at that, but clearly I was confused one
> way or another.
> 
> I think you're right, it's cleaner to merge it into 1.9, so applied and pushed.

Thanks Magnus!