Re: Planning counters in pg_stat_statements (using pgss_store)

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.