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