Planning counters in pg_stat_statements (using pgss_store)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
117 messages Options
123456
Reply | Threaded
Open this post in threaded view
|

RE: Planning counters in pg_stat_statements (using pgss_store)

imai.yoshikazu@fujitsu.com
On Wed, Nov 20, 2019 at 4:55 PM, Julien Rouhaud wrote:

> On Wed, Nov 20, 2019 at 2:06 AM [hidden email] <[hidden email]> wrote:
> >
> > On Tue, Nov 19, 2019 at 2:27 PM, Julien Rouhaud wrote:
> > > On Fri, Nov 15, 2019 at 2:00 AM [hidden email] <[hidden email]> wrote:
> > > >
> > > > Actually I also don't have strong opinion but I thought someone
> > > > would complain about renaming of those columns and
> > > also some tools like monitoring which use those columns will not
> > > work. If we use {total, min, max, mean, stddev}_time, someone might
> > > mistakenly understand {total, min, max, mean, stddev}_time mean {total, min, max, mean, stddev} of planning and
> execution.
> > > > If I need to choose {total, min, max, mean, stddev}_time or
> > > > {total, min, max, mean, stddev}_exec_time, I choose former
> > > one because choosing best name is not worth destructing the existing scripts or tools.
> > >
> > > We could definitely keep (plan|exec)_time for the SRF, and have the
> > > {total, min, max, mean, stddev}_time created by the view to be a sum
> > > of planning + execution for each counter
> >
> > I might misunderstand but if we define {total, min, max, mean,
> > stddev}_time is just a sum of planning + execution for each counter
> > like "select total_plan_time + total_exec_time as total_time from
> > pg_stat_statements", I wonder we can calculate stddev_time correctly.
> > If we prepare variables in the codes to calculate those values, yes,
> > we can correctly calculate those values even for the total_stddev.
>
> Yes you're right, this can't possibly work for most of the counters.
> And also, since there's no guarantee that each execution will follow a planning, providing such global counters for
> min/max/mean and stddev wouldn't make much sense.

Ah, I see. Planning counts and execution counts differ.
It might be difficult to redefine the meaning of {min, max, mean, stddev}_time precisely, and even if we can redefine it correctly, it would not be intuitive.

--
Yoshikazu Imai
Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Julien Rouhaud
On Fri, Nov 22, 2019 at 11:23 AM [hidden email]
<[hidden email]> wrote:

>
> On Wed, Nov 20, 2019 at 4:55 PM, Julien Rouhaud wrote:
> > On Wed, Nov 20, 2019 at 2:06 AM [hidden email] <[hidden email]> wrote:
> > >
> > > On Tue, Nov 19, 2019 at 2:27 PM, Julien Rouhaud wrote:
> > > > On Fri, Nov 15, 2019 at 2:00 AM [hidden email] <[hidden email]> wrote:
> > > > >
> > > > > Actually I also don't have strong opinion but I thought someone
> > > > > would complain about renaming of those columns and
> > > > also some tools like monitoring which use those columns will not
> > > > work. If we use {total, min, max, mean, stddev}_time, someone might
> > > > mistakenly understand {total, min, max, mean, stddev}_time mean {total, min, max, mean, stddev} of planning and
> > execution.
> > > > > If I need to choose {total, min, max, mean, stddev}_time or
> > > > > {total, min, max, mean, stddev}_exec_time, I choose former
> > > > one because choosing best name is not worth destructing the existing scripts or tools.
> > > >
> > > > We could definitely keep (plan|exec)_time for the SRF, and have the
> > > > {total, min, max, mean, stddev}_time created by the view to be a sum
> > > > of planning + execution for each counter
> > >
> > > I might misunderstand but if we define {total, min, max, mean,
> > > stddev}_time is just a sum of planning + execution for each counter
> > > like "select total_plan_time + total_exec_time as total_time from
> > > pg_stat_statements", I wonder we can calculate stddev_time correctly.
> > > If we prepare variables in the codes to calculate those values, yes,
> > > we can correctly calculate those values even for the total_stddev.
> >
> > Yes you're right, this can't possibly work for most of the counters.
> > And also, since there's no guarantee that each execution will follow a planning, providing such global counters for
> > min/max/mean and stddev wouldn't make much sense.
>
> Ah, I see. Planning counts and execution counts differ.
> It might be difficult to redefine the meaning of {min, max, mean, stddev}_time precisely, and even if we can redefine it correctly, it would not be intuitive.
Thomas' automatic patch tester just warned me that the patchset is
broken since 3fd40b628c7db4, which removed the queryString from
ExecCreateTableAs.  New patch version that re-add the queryString, no
changes otherwise.

0001-Pass-query-string-to-the-planner-v3.patch (16K) Download Attachment
0002-Add-planning-counters-to-pg_stat_statements-v3.patch (62K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

legrand legrand
Hi Julien,

I would like to create a link with
https://www.postgresql.org/message-id/1577490124579-0.post@...

where we met an ASSET FAILURE because query text was not initialized ...

The question raised is:

- should query text be always provided
or
- if not how to deal that case (in pgss).

Regards
PAscal



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


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Julien Rouhaud
On Sun, Jan 5, 2020 at 4:11 PM legrand legrand
<[hidden email]> wrote:

>
> Hi Julien,
>
> I would like to create a link with
> https://www.postgresql.org/message-id/1577490124579-0.post@...
>
> where we met an ASSET FAILURE because query text was not initialized ...
>
> The question raised is:
>
> - should query text be always provided
> or
> - if not how to deal that case (in pgss).

I'd think that since the query text was until now always provided,
there's no reason why this patch should change that.  That being said,
there has been other concerns raised wrt. temporary tables in the IVM
patchset, so ISTM that there might be important architectural changes
upcoming, so having to deal with this case in pgss is not rushed
(especially since handling that in pgss would be trivial), and can
help to catch issue with the query text pasing.


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

legrand legrand
Julien Rouhaud wrote
> On Sun, Jan 5, 2020 at 4:11 PM legrand legrand
> &lt;

> legrand_legrand@

> &gt; wrote:
>>
>> Hi Julien,
>>
>> I would like to create a link with
>> https://www.postgresql.org/message-id/

> 1577490124579-0.post@.nabble

>>
>> where we met an ASSET FAILURE because query text was not initialized ...
>>
>> The question raised is:
>>
>> - should query text be always provided
>> or
>> - if not how to deal that case (in pgss).
>
> I'd think that since the query text was until now always provided,
> there's no reason why this patch should change that.  That being said,
> there has been other concerns raised wrt. temporary tables in the IVM
> patchset, so ISTM that there might be important architectural changes
> upcoming, so having to deal with this case in pgss is not rushed
> (especially since handling that in pgss would be trivial), and can
> help to catch issue with the query text pasing.

IVM revealed that ASSERT,
but IVM works fine with pg_stat_statements.track_planning = off.
There may be others parts of postgresql that would have workede fine as
well.

This means 2 things:
- there is a (litle) risk to meet other assert failures when using planning
counters in pgss,
- we have an easy workarround to fix it (disabling track_planning).

But I would have prefered this new feature to work the same way with or
without track_planning activated ;o(



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


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Julien Rouhaud
On Sun, Jan 5, 2020 at 7:02 PM legrand legrand
<[hidden email]> wrote:

>
> Julien Rouhaud wrote
> > On Sun, Jan 5, 2020 at 4:11 PM legrand legrand
> > &lt;
>
> > legrand_legrand@
>
> > &gt; wrote:
> >>
> >> Hi Julien,
> >>
> >> I would like to create a link with
> >> https://www.postgresql.org/message-id/
>
> > 1577490124579-0.post@.nabble
>
> >>
> >> where we met an ASSET FAILURE because query text was not initialized ...
> >>
> >> The question raised is:
> >>
> >> - should query text be always provided
> >> or
> >> - if not how to deal that case (in pgss).
> >
> > I'd think that since the query text was until now always provided,
> > there's no reason why this patch should change that.  That being said,
> > there has been other concerns raised wrt. temporary tables in the IVM
> > patchset, so ISTM that there might be important architectural changes
> > upcoming, so having to deal with this case in pgss is not rushed
> > (especially since handling that in pgss would be trivial), and can
> > help to catch issue with the query text pasing.
>
> IVM revealed that ASSERT,
> but IVM works fine with pg_stat_statements.track_planning = off.

Yes, but on the other hand the current IVM patchset also adds the only
pg_plan_query call that don't provide a query text.  I didn't see any
other possibility, and if there are other cases they're unfortunately
not covered by the full regression tests.

> There may be others parts of postgresql that would have workede fine as
> well.
>
> This means 2 things:
> - there is a (litle) risk to meet other assert failures when using planning
> counters in pgss,
> - we have an easy workarround to fix it (disabling track_planning).
>
> But I would have prefered this new feature to work the same way with or
> without track_planning activated ;o(

Definitely, but fixing the issue in pgss (ignoring planner calls when
we don't have a query text) means that pgss won't give an exhaustive
view of activity anymore, so a fix in IVM would be a better solution.
Let's wait and see if Nagata-san and other people involved in that
have an opinion on it.


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

legrand legrand
In reply to this post by Julien Rouhaud
Hi Julien,

bot is still unhappy
https://travis-ci.org/postgresql-cfbot/postgresql/builds/638701399

portalcmds.c: In function ‘PerformCursorOpen’:
portalcmds.c:93:7: error: ‘queryString’ may be used uninitialized in this
function [-Werror=maybe-uninitialized]
  plan = pg_plan_query(query, queryString, cstmt->options, params);
       ^
portalcmds.c:50:8: note: ‘queryString’ was declared here
  char    *queryString;
        ^
cc1: all warnings being treated as errors
<builtin>: recipe for target 'portalcmds.o' failed
make[3]: *** [portalcmds.o] Error 1
make[3]: Leaving directory
'/home/travis/build/postgresql-cfbot/postgresql/src/backend/commands'
common.mk:39: recipe for target 'commands-recursive' failed
make[2]: *** [commands-recursive] Error 2
make[2]: *** Waiting for unfinished jobs....

regards
PAscal



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


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Julien Rouhaud
Hi,

On Sat, Jan 18, 2020 at 6:14 PM legrand legrand
<[hidden email]> wrote:

>
> Hi Julien,
>
> bot is still unhappy
> https://travis-ci.org/postgresql-cfbot/postgresql/builds/638701399
>
> portalcmds.c: In function ‘PerformCursorOpen’:
> portalcmds.c:93:7: error: ‘queryString’ may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
>   plan = pg_plan_query(query, queryString, cstmt->options, params);
>        ^
> portalcmds.c:50:8: note: ‘queryString’ was declared here
>   char    *queryString;
>         ^
> cc1: all warnings being treated as errors
> <builtin>: recipe for target 'portalcmds.o' failed
> make[3]: *** [portalcmds.o] Error 1
> make[3]: Leaving directory
> '/home/travis/build/postgresql-cfbot/postgresql/src/backend/commands'
> common.mk:39: recipe for target 'commands-recursive' failed
> make[2]: *** [commands-recursive] Error 2
> make[2]: *** Waiting for unfinished jobs....
Indeed, thanks for the report!  PFA rebased v4 version of the patchset.

0001-Pass-query-string-to-the-planner_v4.patch (14K) Download Attachment
0002-Add-planning-counters-to-pg_stat_statements_v4.patch (38K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

legrand legrand
In reply to this post by Julien Rouhaud
Hi Julien,

>> But I would have prefered this new feature to work the same way with or
>> without track_planning activated ;o(

> Definitely, but fixing the issue in pgss (ignoring planner calls when
> we don't have a query text) means that pgss won't give an exhaustive
> view of activity anymore, so a fix in IVM would be a better solution.
> Let's wait and see if Nagata-san and other people involved in that
> have an opinion on it.

It seems IVM team does not consider this point as a priority ...
We should not wait for them, if we want to keep a chance to be
included in PG13.

So we have to make this feature more robust, an assert failure being
considered as a severe regression (even if this is not coming from pgss).

I like the idea of adding a check for a non-zero queryId in the new
pgss_planner_hook() (zero queryid shouldn't be reserved for
utility_statements ?).

Fixing the corner case where a query (with no sql text) can be planned
without being parsed is an other subject that should be resolved in an
other thread.

This kind of query was ignored in pgss, it should be ignored in pgss with
planning counters.

Any thoughts ?
Regards
PAscal



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


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Julien Rouhaud
Hi,

On Fri, Feb 28, 2020 at 4:06 PM legrand legrand
<[hidden email]> wrote:
>
> It seems IVM team does not consider this point as a priority ...

Well, IVM is a big project and I agree that fixing this issue isn't
the most urgent one, especially since there's no guarantee that this
pgss planning patch will be committed, or with the current behavior.

> We should not wait for them, if we want to keep a chance to be
> included in PG13.
>
> So we have to make this feature more robust, an assert failure being
> considered as a severe regression (even if this is not coming from pgss).

I'm still not convinced that handling NULL query string, as in
sometimes ignoring planning counters, is the right solution here. For
now all code is able to provide it (or at least all the code that goes
through make installcheck).   I'm wondering if it'd be better to
instead add a similar assert in pg_plan_query, to make sure that this
requirements is always met even without using pg_stat_statements, or
any other extension that would also rely on that.

I also realized that the last version of the patch I sent was a rebase
of the wrong version, I'll send the correct version soon.

> I like the idea of adding a check for a non-zero queryId in the new
> pgss_planner_hook() (zero queryid shouldn't be reserved for
> utility_statements ?).

Some assert hit later, I can say that it's not always true.  For
instance a CREATE TABLE AS won't run parse analysis for the underlying
query, as this has already been done for the original statement, but
will still call the planner.  I'll change pgss_planner_hook to ignore
such cases, as pgss_store would otherwise think that it's a utility
statement.  That'll probably incidentally fix the IVM incompatibility.


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

legrand legrand
>> I like the idea of adding a check for a non-zero queryId in the new
>> pgss_planner_hook() (zero queryid shouldn't be reserved for
>> utility_statements ?).

> Some assert hit later, I can say that it's not always true.  For
> instance a CREATE TABLE AS won't run parse analysis for the underlying
> query, as this has already been done for the original statement, but
> will still call the planner.  I'll change pgss_planner_hook to ignore
> such cases, as pgss_store would otherwise think that it's a utility
> statement.  That'll probably incidentally fix the IVM incompatibility.

Today with or without test on parse->queryId != UINT64CONST(0),
CTAS is collected as a utility_statement without planning counter.
This seems to me respectig the rule, not sure that this needs any
new (risky) change to the actual (quite stable) patch.





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


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Julien Rouhaud
On Sun, Mar 1, 2020 at 3:55 PM legrand legrand
<[hidden email]> wrote:

>
> >> I like the idea of adding a check for a non-zero queryId in the new
> >> pgss_planner_hook() (zero queryid shouldn't be reserved for
> >> utility_statements ?).
>
> > Some assert hit later, I can say that it's not always true.  For
> > instance a CREATE TABLE AS won't run parse analysis for the underlying
> > query, as this has already been done for the original statement, but
> > will still call the planner.  I'll change pgss_planner_hook to ignore
> > such cases, as pgss_store would otherwise think that it's a utility
> > statement.  That'll probably incidentally fix the IVM incompatibility.
>
> Today with or without test on parse->queryId != UINT64CONST(0),
> CTAS is collected as a utility_statement without planning counter.
> This seems to me respectig the rule, not sure that this needs any
> new (risky) change to the actual (quite stable) patch.

But the queryid ends up not being computed the same way:

# select queryid, query, plans, calls from pg_stat_statements where
query like 'create table%';
       queryid       |             query              | plans | calls
---------------------+--------------------------------+-------+-------
 8275950546884151007 | create table test as select 1; |     1 |     0
 7546197440584636081 | create table test as select 1  |     0 |     1
(2 rows)

That's because CreateTableAsStmt->query doesn't have a query
location/len, as transformTopLevelStmt is only setting that for the
top level Query.  That's probably an oversight in ab1f0c82257, but I'm
not sure what's the best way to fix that.  Should we pass that
information to all transformXXX function, or let transformTopLevelStmt
handle that.


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

legrand legrand
Julien Rouhaud wrote
> On Sun, Mar 1, 2020 at 3:55 PM legrand legrand
> &lt;

> legrand_legrand@

> &gt; wrote:
>>
>> >> I like the idea of adding a check for a non-zero queryId in the new
>> >> pgss_planner_hook() (zero queryid shouldn't be reserved for
>> >> utility_statements ?).
>>
>> > Some assert hit later, I can say that it's not always true.  For
>> > instance a CREATE TABLE AS won't run parse analysis for the underlying
>> > query, as this has already been done for the original statement, but
>> > will still call the planner.  I'll change pgss_planner_hook to ignore
>> > such cases, as pgss_store would otherwise think that it's a utility
>> > statement.  That'll probably incidentally fix the IVM incompatibility.
>>
>> Today with or without test on parse->queryId != UINT64CONST(0),
>> CTAS is collected as a utility_statement without planning counter.
>> This seems to me respectig the rule, not sure that this needs any
>> new (risky) change to the actual (quite stable) patch.
>
> But the queryid ends up not being computed the same way:
>
> # select queryid, query, plans, calls from pg_stat_statements where
> query like 'create table%';
>        queryid       |             query              | plans | calls
> ---------------------+--------------------------------+-------+-------
>  8275950546884151007 | create table test as select 1; |     1 |     0
>  7546197440584636081 | create table test as select 1  |     0 |     1
> (2 rows)
>
> That's because CreateTableAsStmt->query doesn't have a query
> location/len, as transformTopLevelStmt is only setting that for the
> top level Query.  That's probably an oversight in ab1f0c82257, but I'm
> not sure what's the best way to fix that.  Should we pass that
> information to all transformXXX function, or let transformTopLevelStmt
> handle that.


arf, this was not the case in my testing env (that is not up to date) :o(
and would not have appeared at all with the proposed test on
parse->queryId != UINT64CONST(0) ...




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


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Julien Rouhaud
On Mon, Mar 2, 2020 at 1:01 PM legrand legrand
<[hidden email]> wrote:

>
> Julien Rouhaud wrote
> > On Sun, Mar 1, 2020 at 3:55 PM legrand legrand
> > &lt;
>
> > legrand_legrand@
>
> > &gt; wrote:
> >>
> >> >> I like the idea of adding a check for a non-zero queryId in the new
> >> >> pgss_planner_hook() (zero queryid shouldn't be reserved for
> >> >> utility_statements ?).
> >>
> >> > Some assert hit later, I can say that it's not always true.  For
> >> > instance a CREATE TABLE AS won't run parse analysis for the underlying
> >> > query, as this has already been done for the original statement, but
> >> > will still call the planner.  I'll change pgss_planner_hook to ignore
> >> > such cases, as pgss_store would otherwise think that it's a utility
> >> > statement.  That'll probably incidentally fix the IVM incompatibility.
> >>
> >> Today with or without test on parse->queryId != UINT64CONST(0),
> >> CTAS is collected as a utility_statement without planning counter.
> >> This seems to me respectig the rule, not sure that this needs any
> >> new (risky) change to the actual (quite stable) patch.
> >
> > But the queryid ends up not being computed the same way:
> >
> > # select queryid, query, plans, calls from pg_stat_statements where
> > query like 'create table%';
> >        queryid       |             query              | plans | calls
> > ---------------------+--------------------------------+-------+-------
> >  8275950546884151007 | create table test as select 1; |     1 |     0
> >  7546197440584636081 | create table test as select 1  |     0 |     1
> > (2 rows)
> >
> > That's because CreateTableAsStmt->query doesn't have a query
> > location/len, as transformTopLevelStmt is only setting that for the
> > top level Query.  That's probably an oversight in ab1f0c82257, but I'm
> > not sure what's the best way to fix that.  Should we pass that
> > information to all transformXXX function, or let transformTopLevelStmt
> > handle that.
>
>
> arf, this was not the case in my testing env (that is not up to date) :o(
> and would not have appeared at all with the proposed test on
> parse->queryId != UINT64CONST(0) ...

I'm not sure what was the exact behavior you had, but that shouldn't
have changed since previous version.  The underlying query isn't a top
level statement, so maybe you didn't set pg_stat_statements.track =
'all'?


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

legrand legrand
Never mind ...

Please consider PG13 shortest path ;o)

My one is  parse->queryId != UINT64CONST(0) in pgss_planner_hook().
It fixes IVM problem (verified),
and keep CTAS equal to pgss without planning counters (verified too).

Regards
PAscal






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


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Julien Rouhaud
On Thu, Mar 05, 2020 at 01:26:19PM -0700, legrand legrand wrote:
> Please consider PG13 shortest path ;o)
>
> My one is  parse->queryId != UINT64CONST(0) in pgss_planner_hook().
> It fixes IVM problem (verified),
> and keep CTAS equal to pgss without planning counters (verified too).

I still disagree that hiding this problem is the right fix, but since no one
objected here's a v5 with that behavior.  Hopefully this will be fixed in v14.

v5-0001-Pass-query-string-to-the-planner.patch (10K) Download Attachment
v5-0002-Add-planning-counters-to-pg_stat_statements.patch (46K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: Planning counters in pg_stat_statements (using pgss_store)

imai.yoshikazu@fujitsu.com
Hi Julien,

On Mon, Mar 9, 2020 at 10:32 AM, Julien Rouhaud wrote:
> On Thu, Mar 05, 2020 at 01:26:19PM -0700, legrand legrand wrote:
> > Please consider PG13 shortest path ;o)
> >
> > My one is  parse->queryId != UINT64CONST(0) in pgss_planner_hook().
> > It fixes IVM problem (verified),
> > and keep CTAS equal to pgss without planning counters (verified too).
>
> I still disagree that hiding this problem is the right fix, but since no one
> objected here's a v5 with that behavior.  Hopefully this will be fixed in v14.

Is there any case that query_text will be NULL when executing pg_plan_query?
If query_text will be NULL, we need to add codes to avoid errors in
pgss_store like as current patch. If query_text will not be NULL, we should
add Assert in pg_plan_query so that other developers can notice that they
would not mistakenly set query_text as NULL even without using pgss_planning
counter.

--
Yoshikazu Imai



Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Julien Rouhaud
Hi Imai-san,

On Thu, Mar 12, 2020 at 05:28:38AM +0000, [hidden email] wrote:

> Hi Julien,
>
> On Mon, Mar 9, 2020 at 10:32 AM, Julien Rouhaud wrote:
> > On Thu, Mar 05, 2020 at 01:26:19PM -0700, legrand legrand wrote:
> > > Please consider PG13 shortest path ;o)
> > >
> > > My one is  parse->queryId != UINT64CONST(0) in pgss_planner_hook().
> > > It fixes IVM problem (verified),
> > > and keep CTAS equal to pgss without planning counters (verified too).
> >
> > I still disagree that hiding this problem is the right fix, but since no one
> > objected here's a v5 with that behavior.  Hopefully this will be fixed in v14.
>
> Is there any case that query_text will be NULL when executing pg_plan_query?
With current sources, there are no cases where the query text isn't provided
AFAICS.

> If query_text will be NULL, we need to add codes to avoid errors in
> pgss_store like as current patch. If query_text will not be NULL, we should
> add Assert in pg_plan_query so that other developers can notice that they
> would not mistakenly set query_text as NULL even without using pgss_planning
> counter.

I totally agree.  I already had such assert locally, and regression tests pass
without any problem.  I'm attaching a v6 with that extra assert.  If the
first patch is committed, it'll now be a requirement to provide it.  Or if
people think it's not, it'll make sure that we'll discuss it.

v6-0001-Pass-query-string-to-the-planner.patch (10K) Download Attachment
v6-0002-Add-planning-counters-to-pg_stat_statements.patch (46K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: Planning counters in pg_stat_statements (using pgss_store)

imai.yoshikazu@fujitsu.com
On Thu, Mar 12, 2020 at 6:31 AM, Julien Rouhaud wrote:

> On Thu, Mar 12, 2020 at 05:28:38AM +0000, [hidden email] wrote:
> > Hi Julien,
> >
> > On Mon, Mar 9, 2020 at 10:32 AM, Julien Rouhaud wrote:
> > > On Thu, Mar 05, 2020 at 01:26:19PM -0700, legrand legrand wrote:
> > > > Please consider PG13 shortest path ;o)
> > > >
> > > > My one is  parse->queryId != UINT64CONST(0) in pgss_planner_hook().
> > > > It fixes IVM problem (verified),
> > > > and keep CTAS equal to pgss without planning counters (verified too).
> > >
> > > I still disagree that hiding this problem is the right fix, but since no one
> > > objected here's a v5 with that behavior.  Hopefully this will be fixed in v14.
> >
> > Is there any case that query_text will be NULL when executing pg_plan_query?
>
> With current sources, there are no cases where the query text isn't provided
> AFAICS.
>
> > If query_text will be NULL, we need to add codes to avoid errors in
> > pgss_store like as current patch. If query_text will not be NULL, we should
> > add Assert in pg_plan_query so that other developers can notice that they
> > would not mistakenly set query_text as NULL even without using pgss_planning
> > counter.
>
> I totally agree.  I already had such assert locally, and regression tests pass
> without any problem.  I'm attaching a v6 with that extra assert.  If the
> first patch is committed, it'll now be a requirement to provide it.  Or if
> people think it's not, it'll make sure that we'll discuss it.

I see. I also don't come up with any case of query_text is NULL. Now we need
other people's opinion about here.


I'll summary code review of this thread.

[Performance]

If track_planning is not enabled, performance will drop 0.2-0.6% which can be
ignored. If track_planning is enabled, performance will drop 0-2.2%. 2.2% is a
bit large but I think it is still acceptable because people using this feature
might take account that some overhead will happen for additional calling of a
gettime function.

https://www.postgresql.org/message-id/CY4PR20MB12227E5CE199FFBB90C68A13BCB40%40CY4PR20MB1222.namprd20.prod.outlook.com

[Values in each row]

* Rows for planner time are added as {total/min/max/mean/stddev}_plan_time.

  These are enough statistics for users who want to investigate the
  planning time.

* Rows for executor time are changed from {total/min/max/mean/stddev}_time
to {total/min/max/mean/stddev}_exec_time.

  Because of changing the name of the rows, there's no backward compatibility.
  Thus some users needs to modify scripts which using previous version of the
  pg_stat_statements. I believe it is not expensive to rewrite scripts along
  this change and it would be better to give an appropriate name to a row
  for future users.
  I also haven't seen big opposition about losing backward compatibility so
  far.

* We don't provide {total/min/max/mean/stddev}_time.

  Users can calculate total_time as total_plan_time + total_exec_time on their
  own. About {min/max/mean/stddev}_time, it will not make much sense
  because it is not ensured that executor follows planner and each counter
  value will be different largely between planner and executor.

* bufusage still only counts the buffer usage during executor.

  Now we have the ability to count the buffer usage during planner but we keep
  the bufusage count the buffer usage during executor for now.

[Coding]

* We add Assert in pg_plan_query so that query_text will not be NULL when
executing planner.

  There's no case query_text will be NULL with current sources. It is not
  ensured there will be any case query_text will be possibly NULL in the
  future though. Some considerations are needed by other people about this.


I don't have any other comments for now. After looking patches over again and
if there are no other comments about this patch, I'll set this patch as ready
for committer for getting more opinion.

--
Yoshikazu Imai



Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Julien Rouhaud
On Thu, Mar 12, 2020 at 10:19 AM [hidden email]
<[hidden email]> wrote:
>
> I'll summary code review of this thread.

Thanks for the summary!  I just have some minor comments

> [Performance]
>
> If track_planning is not enabled, performance will drop 0.2-0.6% which can be
> ignored. If track_planning is enabled, performance will drop 0-2.2%. 2.2% is a
> bit large but I think it is still acceptable because people using this feature
> might take account that some overhead will happen for additional calling of a
> gettime function.
>
> https://www.postgresql.org/message-id/CY4PR20MB12227E5CE199FFBB90C68A13BCB40%40CY4PR20MB1222.namprd20.prod.outlook.com
>
> [Values in each row]
>
> * bufusage still only counts the buffer usage during executor.
>
>   Now we have the ability to count the buffer usage during planner but we keep
>   the bufusage count the buffer usage during executor for now.

The bufusage should reflect the sum of planning and execution usage if
track_planning is enabled.  Did I miss something there?

> [Coding]
>
> * We add Assert in pg_plan_query so that query_text will not be NULL when
> executing planner.
>
>   There's no case query_text will be NULL with current sources. It is not
>   ensured there will be any case query_text will be possibly NULL in the
>   future though. Some considerations are needed by other people about this.

There's at least the current version of IVM patchset that lacks the
querytext.  Looking at various extensions, I see that pg_background
and pglogical call pg_plan_query internally but shouldn't have any
issue providing the query text.  But there's also citus extension,
which don't keep around the query string at least when distributing
plans, which makes sense since it's of no use and they're heavily
modifying the original Query.  I think that citus folks opinion on the
subject would be useful, so I'm Cc-ing Marco.


123456