Feedback-ID: ia2694551:Fastmail
Date: Tue, 11 Feb 2025 19:51:43 +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: <202502111851.yfhldozu4c6y@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <zgvzpnya6xzil2ke6em6c5aaqx7epaad4t34dsrj6cfw5rvzkk@llgs5vjn5k53>
Archived-At: 
 <https://www.postgresql.org/message-id/202502111851.yfhldozu4c6y%40alvherre.pgsql>
Precedence: bulk

On 2025-Feb-11, Dmitry Dolgov wrote:

> > On Tue, Feb 11, 2025 at 10:49:59AM GMT, Sami Imseih wrote:
> > I have only looked at 0001, but I am wondering why
> > query_id_const_merge is a pg_stat_statements GUC
> > rather than a core GUC?
> 
> It was moved from being a core GUC into a pg_stat_statements GUC on the request
> from the reviewers. Community tries to prevent adding more and more core GUCs
> into PostgreSQL.

I understand, but I tend to disagree with the argument because it's okay
to simplify things that can be simplified, but if simplifying makes them
more complex, then it goes against the original desire for simplicity
anyway.  My impression is that it would be better to put it back.

(Otherwise, how do you document the behavior that pg_stat_activity
suddenly emits different query_ids than before because you installed
pg_stat_statement and enabled this feature?  It just doesn't make much
sense.)

> > I suppose this is telling us that detecting the case with consts wrapped
> > in casts is not really optional.  (Dmitry said this was supported at
> > early stages of the patch, and now I'm really curious about that
> > implementation because what IsMergeableConstList sees is a FuncExpr that
> > invokes the cast function for float8 to int4.)
> 
> Yes, those cases in question are usually FuncExpr. The original patch
> implementation used to handle that via simplifying the node with
> eval_const_expressions to figure out if the value we work with is a constant.
> This approach was marked as too risky by reviewers, as this could reach a lot
> of unexpected functionality in the mutator part.

Hmm, what about doing something much simpler, such as testing whether
there's just a CoerceViaIO/RelabelType around a Const or a one-parameter
function call of an immutable boostrap-OID function that has a Const as
argument, and trivial cases like that?  Something very very simple
that's going to catch the majority of cases without anything as complex
as a node walker.

Maybe something like statext_is_compatible_clause_internal() can be an
inspiration (and even that is far more complex than we need here.)

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"If it is not right, do not do it.
If it is not true, do not say it." (Marcus Aurelius, Meditations)