Re: pg_stat_statements and "IN" conditions

From: Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
To: Sutou Kouhei <kou(at)clear-code(dot)com>
Cc: yasuo(dot)honda(at)gmail(dot)com, tgl(at)sss(dot)pgh(dot)pa(dot)us, smithpb2250(at)gmail(dot)com, vignesh21(at)gmail(dot)com, michael(at)paquier(dot)xyz, nathandbossart(at)gmail(dot)com, stark(dot)cfm(at)gmail(dot)com, geidav(dot)pg(at)gmail(dot)com, sk(at)zsrv(dot)org, alvherre(at)alvh(dot)no-ip(dot)org, marcos(at)f10(dot)com(dot)br, robertmhaas(at)gmail(dot)com, david(at)pgmasters(dot)net, pgsql-hackers(at)postgresql(dot)org, pavel(dot)trukhanov(at)gmail(dot)com
Subject: Re: pg_stat_statements and "IN" conditions
Date: 2024-04-23 08:18:15
Message-ID: 20240423081815.r4zvaluze274tui2@ddolgov.remote.csb
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Maxim Orlov 2024-04-23 08:23:41 POC: make mxidoff 64 bits
Previous Message Michael Paquier 2024-04-23 07:07:32 Re: Direct SSL connection with ALPN and HBA rules