Re: Planning counters in pg_stat_statements (using pgss_store)
От | Julien Rouhaud |
---|---|
Тема | Re: Planning counters in pg_stat_statements (using pgss_store) |
Дата | |
Msg-id | 20200314172733.mg7qpyumlyythm25@nol обсуждение исходный текст |
Ответ на | RE: Planning counters in pg_stat_statements (using pgss_store) (legrand legrand <legrand_legrand@hotmail.com>) |
Ответы |
RE: Planning counters in pg_stat_statements (using pgss_store)
Re: Planning counters in pg_stat_statements (using pgss_store) |
Список | pgsql-hackers |
On Sat, Mar 14, 2020 at 03:04:00AM -0700, legrand legrand wrote: > imai.yoshikazu@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). > 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.
Вложения
В списке pgsql-hackers по дате отправления: