Feedback-ID: ia2694551:Fastmail
Date: Thu, 13 Feb 2025 13:47:01 +0100
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: Sami Imseih <samimseih@gmail.com>,
	Kirill Reshke <reshkekirill@gmail.com>, Sergei Kornilov <sk@zsrv.org>,
 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,
 marcos@f10.com.br,
	robertmhaas@gmail.com, david@pgmasters.net, pgsql-hackers@postgresql.org,
	pavel.trukhanov@gmail.com, Sutou Kouhei <kou@clear-code.com>
Subject: Re: pg_stat_statements and "IN" conditions
Message-ID: <202502131247.zls4jgcl2yqe@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <qvotbnl7ycigw3txl5kcbts4nxycucggnixmfqidlfmx3nnpjw@dxw6duw35dno>
Archived-At: 
 <https://www.postgresql.org/message-id/202502131247.zls4jgcl2yqe%40alvherre.pgsql>
Precedence: bulk

On 2025-Feb-13, Dmitry Dolgov wrote:

> Here is how it looks like (posting only the first patch, since we
> concentrate on it). This version handles just a little more to cover
> simpe cases like the implicit convertion above. The GUC is also moved
> out from pgss and renamed to query_id_merge_values. On top I've added
> more tests showing the impact, as well as sometimes awkward looking
> normalized query I was talking about. I'm going to experiment how to
> iron out the latter.

Thanks!  It's looking better.  Some small comments -- please add the new
GUC to postgresql.conf.sample.  Also, how wed are you to
"query_id_merge_values" as a name?  It's not in any way obvious that
this is about values in arrays.  How about query_id_squash_arrays?  Or
are you thinking in having values in other types of structures squashed
as well, and that this first patch does it for arrays only but you want
the GUC to also control some future feature?

(I think I prefer "squash" here as a verb to "merge").

> +static bool
> +IsMergeableConst(Node *element)
> +{
> +	if (IsA(element, RelabelType))
> +		element = (Node *) ((RelabelType *) element)->arg;
> +
> +	if (IsA(element, CoerceViaIO))
> +		element = (Node *) ((CoerceViaIO *) element)->arg;
> +
> +	if(IsA(element, FuncExpr))
> +	{
> +		FuncExpr *func = (FuncExpr *) element;
> +		char provolatile = func_volatile(func->funcid);

I think calling func_volatile potentially once per array element is not
good; this might cause dozens/thousands of identical syscache lookups.
Maybe we can pass an initially NIL list from IsMergeableConstList (as
List **), which IsMergeableConst fills with OIDs of functions that have
been checked and found acceptable.  Then the second time around we
search the list first and only do func_volatile() after not finding a
match.


Another thing I didn't quite understand is why you did this rather
baroque-looking list scan:

> +static bool
> +IsMergeableConstList(List *elements, Node **firstExpr, Node **lastExpr)
> +{
> +	ListCell   *temp;
> +	Node	   *firstElem = NULL;
> +
> +	if (elements == NIL)
> +		return false;
> +
> +	if (!query_id_merge_values)
> +	{
> +		/* Merging is disabled, process everything one by one */
> +		return false;
> +	}
> +
> +	firstElem = linitial(elements);
> +
> +	/*
> +	 * If the first expression is a constant, verify if the following elements
> +	 * are constants as well. If yes, the list is eligible for merging.
> +	 */
> +	if (IsMergeableConst(firstElem))
> +	{
> +		foreach(temp, elements)
> +		{
> +			Node *element = lfirst(temp);
> +
> +			if (!IsMergeableConst(element))
> +				return false;
> +		}
> +
> +		*firstExpr = firstElem;
> +		*lastExpr = llast(elements);
> +		return true;
> +	}

Why not just scan the list in the straightforward way, that is

 foreach(temp, elements)
 {
    if (!IsMergeableConst(lfirst(temp)))
       return false;
 }
 *firstExpr = linitial(elements);
 *lastExpr = llast(elements);
 return true;

Is there something being optimized here specifically for the first
element?  I don't see it.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/