Re: Planning counters in pg_stat_statements (using pgss_store)

From: Julien Rouhaud <rjuju123(at)gmail(dot)com>
To: legrand legrand <legrand_legrand(at)hotmail(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Planning counters in pg_stat_statements (using pgss_store)
Date: 2020-03-14 17:27:33
Message-ID: 20200314172733.mg7qpyumlyythm25@nol
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Sat, Mar 14, 2020 at 03:04:00AM -0700, legrand legrand wrote:
> imai(dot)yoshikazu(at)fujitsu(dot)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).

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

Attachment Content-Type Size
v7-0001-Pass-query-string-to-the-planner.patch text/x-diff 10.4 KB
v7-0002-Add-planning-counters-to-pg_stat_statements.patch text/x-diff 45.8 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Paul A Jungwirth 2020-03-14 17:34:44 Re: range_agg
Previous Message Tomas Vondra 2020-03-14 16:56:10 Re: Additional improvements to extended statistics