Feedback-ID: ia2694551:Fastmail
Date: Tue, 18 Mar 2025 19:33:25 +0100
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: Julien Rouhaud <rjuju123@gmail.com>, 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: <202503181833.mhpoqscwxfw3@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ityjgjeekboyjwpzkbtqyrcopi7hdrh4blwcux6rft7sux3qj5@tufruueqvorn>
Archived-At: 
 <https://www.postgresql.org/message-id/202503181833.mhpoqscwxfw3%40alvherre.pgsql>
Precedence: bulk

On 2025-Mar-18, Dmitry Dolgov wrote:

> Thanks, much appreciated! I've inspected the diff between patches and
> run few tests, at the first glance everything looks fine.

Thanks for looking once more.

> > I am tempted to say that explicit casts should also be considered
> > squashable (that is, in IsSquashableConst() also allow the case of
> > func->funcformat == COERCE_EXPLICIT_CAST).
> 
> Well, I admit I may have been burned too much by the initial reception
> of the patch and handled it too conservatively in this regard.

I can totally understand that.  I have added this and pushed.  Hopefully
nobody will hate this too much, and some people might even like it.

By the way, I'm still open to adding the 'powers' mechanism that was
discussed earlier and other simple additions on top of what's there now,
if you have some spare energy to spend on this.  (For full disclosure:
the "powers" thing was discussed in a developer's meeting last year, and
several people said they'd prefer to stick with 0001 and forget about
powers, on the grounds that it produced query texts that were weird and
invalid SQL.  But now that we have the commented-out syntax, it's no
longer invalid SQL, and maybe it's not so weird either.)

But there are other proposals such as handling Params and whatnot.  At
this point, what we'd need for that is a more extensive test suite to
show that we're not breaking other things ...


> Agree, when put together with the OID limitation it doesn't look so bad.
> Somehow I was thinking about the Sami's proposal and the discussion in more
> abstract terms,

Yeah, that happened to me too, and then I checked again and realized
that my initial impression was wrong.

> as if we talk about any arbitrary mutable functions to squash -- I
> still would be cautious about hiding non-bootstrapped mutable
> functions.

Yeah, absolutely.


One thing I noticed while going over the code, is that we say in some
code comments that the list of elements only contributes the first and
last elements to the jumble.  But this is not true -- the list actually
contributes _nothing at all_ to the jumble.  I don't think this causes
any terrible problems, but maybe somebody will discover that I'm wrong
on that.  This isn't trivial to solve, because if you try to add
anything to the jumble from there, you'd break the first/last location
pair matching.  We could maybe fix this by returning the actual
bottommost Const node from IsSquashableConstList() instead of whatever
is wrapping it, and then arrange for _jumbleConst() to receive a boolean
that turns off jumbling of the location.

However, contributing nothing already makes such a query different from
another query that has exactly one element, because that one jumbles
that element.  It could only be confused (in the sense of identical
query_ids) with another list that has zero elements.

Anyway, something to play with.


BTW, it's fun to execute a query that's literally
  select col from tab where col in (1 /*, ... */);
and then
  select col from tab where col in (1, 2);

because now you have two entries in pg_stat_statements with visibly the
same query text, but two different query_ids.  I'm not terribly worried
about this, because who uses a literal "/*, ... */" in a query anyway?
And even if they do, it's easily explained.  But jesters could probably
get a good laugh messing about with these reports.

Thanks for keeping at this for so long!

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