Horton

Re: pg_stat_statements and "IN" conditions

Dmitry Dolgov <9erthalion6@gmail.com>

View thread
From: Dmitry Dolgov <9erthalion6@gmail.com>
To: Sutou Kouhei <kou@clear-code.com>
Cc: yasuo.honda@gmail.com, tgl@sss.pgh.pa.us, smithpb2250@gmail.com, vignesh21@gmail.com, michael@paquier.xyz, nathandbossart@gmail.com, stark.cfm@gmail.com, geidav.pg@gmail.com, sk@zsrv.org, alvherre@alvh.no-ip.org, marcos@f10.com.br, robertmhaas@gmail.com, david@pgmasters.net, pgsql-hackers@postgresql.org, pavel.trukhanov@gmail.com
Date: 2024-04-23T08:18:15Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Introduce squashing of constant lists in query jumbling

  2. Make documentation builds reproducible

  3. Include values of A_Const nodes in query jumbling

  4. Teach planner about more monotonic window functions

  5. Split up guc.c for better build speed and ease of maintenance.

> On Mon, Apr 15, 2024 at 06:09:29PM +0900, Sutou Kouhei wrote:
>
> Thanks. I'm not familiar with this code base but I've
> reviewed these patches because I'm interested in this
> feature too.

Thanks for the review! The commentaries for the first patch make sense
to me, will apply.

> 0003:
>
> > diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
> > index d7841b51cc9..00eec30feb1 100644
> > --- a/contrib/pg_stat_statements/pg_stat_statements.c
> > +++ b/contrib/pg_stat_statements/pg_stat_statements.c
> > ...
> > @@ -2883,12 +2886,22 @@ generate_normalized_query(JumbleState *jstate, const char *query,
> >  		/* The firsts merged constant */
> >  		else if (!skip)
> >  		{
> > +			static const uint32 powers_of_ten[] = {
> > +				1, 10, 100,
> > +				1000, 10000, 100000,
> > +				1000000, 10000000, 100000000,
> > +				1000000000
> > +			};
> > +			int lower_merged = powers_of_ten[magnitude - 1];
> > +			int upper_merged = powers_of_ten[magnitude];
>
> How about adding a reverse function of decimalLength32() to
> numutils.h and use it here?

I was pondering that at some point, but eventually decided to keep it
this way, because:

* the use case is quite specific, I can't image it's being used anywhere
  else

* it would not be strictly reverse, as the transformation itself is not
  reversible in the strict sense

> > -			n_quer_loc += sprintf(norm_query + n_quer_loc, "...");
> > +			n_quer_loc += sprintf(norm_query + n_quer_loc, "... [%d-%d entries]",
> > +								  lower_merged, upper_merged - 1);
>
> Do we still have enough space in norm_query for this change?
> It seems that norm_query expects up to 10 additional bytes
> per jstate->clocations[i].

As far as I understand there should be enough space, because we're going
to replace at least 10 constants with one merge record. But it's a good
point, this should be called out in the commentary explaining why 10
additional bytes are added.

> It seems that we can merge 0001, 0003 and 0004 to one patch.
> (Sorry. I haven't read all discussions yet. If we already
> discuss this, sorry for this noise.)

There is a certain disagreement about which portion of this feature
makes sense to go with first, thus I think keeping all options open is a
good idea. In the end a committer can squash the patches if needed.