Date: Mon, 15 Apr 2024 18:09:29 +0900 (JST)
Message-Id: <20240415.180929.517481101782400391.kou@clear-code.com>
To: 9erthalion6@gmail.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
Subject: Re: pg_stat_statements and "IN" conditions
From: Sutou Kouhei <kou@clear-code.com>
In-Reply-To: <20240404143514.a26f7ttxrbdfc73a@erthalion.local>
References: <20240326205916.aqxffqt7vgqjmnhm@erthalion.local>
	<CAKmOUTkbC4oEBxZAP5=d30AcxjYHte0b+7JpGVNeB217erwa6A@mail.gmail.com>
	<20240404143514.a26f7ttxrbdfc73a@erthalion.local>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/20240415.180929.517481101782400391.kou%40clear-code.com>
Precedence: bulk

Hi,

In <20240404143514.a26f7ttxrbdfc73a@erthalion.local>
  "Re: pg_stat_statements and "IN" conditions" on Thu, 4 Apr 2024 16:35:14 +0200,
  Dmitry Dolgov <9erthalion6@gmail.com> wrote:

> Here is the rebased version.

Thanks. I'm not familiar with this code base but I've
reviewed these patches because I'm interested in this
feature too.

0001:

> diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
> index be823a7f8fa..e9473def361 100644
> --- a/src/backend/nodes/queryjumblefuncs.c
> +++ b/src/backend/nodes/queryjumblefuncs.c
> 
> @@ -212,15 +233,67 @@ RecordConstLocation(JumbleState *jstate, int location)
> ...
> +static bool
> +IsMergeableConstList(List *elements, Const **firstConst, Const **lastConst)
> +{
> +	ListCell   *temp;
> +	Node	   *firstExpr = NULL;
> +
> +	if (elements == NULL)

"elements == NIL" will be better for List.

> +static void
> +_jumbleElements(JumbleState *jstate, List *elements)
> +{
> +	Const *first, *last;
> +	if(IsMergeableConstList(elements, &first, &last))

A space is missing between "if" and "(".

> diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
> index aa727e722cc..cf4f900d4ed 100644
> --- a/src/include/nodes/primnodes.h
> +++ b/src/include/nodes/primnodes.h
> @@ -1333,7 +1333,7 @@ typedef struct ArrayExpr
>  	/* common type of array elements */
>  	Oid			element_typeid pg_node_attr(query_jumble_ignore);
>  	/* the array elements or sub-arrays */
> -	List	   *elements;
> +	List	   *elements pg_node_attr(query_jumble_merge);

Should we also update the pg_node_attr() comment for
query_jumble_merge in nodes.h?


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?

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


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


Thanks,
-- 
kou