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:
> > imai.yoshik...@fujitsu.com 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
