Date: Thu, 26 Mar 2020 15:40:09 +0100
From: Julien Rouhaud <rjuju123@gmail.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: legrand legrand <legrand_legrand@hotmail.com>,
	pgsql-hackers@postgresql.org
Subject: Re: Patch: to pass query string to pg_plan_query()
Message-ID: <20200326144009.GC80836@nol>
References: <1583789487074-0.post@n3.nabble.com>
 <6ecbaaca-5a8e-5fe6-f7e1-a893e708bd7b@oss.nttdata.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6ecbaaca-5a8e-5fe6-f7e1-a893e708bd7b@oss.nttdata.com>
Precedence: bulk

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