Subject: Re: pg_stat_statements and "IN" conditions

From: Álvaro Herrera <[email-masked]>
Date: Mon, 17 Mar 2025 20:05:42 +0100

On 2025-Feb-14, Dmitry Dolgov wrote:

> This should do it. The last patch for today, otherwise I'll probably add
> more bugs than features :)

Thank you.  I've spent some time with this patch in the last few days,
and I propose a few changes.  I renamed everything from "merge" to
"squash"; apart from that, it's mostly docs and code comments changes,
but I also removed the addition of a boolean argument to JUMBLE_LOCATION
which AFAICS is unnecessary, and did away with the business of checking
for function immutability.  I also changed the code layout of
generate_normalized_query(); no functional changes, I just reordered the
code blocks (which caused a couple of lines to appear repeated that
weren't before).

You can see my patch on top of yours here:
https://github.com/alvherre/postgres/commits/query_id_squash_values/
and the CI run here:
https://cirrus-ci.com/build/5660053472018432

In addition, here I attach the complete patch on top of current master.
Unless there's some opposition to this, I intend to push this tomorrow.

I have to admit that looking at this part of the test,

+SELECT * FROM test_squash_cast WHERE data IN
+       (1::int4::casttesttype, 2::int4::casttesttype, 3::int4::casttesttype,
+        4::int4::casttesttype, 5::int4::casttesttype, 6::int4::casttesttype,
+        7::int4::casttesttype, 8::int4::casttesttype, 9::int4::casttesttype,
+        10::int4::casttesttype, 11::int4::casttesttype);
+ id | data 
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+                       query                        | calls 
+----------------------------------------------------+-------
+ SELECT * FROM test_squash_cast WHERE data IN      +|     1
+         ($1 /*, ... */::int4::casttesttype)        | 
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t |     1
+(2 rows)

and

+SELECT * FROM test_squash WHERE id IN (1::oid, 2::oid, 3::oid, 4::oid, 5::oid, 6::oid, 7::oid, 8::oid, 9::oid);
+ id | data 
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+                           query                            | calls 
+------------------------------------------------------------+-------
+ SELECT * FROM test_squash WHERE id IN ($1 /*, ... */::oid) |     1

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).  That would also squash
queries such as this one:

+SELECT * FROM test_squash_bigint WHERE data IN
+   (1::bigint, 2::bigint, 3::bigint, 4::bigint, 5::bigint, 6::bigint,
+    7::bigint, 8::bigint, 9::bigint, 10::bigint, 11::bigint);
+ id | data 
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+                                      query                                       | calls 
+----------------------------------------------------------------------------------+-------
+ SELECT * FROM test_squash_bigint WHERE data IN                                  +|     1
+         ($1::bigint, $2::bigint, $3::bigint, $4::bigint, $5::bigint, $6::bigint,+| 
+          $7::bigint, $8::bigint, $9::bigint, $10::bigint, $11::bigint)           | 

I, frankly, see little argument for making a distinction here.  We can
still discuss whether we prefer it one way or the other; we don't need
that decision to prevent me from pushing the patch I here attach, I think.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"The saddest aspect of life right now is that science gathers knowledge faster
 than society gathers wisdom."  (Isaac Asimov)

---

From: Álvaro Herrera <[email-masked]>
Date: Mon, 17 Mar 2025 20:14:16 +0100

> https://cirrus-ci.com/build/5660053472018432

Heh, this blew up on bogus SGML markup :-(  Fixed and running again:
https://cirrus-ci.com/build/4822893680394240

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Once again, thank you and all of the developers for your hard work on
PostgreSQL.  This is by far the most pleasant management experience of
any database I've worked on."                             (Dan Harris)
http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php

---

From: Dmitry Dolgov <[email-masked]>
Date: Tue, 18 Mar 2025 10:45:14 +0100

> On Mon, Mar 17, 2025 at 08:14:16PM GMT, Álvaro Herrera wrote:

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

> 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. Originally I also had a
concern about normalized queries representation for explicit cast case, but it
was resolved by Julien's suggestion to switch to the /* ... */ format.

> I have to admit that I am leaning towards removing the immutability
> constraint.  The reason is that we already require the function to be
> boostrapped (due to the OID test) and to have implicit cast form, so
> that limits which functions are recognized; the only ones there that are
> not immutable are:
>
>          castsource          │        casttarget        │                 castfunc
> ─────────────────────────────┼──────────────────────────┼──────────────────────────────────────────
>  text                        │ regclass                 │ regclass(text)
>  character varying           │ regclass                 │ regclass(text)
>  date                        │ timestamp with time zone │ timestamptz(date)
>  time without time zone      │ time with time zone      │ timetz(time without time zone)
>  timestamp without time zone │ timestamp with time zone │ timestamptz(timestamp without time zone)

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, as if we talk about any arbitrary mutable functions to squash
-- I still would be cautious about hiding non-bootstrapped mutable functions.

---

From: Álvaro Herrera <[email-masked]>
Date: Tue, 18 Mar 2025 19:33:25 +0100

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.

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

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

---

From: Sami Imseih <[email-masked]>
Date: Tue, 18 Mar 2025 14:54:18 -0400

I want to mention that the current patch does not deal
with external parameters ( prepared statements ) [0] [1]. This could be a
follow-up, as it may need some further discussion. It is important to
address this case, IMO.

[0]
https://www.postgresql.org/message-id/CAA5RZ0uGfxXyzhp9N5nfsS%2BZqF5ngEMC3YtBPtLoeK8EPsjHbw%40mail.gmail.com

[1]
https://www.postgresql.org/message-id/blauoky77sash2qzrbnz6ilfyi7odtvxtdr4ifg4hq4bpqp2uk%406z5yjfsxtcnl

Regards,

Sami

---

From: Álvaro Herrera <[email-masked]>
Date: Tue, 18 Mar 2025 21:17:18 +0100

> follow-up, as it may need some further discussion. It is important to

Yes, I realize that.  Feel free to send a patch.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"E pur si muove" (Galileo Galilei)

---

From: Dmitry Dolgov <[email-masked]>
Date: Wed, 19 Mar 2025 08:31:15 +0100

> On Tue, Mar 18, 2025 at 07:33:25PM GMT, Álvaro Herrera wrote:
>

Yes, I'm planning to continue working on this topic, there are still
plenty of things that could be improved.

> Anyway, something to play with.

Yep, I don't see this as an immediate problem as well, but will do some
experiments with that.

---

From: Dmitry Dolgov <[email-masked]>
Date: Wed, 19 Mar 2025 08:32:55 +0100

> On Tue, Mar 18, 2025 at 02:54:18PM GMT, Sami Imseih wrote:

Sure, it's important and I'm planning to tackle this next. If you want,
we can collaborate on a patch for that.

---

From: Sami Imseih <[email-masked]>
Date: Sat, 22 Mar 2025 15:59:02 -0500

> we can collaborate on a patch for that.

I spent some time looking some more at this, and I believe all that needs
to be done is check for a PRAM node with a type of PARAM_EXTERN.

During planning the planner turns the Param into a Const during
eval_const_expressions_mutator.

If it's as simple as I think it is, I hope we can get this committed for 18.
If not, and a longer discussion is needed, a new thread can be started
for this.

--
Sami Imseih
Amazon Web Services (AWS)