Horton

Thread

Commits

  1. Allow the planner-related functions and hook to accept the query string.

  1. Patch: to pass query string to pg_plan_query()

    legrand legrand <legrand_legrand@hotmail.com> — 2020-03-09T21:31:27Z

    Hello,

This is a call for committers, reviewers and users,
regarding "planning counters in pg_stat_statements"
patch [1] but not only.

Historically, this version of pg_stat_statements
with planning counters was performing 3 calls to 
pgss_store() for non utility statements in:
1 - pgss_post_parse_analyze (init entry with queryid 
    and store query text)
2 - pgss_planner_hook (to store planning counters)
3 - pgss_ExecutorEnd (to store execution counters)

Then a new version was proposed to remove one call 
to pgss_store() by adding the query string to the 
planner pg_plan_query():
1 - pgss_planner_hook (to store planning counters)
2 - pgss_ExecutorEnd (to store execution counters)

Many performances tests where performed concluding
that there is no impact on this subject.

Patch "to pass query string to the planner", could be 
committed by itself, and (maybe) used by other extensions.

If this was done, this new version of pgss with planning
counters could be committed as well, or even later 
(being used as a non core extension starting with pg13). 

So please give us your feedback regarding this patch 
"to pass query string to the planner", if you have other 
use cases, or any comment regarding core architecture.

note:
A problem was discovered during IVM testing,
because some queries without sql text where planned
without being parsed, finishing in pgss with a zero 
queryid.

A work arround is to set track_planning = false,
we have chosen to fix that in pgss by ignoring
zero queryid inside pgss_planner_hook.

Thanks in advance
Regards
PAscal
  
[1] " https://www.postgresql.org/message-id/20200309103142.GA45401%40nol
<planning counters in pg_stat_statements>  "



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

  2. Re: Patch: to pass query string to pg_plan_query()

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-03-26T13:54:35Z

    
On 2020/03/10 6:31, legrand legrand wrote:
> Hello,
> 
> This is a call for committers, reviewers and users,
> regarding "planning counters in pg_stat_statements"
> patch [1] but not only.

Does anyone object to this patch? I'm thinking to commit it separetely
at first before committing the planning_counter_in_pg_stat_statements
patch.

> Historically, this version of pg_stat_statements
> with planning counters was performing 3 calls to
> pgss_store() for non utility statements in:
> 1 - pgss_post_parse_analyze (init entry with queryid
>      and store query text)
> 2 - pgss_planner_hook (to store planning counters)
> 3 - pgss_ExecutorEnd (to store execution counters)
> 
> Then a new version was proposed to remove one call
> to pgss_store() by adding the query string to the
> planner pg_plan_query():

But pgss_store() still needs to be called three times even in
non-utility command if the query has constants. Right?

> 1 - pgss_planner_hook (to store planning counters)
> 2 - pgss_ExecutorEnd (to store execution counters)
> 
> Many performances tests where performed concluding
> that there is no impact on this subject.

Sounds good!

> Patch "to pass query string to the planner", could be
> committed by itself, and (maybe) used by other extensions.
> 
> If this was done, this new version of pgss with planning
> counters could be committed as well, or even later
> (being used as a non core extension starting with pg13).
> 
> So please give us your feedback regarding this patch
> "to pass query string to the planner", if you have other
> use cases, or any comment regarding core architecture.

*As far as I heard*, pg_hint_plan extension uses very tricky way to
extract query string in the planner hook. So this patch would be
very helpful to make pg_hint_plan avoid using such tricky way.

Regards,

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

  3. Re: Patch: to pass query string to pg_plan_query()

    Julien Rouhaud <rjuju123@gmail.com> — 2020-03-26T14:40:09Z

    On Thu, Mar 26, 2020 at 10:54:35PM +0900, Fujii Masao wrote:
> 
> On 2020/03/10 6:31, legrand legrand wrote:
> > Hello,
> > 
> > This is a call for committers, reviewers and users,
> > regarding "planning counters in pg_stat_statements"
> > patch [1] but not only.
> 
> Does anyone object to this patch? I'm thinking to commit it separetely
> at first before committing the planning_counter_in_pg_stat_statements
> patch.
> 
> > Historically, this version of pg_stat_statements
> > with planning counters was performing 3 calls to
> > pgss_store() for non utility statements in:
> > 1 - pgss_post_parse_analyze (init entry with queryid
> >      and store query text)
> > 2 - pgss_planner_hook (to store planning counters)
> > 3 - pgss_ExecutorEnd (to store execution counters)
> > 
> > Then a new version was proposed to remove one call
> > to pgss_store() by adding the query string to the
> > planner pg_plan_query():
> 
> But pgss_store() still needs to be called three times even in
> non-utility command if the query has constants. Right?

Yes indeed, this version is actually adding the 3rd pgss_store call.  Passing
the query string is a collateral requirement in case the entry disappeared
between post parse analysis and planning (which is quite possible with prepared
statements at least), as pgss will in this case fallback storing the as-is
query string, which is still better that no query text at all.

> > 1 - pgss_planner_hook (to store planning counters)
> > 2 - pgss_ExecutorEnd (to store execution counters)
> > 
> > Many performances tests where performed concluding
> > that there is no impact on this subject.
> 
> Sounds good!
> 
> > Patch "to pass query string to the planner", could be
> > committed by itself, and (maybe) used by other extensions.
> > 
> > If this was done, this new version of pgss with planning
> > counters could be committed as well, or even later
> > (being used as a non core extension starting with pg13).
> > 
> > So please give us your feedback regarding this patch
> > "to pass query string to the planner", if you have other
> > use cases, or any comment regarding core architecture.
> 
> *As far as I heard*, pg_hint_plan extension uses very tricky way to
> extract query string in the planner hook. So this patch would be
> very helpful to make pg_hint_plan avoid using such tricky way.

+1

  4. Re: Patch: to pass query string to pg_plan_query()

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-26T15:44:44Z

    Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> Does anyone object to this patch? I'm thinking to commit it separetely
> at first before committing the planning_counter_in_pg_stat_statements
> patch.

I took a quick look through v9-0001-Pass-query-string-to-the-planner.patch
and it's fine by me.  It also matches up with something I've wanted to
do for awhile, which is to make the query string available during
planning and execution so that we can produce error cursors for
run-time errors, when relevant.

(It's a little weird that the patch doesn't make standard_planner
actually *do* anything with the string, like say save it into
the PlannerInfo struct.  But that can come later I guess.)

Note that I wouldn't want to bet that all of these call sites always have
non-null query strings to pass; but probably most of the time they will.

			regards, tom lane

  5. Re: Patch: to pass query string to pg_plan_query()

    Julien Rouhaud <rjuju123@gmail.com> — 2020-03-26T16:45:41Z

    On Thu, Mar 26, 2020 at 11:44:44AM -0400, Tom Lane wrote:
> Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> > Does anyone object to this patch? I'm thinking to commit it separetely
> > at first before committing the planning_counter_in_pg_stat_statements
> > patch.
> 
> I took a quick look through v9-0001-Pass-query-string-to-the-planner.patch
> and it's fine by me.  It also matches up with something I've wanted to
> do for awhile, which is to make the query string available during
> planning and execution so that we can produce error cursors for
> run-time errors, when relevant.
> 
> (It's a little weird that the patch doesn't make standard_planner
> actually *do* anything with the string, like say save it into
> the PlannerInfo struct.  But that can come later I guess.)
> 
> Note that I wouldn't want to bet that all of these call sites always have
> non-null query strings to pass; but probably most of the time they will.

Surprinsingly, the whole regression tests pass flawlessly with an non-null
query string assert, but we did had some discussion about it.  The pending IVM
patch would break that assumption, same as some non trivial extensions like
citus (see
https://www.postgresql.org/message-id/flat/CAFMSG9HJQr%3DH8doWJOp%3DwqyKbVqxMLkk_Qu2KfpmkKvS-Xn7qQ%40mail.gmail.com#ab8ea541b8c8464f7b52ba6d8d480b7d
and later), so we didn't make it a hard requirement.

  6. Re: Patch: to pass query string to pg_plan_query()

    legrand legrand <legrand_legrand@hotmail.com> — 2020-03-27T17:27:52Z

    Tom Lane-2 wrote
> Fujii Masao &lt;

> masao.fujii@.nttdata

> &gt; writes:
>> Does anyone object to this patch? I'm thinking to commit it separetely
>> at first before committing the planning_counter_in_pg_stat_statements
>> patch.
> 
> I took a quick look through v9-0001-Pass-query-string-to-the-planner.patch
> and it's fine by me.  It also matches up with something I've wanted to
> do for awhile, which is to make the query string available during
> planning and execution so that we can produce error cursors for
> run-time errors, when relevant.
> 
> [...]
> 
> 			regards, tom lane


Great !
Good news ;o)

Regards
PAscal



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