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)

Marco Slot
On Thu, Mar 12, 2020 at 11:31 AM Julien Rouhaud <[hidden email]> wrote:
> 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.

Most of the time we keep our Query * data structures in a form that
can be deparsed back into a query string by a modified copy of
ruleutils.c, so we could generate a correct query string if absolutely
necessary, though there are performance-sensitive cases where we'd
rather not have the deparsing overhead.

In case of INSERT..SELECT into a distributed table, we might call
pg_plan_query on the SELECT part (Query *) and send the output into a
DestReceiver that sends tuples into shards of the distributed table
via COPY. The fact that SELECT does not show up in pg_stat_statements
separately is generally fine because it's an implementation detail,
and it would probably be a little confusing because the user never ran
the SELECT query. Moreover, the call to pg_plan_query would already be
reflected in the planning or execution time of the top-level query, so
it would be double counted if it had its own entry.

Another case is when some of the shards turn out to be local to the
server that handles the distributed query. In that case we plan the
queries on those shards via pg_plan_query instead of deparsing and
sending the query string to a remote server. It would be less
confusing for these queries to show in pg_stat_statements, because the
queries on the shards on remote servers will show up as well. However,
this is a performance-sensitive code path where we'd rather avoid
deparsing.

In general, I'd prefer if there was no requirement to pass a correct
query string. I'm ok with passing "SELECT 'citus_internal'" or just ""
if that does not lead to issues. Passing NULL to signal that the
planner call should not be tracked separately does seem a bit cleaner.

Marco


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 1:11 PM Marco Slot <[hidden email]> wrote:

>
> On Thu, Mar 12, 2020 at 11:31 AM Julien Rouhaud <[hidden email]> wrote:
> > 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.
>
> Most of the time we keep our Query * data structures in a form that
> can be deparsed back into a query string by a modified copy of
> ruleutils.c, so we could generate a correct query string if absolutely
> necessary, though there are performance-sensitive cases where we'd
> rather not have the deparsing overhead.

Yes, deparsing is probably too expensive for that use case.

> In case of INSERT..SELECT into a distributed table, we might call
> pg_plan_query on the SELECT part (Query *) and send the output into a
> DestReceiver that sends tuples into shards of the distributed table
> via COPY. The fact that SELECT does not show up in pg_stat_statements
> separately is generally fine because it's an implementation detail,
> and it would probably be a little confusing because the user never ran
> the SELECT query. Moreover, the call to pg_plan_query would already be
> reflected in the planning or execution time of the top-level query, so
> it would be double counted if it had its own entry.

Well, surprising statements can already appears in pg_stat_statements
when you use some psql features, or if you have triggers as those will
run additional queries under the hood.

The difference here is that since citus is a CustomNode, underlying
calls to planner will be accounted for that node, and that's indeed
annoying.  I can see that citus is doing some calls to spi_exec or
Executor* (in ExecuteLocalTaskPlan), which could also trigger
pg_stat_statements, but I don't know if a queryid is present there.

> Another case is when some of the shards turn out to be local to the
> server that handles the distributed query. In that case we plan the
> queries on those shards via pg_plan_query instead of deparsing and
> sending the query string to a remote server. It would be less
> confusing for these queries to show in pg_stat_statements, because the
> queries on the shards on remote servers will show up as well. However,
> this is a performance-sensitive code path where we'd rather avoid
> deparsing.

Agreed.

> In general, I'd prefer if there was no requirement to pass a correct
> query string. I'm ok with passing "SELECT 'citus_internal'" or just ""
> if that does not lead to issues. Passing NULL to signal that the
> planner call should not be tracked separately does seem a bit cleaner.

That's very interesting feedback, thanks!  I'm not a fan of giving a
way for queries to say that they want to be ignored by
pg_stat_statements, but double counting the planning counters seem
even worse, so I'm +0.5 to accept NULL query string in the planner,
incidentally making pgss ignore those.


Reply | Threaded
Open this post in threaded view
|

RE: Planning counters in pg_stat_statements (using pgss_store)

imai.yoshikazu@fujitsu.com
In reply to this post by Julien Rouhaud
On Thu, Mar 12, 2020 at 10:31 AM, Julien Rouhaud wrote:
> > * 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?

Ah, you're right. I somehow misunderstood it. Sorry for the annoying.

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

I saw IVM patchset but I thought it is difficult to impose them to give appropriate
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.

Thank you for looking those codes. I will comment about this in another mail.

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

RE: Planning counters in pg_stat_statements (using pgss_store)

imai.yoshikazu@fujitsu.com
In reply to this post by Julien Rouhaud
On Thu, Mar 12, 2020 at 6:37 PM, Julien Rouhaud wrote:

> On Thu, Mar 12, 2020 at 1:11 PM Marco Slot <[hidden email]> wrote:
> > On Thu, Mar 12, 2020 at 11:31 AM Julien Rouhaud <[hidden email]>
> wrote:
> > > 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.
> >
> > Most of the time we keep our Query * data structures in a form that
> > can be deparsed back into a query string by a modified copy of
> > ruleutils.c, so we could generate a correct query string if absolutely
> > necessary, though there are performance-sensitive cases where we'd
> > rather not have the deparsing overhead.
>
> Yes, deparsing is probably too expensive for that use case.
>
> > In case of INSERT..SELECT into a distributed table, we might call
> > pg_plan_query on the SELECT part (Query *) and send the output into a
> > DestReceiver that sends tuples into shards of the distributed table
> > via COPY. The fact that SELECT does not show up in pg_stat_statements
> > separately is generally fine because it's an implementation detail,
> > and it would probably be a little confusing because the user never ran
> > the SELECT query. Moreover, the call to pg_plan_query would already be
> > reflected in the planning or execution time of the top-level query, so
> > it would be double counted if it had its own entry.
>
> Well, surprising statements can already appears in pg_stat_statements when
> you use some psql features, or if you have triggers as those will run additional
> queries under the hood.
>
> The difference here is that since citus is a CustomNode, underlying calls to
> planner will be accounted for that node, and that's indeed annoying.  I can see
> that citus is doing some calls to spi_exec or
> Executor* (in ExecuteLocalTaskPlan), which could also trigger
> pg_stat_statements, but I don't know if a queryid is present there.
>
> > Another case is when some of the shards turn out to be local to the
> > server that handles the distributed query. In that case we plan the
> > queries on those shards via pg_plan_query instead of deparsing and
> > sending the query string to a remote server. It would be less
> > confusing for these queries to show in pg_stat_statements, because the
> > queries on the shards on remote servers will show up as well. However,
> > this is a performance-sensitive code path where we'd rather avoid
> > deparsing.
>
> Agreed.
>
> > In general, I'd prefer if there was no requirement to pass a correct
> > query string. I'm ok with passing "SELECT 'citus_internal'" or just ""
> > if that does not lead to issues. Passing NULL to signal that the
> > planner call should not be tracked separately does seem a bit cleaner.
>
> That's very interesting feedback, thanks!  I'm not a fan of giving a way for
> queries to say that they want to be ignored by pg_stat_statements, but double
> counting the planning counters seem even worse, so I'm +0.5 to accept NULL
> query string in the planner, incidentally making pgss ignore those.

It is preferable that we can count various queries statistics as much as possible
but if it causes overhead even when without using pg_stat_statements, we would
not have to force them to set appropriate query_text.
About settings a fixed string in query_text, I think it doesn't make much sense
because users can't take any actions by seeing those queries' stats. Moreover, if
we set a fixed string in query_text to avoid pg_stat_statement's errors, codes
would be inexplicable for other developers who don't know there's such
requirements.
After all, I agree accepting NULL query string in the planner.

I don't know it is useful but there are also codes that avoid an error when
sourceText is NULL.

executor_errposition(EState *estate, int location)
{
    ...
    /* Can't do anything if source text is not available */
    if (estate == NULL || estate->es_sourceText == NULL)
}

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

RE: Planning counters in pg_stat_statements (using pgss_store)

legrand legrand
[hidden email] wrote
> On Thu, Mar 12, 2020 at 6:37 PM, Julien Rouhaud wrote:
>> On Thu, Mar 12, 2020 at 1:11 PM Marco Slot &lt;

> marco.slot@

> &gt; wrote:
>> > On Thu, Mar 12, 2020 at 11:31 AM Julien Rouhaud &lt;

> rjuju123@

> &gt;
>> wrote:
>> > > 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.
>> >
>> > Most of the time we keep our Query * data structures in a form that
>> > can be deparsed back into a query string by a modified copy of
>> > ruleutils.c, so we could generate a correct query string if absolutely
>> > necessary, though there are performance-sensitive cases where we'd
>> > rather not have the deparsing overhead.
>>
>> Yes, deparsing is probably too expensive for that use case.
>>
>> > In case of INSERT..SELECT into a distributed table, we might call
>> > pg_plan_query on the SELECT part (Query *) and send the output into a
>> > DestReceiver that sends tuples into shards of the distributed table
>> > via COPY. The fact that SELECT does not show up in pg_stat_statements
>> > separately is generally fine because it's an implementation detail,
>> > and it would probably be a little confusing because the user never ran
>> > the SELECT query. Moreover, the call to pg_plan_query would already be
>> > reflected in the planning or execution time of the top-level query, so
>> > it would be double counted if it had its own entry.
>>
>> Well, surprising statements can already appears in pg_stat_statements
>> when
>> you use some psql features, or if you have triggers as those will run
>> additional
>> queries under the hood.
>>
>> The difference here is that since citus is a CustomNode, underlying calls
>> to
>> planner will be accounted for that node, and that's indeed annoying.  I
>> can see
>> that citus is doing some calls to spi_exec or
>> Executor* (in ExecuteLocalTaskPlan), which could also trigger
>> pg_stat_statements, but I don't know if a queryid is present there.
>>
>> > Another case is when some of the shards turn out to be local to the
>> > server that handles the distributed query. In that case we plan the
>> > queries on those shards via pg_plan_query instead of deparsing and
>> > sending the query string to a remote server. It would be less
>> > confusing for these queries to show in pg_stat_statements, because the
>> > queries on the shards on remote servers will show up as well. However,
>> > this is a performance-sensitive code path where we'd rather avoid
>> > deparsing.
>>
>> Agreed.
>>
>> > In general, I'd prefer if there was no requirement to pass a correct
>> > query string. I'm ok with passing "SELECT 'citus_internal'" or just ""
>> > if that does not lead to issues. Passing NULL to signal that the
>> > planner call should not be tracked separately does seem a bit cleaner.
>>
>> That's very interesting feedback, thanks!  I'm not a fan of giving a way
>> for
>> queries to say that they want to be ignored by pg_stat_statements, but
>> double
>> counting the planning counters seem even worse, so I'm +0.5 to accept
>> NULL
>> query string in the planner, incidentally making pgss ignore those.
>
> It is preferable that we can count various queries statistics as much as
> possible
> but if it causes overhead even when without using pg_stat_statements, we
> would
> not have to force them to set appropriate query_text.
> About settings a fixed string in query_text, I think it doesn't make much
> sense
> because users can't take any actions by seeing those queries' stats.
> Moreover, if
> we set a fixed string in query_text to avoid pg_stat_statement's errors,
> codes
> would be inexplicable for other developers who don't know there's such
> requirements.
> After all, I agree accepting NULL query string in the planner.
>
> I don't know it is useful but there are also codes that avoid an error
> when
> sourceText is NULL.
>
> executor_errposition(EState *estate, int location)
> {
>     ...
>     /* Can't do anything if source text is not available */
>     if (estate == NULL || estate->es_sourceText == NULL)
> }
>
> --
> Yoshikazu Imai

Hello Imai,

My understanding of V5 patch, that checks for Non-Zero queryid,
manage properly case where sourceText is NULL.

A NULL sourceText means that there was no Parsing for the associated
query, if there was no Parsing, there is no queryid (queryId=0),
and no planning counters update.

It doesn't change pg_plan_query behaviour (no regression for Citus, IVM,
...),
and was tested with success for IVM.

If my understanding is wrong, then setting track_planning = false
would still be the work arround for the very rare (no core) extension(s)
that may hit the NULL query text assertion failure.

What do you think about this ?
Would this make V5 patch ready for committers ?

Thanks in advance.
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)

legrand legrand
> I don't know it is useful but there are also codes that avoid an error when
> sourceText is NULL.

> executor_errposition(EState *estate, int location)
> {
>     ...
>    /* Can't do anything if source text is not available */
>    if (estate == NULL || estate->es_sourceText == NULL)
> }


or maybe would you prefer to replace the Non-Zero queryid test
by Non-NULL sourcetext one ?



--
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
In reply to this post by legrand legrand
On Sat, Mar 14, 2020 at 03:04:00AM -0700, legrand legrand wrote:

> [hidden email] wrote
> > On Thu, Mar 12, 2020 at 6:37 PM, Julien Rouhaud wrote:
> >> That's very interesting feedback, thanks!  I'm not a fan of giving a way
> >> for
> >> queries to say that they want to be ignored by pg_stat_statements, but
> >> double
> >> counting the planning counters seem even worse, so I'm +0.5 to accept
> >> NULL
> >> query string in the planner, incidentally making pgss ignore those.
> >
> > It is preferable that we can count various queries statistics as much as
> > possible
> > but if it causes overhead even when without using pg_stat_statements, we
> > would
> > not have to force them to set appropriate query_text.
> > About settings a fixed string in query_text, I think it doesn't make much
> > sense
> > because users can't take any actions by seeing those queries' stats.
> > Moreover, if
> > we set a fixed string in query_text to avoid pg_stat_statement's errors,
> > codes
> > would be inexplicable for other developers who don't know there's such
> > requirements.
> > After all, I agree accepting NULL query string in the planner.
> >
> > I don't know it is useful but there are also codes that avoid an error
> > when
> > sourceText is NULL.
> >
> > executor_errposition(EState *estate, int location)
> > {
> >     ...
> >     /* Can't do anything if source text is not available */
> >     if (estate == NULL || estate->es_sourceText == NULL)

I'm wondering if that's really possible.  But pgss uses the QueryDesc, which
should always have a query text (since pgss relies on that).


> My understanding of V5 patch, that checks for Non-Zero queryid,
> manage properly case where sourceText is NULL.
>
> A NULL sourceText means that there was no Parsing for the associated
> query, if there was no Parsing, there is no queryid (queryId=0),
> and no planning counters update.
>
> It doesn't change pg_plan_query behaviour (no regression for Citus, IVM,
> ...),
> and was tested with success for IVM.
>
> If my understanding is wrong, then setting track_planning = false
> would still be the work arround for the very rare (no core) extension(s)
> that may hit the NULL query text assertion failure.
>
> What do you think about this ?

I don't think that's a correct assumption.  I obviously didn't read all of
citus extension, but it looks like what's happening is that they get generate a
custom Query from the original one, with all the modification needed for
distributed execution and whatnot, which is then fed to the planner.  I think
it's entirely mossible that the modified Query herits from a previously set
queryid, while still not really having a query text.  And if citus doesn't do
that, it doesn't seem like an illegal use cuse anyway.

I'm instead attaching a v7 which removes the assert in pg_plan_query, and
modify pgss_planner_hook to also ignore queries without a query text, as this
seems the best option.

v7-0001-Pass-query-string-to-the-planner.patch (10K) Download Attachment
v7-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 Sat, Mar 14, 2020 at 5:28 PM, Julien Rouhaud wrote:

> On Sat, Mar 14, 2020 at 03:04:00AM -0700, legrand legrand wrote:
> > [hidden email] wrote
> > > On Thu, Mar 12, 2020 at 6:37 PM, Julien Rouhaud wrote:
> > >> That's very interesting feedback, thanks!  I'm not a fan of giving a way
> > >> for
> > >> queries to say that they want to be ignored by pg_stat_statements, but
> > >> double
> > >> counting the planning counters seem even worse, so I'm +0.5 to accept
> > >> NULL
> > >> query string in the planner, incidentally making pgss ignore those.
> > >
> > > It is preferable that we can count various queries statistics as much as
> > > possible
> > > but if it causes overhead even when without using pg_stat_statements, we
> > > would
> > > not have to force them to set appropriate query_text.
> > > About settings a fixed string in query_text, I think it doesn't make much
> > > sense
> > > because users can't take any actions by seeing those queries' stats.
> > > Moreover, if
> > > we set a fixed string in query_text to avoid pg_stat_statement's errors,
> > > codes
> > > would be inexplicable for other developers who don't know there's such
> > > requirements.
> > > After all, I agree accepting NULL query string in the planner.
> > >
> > > I don't know it is useful but there are also codes that avoid an error
> > > when
> > > sourceText is NULL.
> > >
> > > executor_errposition(EState *estate, int location)
> > > {
> > >     ...
> > >     /* Can't do anything if source text is not available */
> > >     if (estate == NULL || estate->es_sourceText == NULL)
>
>
> I'm wondering if that's really possible.  But pgss uses the QueryDesc, which
> should always have a query text (since pgss relies on that).

I cited those codes because I just wanted to say there's already an assumption
that query text in QueryDesc would be NULL, whether or not it is true.


> > My understanding of V5 patch, that checks for Non-Zero queryid,
> > manage properly case where sourceText is NULL.
> >
> > A NULL sourceText means that there was no Parsing for the associated
> > query, if there was no Parsing, there is no queryid (queryId=0),
> > and no planning counters update.
> >
> > It doesn't change pg_plan_query behaviour (no regression for Citus, IVM,
> > ...),
> > and was tested with success for IVM.
> >
> > If my understanding is wrong, then setting track_planning = false
> > would still be the work arround for the very rare (no core) extension(s)
> > that may hit the NULL query text assertion failure.
> >
> > What do you think about this ?
>
>
> I don't think that's a correct assumption.  I obviously didn't read all of
> citus extension, but it looks like what's happening is that they get generate a
> custom Query from the original one, with all the modification needed for
> distributed execution and whatnot, which is then fed to the planner.  I think
> it's entirely mossible that the modified Query herits from a previously set
> queryid, while still not really having a query text.  And if citus doesn't do
> that, it doesn't seem like an illegal use cuse anyway.

Indeed. It can happen that queryid has some value while query_text is NULL.


> I'm instead attaching a v7 which removes the assert in pg_plan_query, and
> modify pgss_planner_hook to also ignore queries without a query text, as this
> seems the best option.

Thank you.
It also seems to me that is the best option.

BTW, I recheck the patchset.
I think codes are ready for committer but should we modify the documentation?
{min,max,mean,stddev}_time is now obsoleted so it is better to modify it to
{min,max,mean,stddev}_exec_time and add {min,max,mean,stddev}_plan_time.


--
Yoshikazu Imai



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
> I'm instead attaching a v7 which removes the assert in pg_plan_query, and
> modify pgss_planner_hook to also ignore queries without a query text, as
> this
> seems the best option.

Ok, it was the second solution, go on !



--
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
In reply to this post by imai.yoshikazu@fujitsu.com
On Mon, Mar 16, 2020 at 01:34:11AM +0000, [hidden email] wrote:

> On Sat, Mar 14, 2020 at 5:28 PM, Julien Rouhaud wrote:
> > I don't think that's a correct assumption.  I obviously didn't read all of
> > citus extension, but it looks like what's happening is that they get generate a
> > custom Query from the original one, with all the modification needed for
> > distributed execution and whatnot, which is then fed to the planner.  I think
> > it's entirely mossible that the modified Query herits from a previously set
> > queryid, while still not really having a query text.  And if citus doesn't do
> > that, it doesn't seem like an illegal use cuse anyway.
>
> Indeed. It can happen that queryid has some value while query_text is NULL.
>
>
> > I'm instead attaching a v7 which removes the assert in pg_plan_query, and
> > modify pgss_planner_hook to also ignore queries without a query text, as this
> > seems the best option.
>
> Thank you.
> It also seems to me that is the best option.

Thanks Imai-san and PAscal for the feedback, it seems that we have an
agreement!


> BTW, I recheck the patchset.
> I think codes are ready for committer but should we modify the documentation?
> {min,max,mean,stddev}_time is now obsoleted so it is better to modify it to
> {min,max,mean,stddev}_exec_time and add {min,max,mean,stddev}_plan_time.


Oh indeed, I totally forgot about this.  I'm attaching v8 with updated
documentation that should match what was implemented since some versions.

v8-0001-Pass-query-string-to-the-planner.patch (10K) Download Attachment
v8-0002-Add-planning-counters-to-pg_stat_statements.patch (47K) 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 Mon, Mar 16, 2020 at 9:49 PM, Julien Rouhaud wrote:

> On Mon, Mar 16, 2020 at 01:34:11AM +0000, [hidden email]
> wrote:
> > On Sat, Mar 14, 2020 at 5:28 PM, Julien Rouhaud wrote:
> > > I don't think that's a correct assumption.  I obviously didn't read
> > > all of citus extension, but it looks like what's happening is that
> > > they get generate a custom Query from the original one, with all the
> > > modification needed for distributed execution and whatnot, which is
> > > then fed to the planner.  I think it's entirely mossible that the
> > > modified Query herits from a previously set queryid, while still not
> > > really having a query text.  And if citus doesn't do that, it doesn't seem like
> an illegal use cuse anyway.
> >
> > Indeed. It can happen that queryid has some value while query_text is NULL.
> >
> >
> > > I'm instead attaching a v7 which removes the assert in
> > > pg_plan_query, and modify pgss_planner_hook to also ignore queries
> > > without a query text, as this seems the best option.
> >
> > Thank you.
> > It also seems to me that is the best option.
>
>
> Thanks Imai-san and PAscal for the feedback, it seems that we have an
> agreement!
>
>
> > BTW, I recheck the patchset.
> > I think codes are ready for committer but should we modify the
> documentation?
> > {min,max,mean,stddev}_time is now obsoleted so it is better to modify
> > it to {min,max,mean,stddev}_exec_time and add
> {min,max,mean,stddev}_plan_time.
>
>
> Oh indeed, I totally forgot about this.  I'm attaching v8 with updated
> documentation that should match what was implemented since some
> versions.

Okay, I checked it.
So I'll mark this as a ready for committer.

Thanks
--
Yoshikazu Imai


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Sergei Kornilov
In reply to this post by Julien Rouhaud
Hello

I was inactive for a while... Sorry.

>>  BTW, I recheck the patchset.
>>  I think codes are ready for committer but should we modify the documentation?
>>  {min,max,mean,stddev}_time is now obsoleted so it is better to modify it to
>>  {min,max,mean,stddev}_exec_time and add {min,max,mean,stddev}_plan_time.
>
> Oh indeed, I totally forgot about this. I'm attaching v8 with updated
> documentation that should match what was implemented since some versions.

Yet another is missed in docs: total_time

I specifically verified that the new loaded library works with the old version of the extension in the database. I have not noticed issues here.

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

I will be happy even with 10% overhead due to enabled track_planning... (but in this case disabled by default) log_min_duration_statement = 0 with log parsing is much more expensive.
I think 1-2% is acceptable and we can set track_planning = on by default as patch does.

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

Maybe release it as 2.0 version instead of minor update 1.18?

regards, Sergei


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Julien Rouhaud
On Fri, Mar 20, 2020 at 05:09:05PM +0300, Sergei Kornilov wrote:
> Hello
>
> Yet another is missed in docs: total_time

Oh good catch!  I rechecked many time the field, and totally missed that the
documentation is referring to the view, which has an additional column, and not
the function.  Attached v9 fixes that.

> I specifically verified that the new loaded library works with the old version of the extension in the database. I have not noticed issues here.

Thanks for those extra checks.

> > 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.
>
> I will be happy even with 10% overhead due to enabled track_planning... (but in this case disabled by default) log_min_duration_statement = 0 with log parsing is much more expensive.
> I think 1-2% is acceptable and we can set track_planning = on by default as patch does.
>
> > * Rows for executor time are changed from {total/min/max/mean/stddev}_time to {total/min/max/mean/stddev}_exec_time.
>
> Maybe release it as 2.0 version instead of minor update 1.18?
I don't have an opinion on that, I'd be fine with any version.  I kept 1.18 in
the patch for now.

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

Re: Planning counters in pg_stat_statements (using pgss_store)

Fujii Masao-4


On 2020/03/21 4:30, Julien Rouhaud wrote:
> On Fri, Mar 20, 2020 at 05:09:05PM +0300, Sergei Kornilov wrote:
>> Hello
>>
>> Yet another is missed in docs: total_time
>
> Oh good catch!  I rechecked many time the field, and totally missed that the
> documentation is referring to the view, which has an additional column, and not
> the function.  Attached v9 fixes that.

Thanks for the patch! Here are the review comments from me.

- PGSS_V1_3
+ PGSS_V1_3,
+ PGSS_V1_8

WAL usage patch [1] increments this version to 1_4 instead of 1_8.
I *guess* that's because previously this version was maintained
independently from pg_stat_statements' version. For example,
pg_stat_statements 1.4 seems to have used PGSS_V1_3.

+ /*
+ * We can't process the query if no query_text is provided, as pgss_store
+ * needs it.  We also ignore query without queryid, as it would be treated
+ * as a utility statement, which may not be the case.
+ */

Could you tell me why the planning stats are not tracked when executing
utility statements? In some utility statements like REFRESH MATERIALIZED VIEW,
the planner would work.

+static BufferUsage
+compute_buffer_counters(BufferUsage start, BufferUsage stop)
+{
+ BufferUsage result;

BufferUsageAccumDiff() has very similar logic. Isn't it better to expose
and use that function rather than creating new similar function?

  values[i++] = Int64GetDatumFast(tmp.rows);
  values[i++] = Int64GetDatumFast(tmp.shared_blks_hit);
  values[i++] = Int64GetDatumFast(tmp.shared_blks_read);

Previously (without the patch) pg_stat_statements_1_3() reported
the buffer usage counters updated only in execution phase. But,
in the patched version, pg_stat_statements_1_3() reports the total
of buffer usage counters updated in both planning and execution
phases. Is this OK? I'm not sure how seriously we should ensure
the backward compatibility for pg_stat_statements....

+/* contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql */

ISTM it's good timing to have also pg_stat_statements--1.8.sql since
the definition of pg_stat_statements() is changed. Thought?

[1]
https://postgr.es/m/CAB-hujrP8ZfUkvL5OYETipQwA=e3n7oqHFU=4ZLxWS_Cza3kQQ@...

Regards,

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


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Julien Rouhaud
On Wed, Mar 25, 2020 at 10:09:37PM +0900, Fujii Masao wrote:

>
> On 2020/03/21 4:30, Julien Rouhaud wrote:
> > On Fri, Mar 20, 2020 at 05:09:05PM +0300, Sergei Kornilov wrote:
> > > Hello
> > >
> > > Yet another is missed in docs: total_time
> >
> > Oh good catch!  I rechecked many time the field, and totally missed that the
> > documentation is referring to the view, which has an additional column, and not
> > the function.  Attached v9 fixes that.
>
> Thanks for the patch! Here are the review comments from me.
>
> - PGSS_V1_3
> + PGSS_V1_3,
> + PGSS_V1_8
>
> WAL usage patch [1] increments this version to 1_4 instead of 1_8.
> I *guess* that's because previously this version was maintained
> independently from pg_stat_statements' version. For example,
> pg_stat_statements 1.4 seems to have used PGSS_V1_3.

Oh right.  It seems that I changed that many versions ago, I'm not sure why.
I'm personally fine with any, but I think this was previously raised and
consensus was to keep distinct counters.  Unless you prefer to keep it this
way, I'll send an updated version (with other possible modifications depending
on the rest of the mail) using PGSS_V1_4.

> + /*
> + * We can't process the query if no query_text is provided, as pgss_store
> + * needs it.  We also ignore query without queryid, as it would be treated
> + * as a utility statement, which may not be the case.
> + */
>
> Could you tell me why the planning stats are not tracked when executing
> utility statements? In some utility statements like REFRESH MATERIALIZED VIEW,
> the planner would work.

I explained that in [1].  The problem is that the underlying statement doesn't
get the proper stmt_location and stmt_len, so you eventually end up with two
different entries.  I suggested fixing transformTopLevelStmt() to handle the
various DDL that can contain optimisable statements, but everyone preferred to
postpone that for a future enhencement.

> +static BufferUsage
> +compute_buffer_counters(BufferUsage start, BufferUsage stop)
> +{
> + BufferUsage result;
>
> BufferUsageAccumDiff() has very similar logic. Isn't it better to expose
> and use that function rather than creating new similar function?

Oh, I thought this wouldn't be acceptable.  That's indeed better so I'll do
that instead.

>   values[i++] = Int64GetDatumFast(tmp.rows);
>   values[i++] = Int64GetDatumFast(tmp.shared_blks_hit);
>   values[i++] = Int64GetDatumFast(tmp.shared_blks_read);
>
> Previously (without the patch) pg_stat_statements_1_3() reported
> the buffer usage counters updated only in execution phase. But,
> in the patched version, pg_stat_statements_1_3() reports the total
> of buffer usage counters updated in both planning and execution
> phases. Is this OK? I'm not sure how seriously we should ensure
> the backward compatibility for pg_stat_statements....

That's indeed a behavior change, although the new behavior is probably better
as user want to know how much resource a query is consuming overall.  We could
distinguish all buffers with a plan/exec version, but it seems quite overkill.

> +/* contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql */
>
> ISTM it's good timing to have also pg_stat_statements--1.8.sql since
> the definition of pg_stat_statements() is changed. Thought?

I thought that since CreateExtension() was modified to be able to find it's way
automatically, we shouldn't provide base version anymore, to minimize
maintenance burden and also avoid possible bug/discrepancy.  The only drawback
is that it'll do multiple CREATE or DROP/CREATE of the function usually once
per database, which doesn't seem like a big problem.

[1] https://www.postgresql.org/message-id/CAOBaU_Y-y+VOhTZgDOuDk6-9V72-ZXdWccXo_kx0P4DDBEEh9A@...


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Sergei Kornilov
In reply to this post by Fujii Masao-4
Hello

> WAL usage patch [1] increments this version to 1_4 instead of 1_8.
> I *guess* that's because previously this version was maintained
> independently from pg_stat_statements' version. For example,
> pg_stat_statements 1.4 seems to have used PGSS_V1_3.

As far as I remember, this was my proposed change in review a year ago.
I think that having a clear analogy between the extension version and the function name would be more clear than sequential numbering of PGSS_V with different extension versions.
For pgss 1.4 it was fine to use PGSS_V1_3, because there were no changes in pg_stat_statements_internal.
pg_stat_statements 1.3 will call pg_stat_statements_1_3
pg_stat_statements 1.4 - 1.7 will still call pg_stat_statements_1_3. In my opinion, this is the correct naming, since we did not need a new function.
but pg_stat_statements 1.8 will call pg_stat_statements_1_4. It's not confusing?

Well, no strong opinion.

regards, Sergei


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Fujii Masao-4


On 2020/03/26 2:17, Sergei Kornilov wrote:

> Hello
>
>> WAL usage patch [1] increments this version to 1_4 instead of 1_8.
>> I *guess* that's because previously this version was maintained
>> independently from pg_stat_statements' version. For example,
>> pg_stat_statements 1.4 seems to have used PGSS_V1_3.
>
> As far as I remember, this was my proposed change in review a year ago.
> I think that having a clear analogy between the extension version and the function name would be more clear than sequential numbering of PGSS_V with different extension versions.
> For pgss 1.4 it was fine to use PGSS_V1_3, because there were no changes in pg_stat_statements_internal.
> pg_stat_statements 1.3 will call pg_stat_statements_1_3
> pg_stat_statements 1.4 - 1.7 will still call pg_stat_statements_1_3. In my opinion, this is the correct naming, since we did not need a new function.
> but pg_stat_statements 1.8 will call pg_stat_statements_1_4. It's not confusing?

Yeah, I withdraw my comment and agree that 1_8 looks less confusing.

Regards,

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


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Fujii Masao-4
In reply to this post by Julien Rouhaud


On 2020/03/25 22:45, Julien Rouhaud wrote:

> On Wed, Mar 25, 2020 at 10:09:37PM +0900, Fujii Masao wrote:
>>
>> On 2020/03/21 4:30, Julien Rouhaud wrote:
>>> On Fri, Mar 20, 2020 at 05:09:05PM +0300, Sergei Kornilov wrote:
>>>> Hello
>>>>
>>>> Yet another is missed in docs: total_time
>>>
>>> Oh good catch!  I rechecked many time the field, and totally missed that the
>>> documentation is referring to the view, which has an additional column, and not
>>> the function.  Attached v9 fixes that.
>>
>> Thanks for the patch! Here are the review comments from me.
>>
>> - PGSS_V1_3
>> + PGSS_V1_3,
>> + PGSS_V1_8
>>
>> WAL usage patch [1] increments this version to 1_4 instead of 1_8.
>> I *guess* that's because previously this version was maintained
>> independently from pg_stat_statements' version. For example,
>> pg_stat_statements 1.4 seems to have used PGSS_V1_3.
>
> Oh right.  It seems that I changed that many versions ago, I'm not sure why.
> I'm personally fine with any, but I think this was previously raised and
> consensus was to keep distinct counters.  Unless you prefer to keep it this
> way, I'll send an updated version (with other possible modifications depending
> on the rest of the mail) using PGSS_V1_4.
>
>> + /*
>> + * We can't process the query if no query_text is provided, as pgss_store
>> + * needs it.  We also ignore query without queryid, as it would be treated
>> + * as a utility statement, which may not be the case.
>> + */
>>
>> Could you tell me why the planning stats are not tracked when executing
>> utility statements? In some utility statements like REFRESH MATERIALIZED VIEW,
>> the planner would work.
>
> I explained that in [1].  The problem is that the underlying statement doesn't
> get the proper stmt_location and stmt_len, so you eventually end up with two
> different entries.

It's not problematic to have two different entries in that case. Right?
The actual problem is that the statements reported in those entries are
very similar? For example, when "create table test as select 1;" is executed,
it's strange to get the following two entries, as you explained.

     create table test as select 1;
     create table test as select 1

But it seems valid to get the following two entries in that case?

     select 1
     create table test as select 1

The former is the nested statement and the latter is the top statement.

> I suggested fixing transformTopLevelStmt() to handle the
> various DDL that can contain optimisable statements, but everyone preferred to
> postpone that for a future enhencement.

Understood. Thanks for explanation!

>> +static BufferUsage
>> +compute_buffer_counters(BufferUsage start, BufferUsage stop)
>> +{
>> + BufferUsage result;
>>
>> BufferUsageAccumDiff() has very similar logic. Isn't it better to expose
>> and use that function rather than creating new similar function?
>
> Oh, I thought this wouldn't be acceptable.  That's indeed better so I'll do
> that instead.

Thanks! But of course this is trivial thing, so it's ok to do that later.

>>   values[i++] = Int64GetDatumFast(tmp.rows);
>>   values[i++] = Int64GetDatumFast(tmp.shared_blks_hit);
>>   values[i++] = Int64GetDatumFast(tmp.shared_blks_read);
>>
>> Previously (without the patch) pg_stat_statements_1_3() reported
>> the buffer usage counters updated only in execution phase. But,
>> in the patched version, pg_stat_statements_1_3() reports the total
>> of buffer usage counters updated in both planning and execution
>> phases. Is this OK? I'm not sure how seriously we should ensure
>> the backward compatibility for pg_stat_statements....
>
> That's indeed a behavior change, although the new behavior is probably better
> as user want to know how much resource a query is consuming overall.  We could
> distinguish all buffers with a plan/exec version, but it seems quite overkill.

Ok.

>
>> +/* contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql */
>>
>> ISTM it's good timing to have also pg_stat_statements--1.8.sql since
>> the definition of pg_stat_statements() is changed. Thought?
>
> I thought that since CreateExtension() was modified to be able to find it's way
> automatically, we shouldn't provide base version anymore, to minimize
> maintenance burden and also avoid possible bug/discrepancy.  The only drawback
> is that it'll do multiple CREATE or DROP/CREATE of the function usually once
> per database, which doesn't seem like a big problem.

Ok.

Here are other comments.

- if (jstate)
+ if (kind == PGSS_JUMBLE)

Why is PGSS_JUMBLE necessary? ISTM that we can still use jstate here, instead.

If it's ok to remove PGSS_JUMBLE, we can define PGSS_NUM_KIND(=2) instead
and replace 2 in, e.g., total_time[2] with PGSS_NUM_KIND. Thought?

+      <entry><structfield>total_time</structfield></entry>
+      <entry><type>double precision</type></entry>
+      <entry></entry>
+      <entry>
+        Total time spend planning and executing the statement, in milliseconds
+      </entry>
+     </row>

pg_stat_statements view has this column but the function not.
We should make both have the column or not at all, for consistency?
I'm not sure if it's good thing to expose the sum of total_plan_time
and total_exec_time as total_time. If some users want that, they can
easily calculate it from total_plan_time and total_exec_time by using
their own logic.

+ nested_level++;
+ PG_TRY();

In old thread [1], Tom Lane commented the usage of nested_level
in the planner hook. There seems no reply to that so far. What's
your opinion about that comment?

[1] https://www.postgresql.org/message-id/28980.1515803777@...

Regards,

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


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Julien Rouhaud
On Thu, Mar 26, 2020 at 08:08:35PM +0900, Fujii Masao wrote:

>
> On 2020/03/25 22:45, Julien Rouhaud wrote:
> > On Wed, Mar 25, 2020 at 10:09:37PM +0900, Fujii Masao wrote:
> > > + /*
> > > + * We can't process the query if no query_text is provided, as pgss_store
> > > + * needs it.  We also ignore query without queryid, as it would be treated
> > > + * as a utility statement, which may not be the case.
> > > + */
> > >
> > > Could you tell me why the planning stats are not tracked when executing
> > > utility statements? In some utility statements like REFRESH MATERIALIZED VIEW,
> > > the planner would work.
> >
> > I explained that in [1].  The problem is that the underlying statement doesn't
> > get the proper stmt_location and stmt_len, so you eventually end up with two
> > different entries.
>
> It's not problematic to have two different entries in that case. Right?

I will unnecessarily bloat the entries, and makes users life harder too.  This
example is quite easy to deal with, but if the application is sending
multi-query statements, you'll just end up with a mess impossible to properly
handle.

> The actual problem is that the statements reported in those entries are
> very similar? For example, when "create table test as select 1;" is executed,
> it's strange to get the following two entries, as you explained.
>
>     create table test as select 1;
>     create table test as select 1
>
> But it seems valid to get the following two entries in that case?
>
>     select 1
>     create table test as select 1
>
> The former is the nested statement and the latter is the top statement.

I think that there should only be 1 entry, the utility command.  It seems easy
to correlate the planning time to the underlying query, but I'm not entirely
sure that the execution counters won't be impacted by the fact is being run in
a utilty statements.  Also, for now current pgss behavior is to always merge
underlying optimisable statements in the utility command, and it seems a bit
late in this release cycle to revisit that.

I'd be happy to work on improving that for the next release, among other
things.  For instance the total lack of normalization for utility commands [2]
is also something that has been bothering me for a long time.  In some
workloads, you can end up with the entries almost entirely filled with
1-time-execution commands, just because it's using random identifiers, so you
have no other choice than to disable track_utility, although it would have been
useful for other commands.

> Here are other comments.
>
> - if (jstate)
> + if (kind == PGSS_JUMBLE)
>
> Why is PGSS_JUMBLE necessary? ISTM that we can still use jstate here, instead.
>
> If it's ok to remove PGSS_JUMBLE, we can define PGSS_NUM_KIND(=2) instead
> and replace 2 in, e.g., total_time[2] with PGSS_NUM_KIND. Thought?

Yes, we could be using jstate here.  I originally used that to avoid passing
PGSS_EXEC (or the other one) as a way to say "ignore this information as
there's the jstate which says it's yet another meaning".  If that's not an
issue, I can change that as PGSS_NUM_KIND will clearly improve the explicit "2"
all over the place.

> +      <entry><structfield>total_time</structfield></entry>
> +      <entry><type>double precision</type></entry>
> +      <entry></entry>
> +      <entry>
> +        Total time spend planning and executing the statement, in milliseconds
> +      </entry>
> +     </row>
>
> pg_stat_statements view has this column but the function not.
> We should make both have the column or not at all, for consistency?
> I'm not sure if it's good thing to expose the sum of total_plan_time
> and total_exec_time as total_time. If some users want that, they can
> easily calculate it from total_plan_time and total_exec_time by using
> their own logic.

I think we originally added it as a way to avoid too much compatibility break,
and also because it seems like a field most users will be interested in anyway.
Now that I'm thinking about it again, I indeed think it was a mistake to have
that in view part only.  Not mainly for consistency, but for users who would be
interested in the total_time field while not wanting to pay the overhead of
retrieving the query text if they don't need it.  So I'll change that!

> + nested_level++;
> + PG_TRY();
>
> In old thread [1], Tom Lane commented the usage of nested_level
> in the planner hook. There seems no reply to that so far. What's
> your opinion about that comment?
>
> [1] https://www.postgresql.org/message-id/28980.1515803777@...

Oh thanks, I didn't noticed this part of the discussion.  I agree with Tom's
concern, and I think that having a specific nesting level variable for the
planner is the best workaround, so I'll implement that.

[2] https://twitter.com/fujii_masao/status/1242978261572837377


Reply | Threaded
Open this post in threaded view
|

Re: Planning counters in pg_stat_statements (using pgss_store)

Julien Rouhaud
On Thu, Mar 26, 2020 at 02:22:47PM +0100, Julien Rouhaud wrote:

> On Thu, Mar 26, 2020 at 08:08:35PM +0900, Fujii Masao wrote:
> >
> > Here are other comments.
> >
> > - if (jstate)
> > + if (kind == PGSS_JUMBLE)
> >
> > Why is PGSS_JUMBLE necessary? ISTM that we can still use jstate here, instead.
> >
> > If it's ok to remove PGSS_JUMBLE, we can define PGSS_NUM_KIND(=2) instead
> > and replace 2 in, e.g., total_time[2] with PGSS_NUM_KIND. Thought?
>
> Yes, we could be using jstate here.  I originally used that to avoid passing
> PGSS_EXEC (or the other one) as a way to say "ignore this information as
> there's the jstate which says it's yet another meaning".  If that's not an
> issue, I can change that as PGSS_NUM_KIND will clearly improve the explicit "2"
> all over the place.
Done, passing PGSS_PLAN when jumble is intended, with a comment saying that the
pgss_kind is ignored in that case.

> > +      <entry><structfield>total_time</structfield></entry>
> > +      <entry><type>double precision</type></entry>
> > +      <entry></entry>
> > +      <entry>
> > +        Total time spend planning and executing the statement, in milliseconds
> > +      </entry>
> > +     </row>
> >
> > pg_stat_statements view has this column but the function not.
> > We should make both have the column or not at all, for consistency?
> > I'm not sure if it's good thing to expose the sum of total_plan_time
> > and total_exec_time as total_time. If some users want that, they can
> > easily calculate it from total_plan_time and total_exec_time by using
> > their own logic.
>
> I think we originally added it as a way to avoid too much compatibility break,
> and also because it seems like a field most users will be interested in anyway.
> Now that I'm thinking about it again, I indeed think it was a mistake to have
> that in view part only.  Not mainly for consistency, but for users who would be
> interested in the total_time field while not wanting to pay the overhead of
> retrieving the query text if they don't need it.  So I'll change that!
Done

> > + nested_level++;
> > + PG_TRY();
> >
> > In old thread [1], Tom Lane commented the usage of nested_level
> > in the planner hook. There seems no reply to that so far. What's
> > your opinion about that comment?
> >
> > [1] https://www.postgresql.org/message-id/28980.1515803777@...
>
> Oh thanks, I didn't noticed this part of the discussion.  I agree with Tom's
> concern, and I think that having a specific nesting level variable for the
> planner is the best workaround, so I'll implement that.
Done.

I also exported BufferUsageAccumDiff as mentioned previously, as it seems
clearner and will avoid future useless code churn, and run pgindent.

v10 attached.

v10-0001-Pass-query-string-to-the-planner.patch (10K) Download Attachment
v10-0002-Add-planning-counters-to-pg_stat_statements.patch (51K) Download Attachment
123456