Re: pg_stat_statements oddity with track = all

On Tue, Mar 9, 2021 at 3:39 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Mon, Mar 08, 2021 at 06:03:59PM +0100, Magnus Hagander wrote:
> > On Sun, Mar 7, 2021 at 8:39 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > >
> > > Yes I was a bit worried about the possible extra text entry.  I kept things
> > > simple until now as the general opinion was that entries existing for both top
> > > level and nested level should be very rare so adding extra code and overhead to
> > > spare a few query texts wouldn't be worth it.
> > >
> > > I think that using a flag might be a bit expensive, as we would have to make
> > > sure that at least one of the possible two entries has it.  So if there are 2
> > > entries and the one with the flag is evicted, we would have to transfer the
> > > flag to the other one, and check the existence of the flag when allocatin a new
> > > entry.  And all of that has to be done holding an exclusive lock on pgss->lock.
> >
> > Yeah, we'd certainly want to minimize things. But what if they both
> > have the flag at that point? Then we wouldn't have to care on
> > eviction? But yes, for new allications we'd have to look up if the
> > query existed with the other value of the flag, and copy it over in
> > that case.
>
> I think that we might be able to handle that without a flag.  The only thing
> that would need to be done is when creating an entry, look for an existing
> entry with the opposite flag, and if there's simply use the same
> (query_offset, query_len) info.  This doesn't sound that expensive.

That's basically what I was trying to say :)


> The real pain point will be that the garbage collection phase
> will become way more expensive as it will now have to somehow maintain that
> knowledge, which will require additional lookups for each entry.  I'm a bit
> concerned about that, especially with the current heuristic to schedule garbage
> collection.  For now, need_qc_qtext says that we have to do it if the extent is
> more than 512 (B) * pgss_max.  This probably doesn't work well for people using
> ORM as they tend to generate gigantic SQL queries.

Right, the cost would be mostly on the GC  side. I've never done any
profiling to see how big of a thing that is in systems today -- have
you?


> If we implement query text deduplication, should we add another GUC for that
> "512" magic value so that people can minimize the gc overhead if they know they
> have gigantic queries, or simply don't mind bigger qtext file?
>
> > > Maybe having a new hash table (without the toplevel flag) for those query text
> > > might be better, or maybe pgss performance is already so terrible when you have
> > > to regularly evict entries that it wouldn't make any real difference.
> > >
> > > Should I try to add some extra code to make sure that we only store the query
> > > text once, or should I document that there might be duplicate, but we expect
> > > that to be very rare?
> >
> > If we expect it to be rare, I think it might be reasonable to just
> > document that. But do we really have a strong argument for it being
> > rare?
>
> I don't that think that anyone really had a strong argument, mostly gut
> feeling.  Note that pg_stat_kcache already implemented that toplevel flags, so
> if people are using that extension in a recent version they might have some
> figures to show.  I'll ping some people that I know are using it.

Great -- data always wins over gut feelings :)


> One good argument would be that gigantic queries generated by ORM should always
> be executed as top level statements.

Yes, that's true. And it probably holds as a more generic case as
well, that is the queries that are likely to show up both top-level
and lower-level are more likely to be relatively simple ones. (Except
for example during the development of functions/procs where they're
often executed top level as well to test etc, but that's not the most
important case to optimize for)


> I previously tried with the postgres regression tests, which clearly isn't a
> representative workload, and as far as I can see the vast majority of queries
> executed bost as top level and nested level are DDL implying recursion (e.g. a
> CREATE TABLE with underlying index creation).

I think the PostgreSQL regression tests are so far from a real world
workload that the input in this case has a value of exactly zero.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/