Subject: pg_stat_statements and "IN" conditions

From: Dmitry Dolgov <[email-masked]>
Date: Wed, 12 Aug 2020 18:19:02 +0200

Hi

I would like to start another thread to follow up on [1], mostly to bump up the
topic. Just to remind, it's about how pg_stat_statements jumbling ArrayExpr in
queries like:

    SELECT something FROM table WHERE col IN (1, 2, 3, ...)

The current implementation produces different jumble hash for every different
number of arguments for essentially the same query. Unfortunately a lot of ORMs
like to generate these types of queries, which in turn leads to
pg_stat_statements pollution. Ideally we want to prevent this and have only one
record for such a query.

As the result of [1] I've identified two highlighted approaches to improve this
situation:

* Reduce the generated ArrayExpr to an array Const immediately, in cases where
  all the inputs are Consts.

* Make repeating Const to contribute nothing to the resulting hash.

I've tried to prototype both approaches to find out pros/cons and be more
specific. Attached patches could not be considered a completed piece of work,
but they seem to work, mostly pass the tests and demonstrate the point. I would
like to get some high level input about them and ideally make it clear what is
the preferred solution to continue with.

# Reducing ArrayExpr to an array Const

IIUC this requires producing a Const with ArrayType constvalue in
transformAExprIn for ScalarArrayOpExpr. This could be a general improvement,
since apparently it's being done later anyway. But it deals only with Const,
which leaves more on the table, e.g. Params and other similar types of
duplication we observe when repeating constants are wrapped into VALUES.

Another point here is that it's quite possible this approach will still require
corresponding changes in pg_stat_statements, since just preventing duplicates
to show also loses the information. Ideally we also need to have some
understanding how many elements are actually there and display it, e.g. in
cases when there is just one outlier query that contains a huge IN list.

# Contribute nothing to the hash

I guess there could be multiple ways of doing this, but the first idea I had in
mind is to skip jumbling when necessary. At the same time it can be implemented
more centralized for different types of queries (although in the attached patch
there are only Const & Values). In the simplest case we just identify sequence
of constants of the same type, which just ignores any other cases when stuff is
mixed. But I believe it's something that could be considered a rare corner case
and it's better to start with the simplest solution.

Having said that I believe the second approach of contributing nothing to the
hash sounds more appealing, but would love to hear other opinions.

[1]: https://www.postgresql.org/message-id/flat/CAF42k%3DJCfHMJtkAVXCzBn2XBxDC83xb4VhV7jU7enPnZ0CfEQQ%40mail.gmail.com

---

From: Dmitry Dolgov <[email-masked]>
Date: Wed, 18 Nov 2020 17:04:32 +0100

> On Wed, Aug 12, 2020 at 06:19:02PM +0200, Dmitry Dolgov wrote:
>

I've implemented the second approach mentioned above, this version was
tested on our test clusters for some time without visible issues. Will
create a CF item and would appreciate any feedback.

---

From: Chengxi Sun <[email-masked]>
Date: Wed, 09 Dec 2020 03:37:40 +0000

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            not tested

Hi, I did some test and it works well

---

From: Dmitry Dolgov <[email-masked]>
Date: Wed, 9 Dec 2020 19:49:44 +0100

> On Wed, Dec 09, 2020 at 03:37:40AM +0000, Chengxi Sun wrote:
>

Thanks for testing!

---

From: Dmitry Dolgov <[email-masked]>
Date: Sat, 26 Dec 2020 11:46:35 +0100

> On Wed, Nov 18, 2020 at 05:04:32PM +0100, Dmitry Dolgov wrote:

After more testing I found couple of things that could be improved,
namely in the presence of non-reducible constants one part of the query
was not copied into the normalized version, and this approach also could
be extended for Params. These are incorporated in the attached patch.

---

From: Zhihong Yu <[email-masked]>
Date: Sat, 26 Dec 2020 08:53:28 -0800

Hi,
A few comments.

+                           "After this number of duplicating constants
start to merge them.",

duplicating -> duplicate

+               foreach(lc, (List *) expr)
+               {
+                   Node * subExpr = (Node *) lfirst(lc);
+
+                   if (!IsA(subExpr, Const))
+                   {
+                       allConst = false;
+                       break;
+                   }
+               }

It seems the above foreach loop (within foreach(temp, (List *) node)) can
be preceded with a check that allConst is true. Otherwise the loop can be
skipped.

+                   if (currentExprIdx == pgss_merge_threshold - 1)
+                   {
+                       JumbleExpr(jstate, expr);
+
+                       /*
+                        * A const expr is already found, so JumbleExpr must
+                        * record it. Mark it as merged, it will be the
first
+                        * merged but still present in the statement query.
+                        */
+                       Assert(jstate->clocations_count > 0);
+                       jstate->clocations[jstate->clocations_count -
1].merged = true;
+                       currentExprIdx++;
+                   }

The above snippet occurs a few times. Maybe extract into a helper method.

Cheers

---

From: Dmitry Dolgov <[email-masked]>
Date: Tue, 5 Jan 2021 13:52:30 +0100

> On Sat, Dec 26, 2020 at 08:53:28AM -0800, Zhihong Yu wrote:
> Hi,
> A few comments.
>

Thanks for noticing. Now that I look at it closer I think it's the other
way around, the loop above checking constants for the first expression
is not really necessary.

> The above snippet occurs a few times. Maybe extract into a helper method.

Originally I was hesitant to extract it was because it's quite small
part of the code. But now I've realized that the part relevant to lists
is not really correct, which makes those bits even more different, so I
think it makes sense to leave it like that. What do you think?

---

From: Zhihong Yu <[email-masked]>
Date: Tue, 5 Jan 2021 07:51:42 -0800

Hi, Dmitry:

+   int         lastExprLenght = 0;

Did you mean to name the variable lastExprLenghth ?

w.r.t. extracting to helper method, the second and third if (currentExprIdx
== pgss_merge_threshold - 1) blocks are similar.
It is up to you whether to create the helper method.
I am fine with the current formation.

Cheers

---

From: David Steele <[email-masked]>
Date: Thu, 18 Mar 2021 09:38:09 -0400

On 1/5/21 10:51 AM, Zhihong Yu wrote:
> w.r.t. extracting to helper method, the second and third 
> if (currentExprIdx == pgss_merge_threshold - 1) blocks are similar.

Dmitry, thoughts on this review?

Regards,
-- 
-David
[email-masked]

---

From: Dmitry Dolgov <[email-masked]>
Date: Thu, 18 Mar 2021 16:50:02 +0100

> On Thu, Mar 18, 2021 at 09:38:09AM -0400, David Steele wrote:

Oh, right. lastExprLenghth is obviously a typo, and as we agreed that
the helper is not strictly necessary I wanted to wait a bit hoping for
more feedback and eventually to post an accumulated patch. Doesn't make
sense to post another version only to fix one typo :)

---

From: Dmitry Dolgov <[email-masked]>
Date: Tue, 15 Jun 2021 17:18:50 +0200

> On Thu, Mar 18, 2021 at 04:50:02PM +0100, Dmitry Dolgov wrote:

Hi,

I've prepared a new rebased version to deal with the new way of
computing query id, but as always there is one tricky part. From what I
understand, now an external module can provide custom implementation for
query id computation algorithm. It seems natural to think this machinery
could be used instead of patch in the thread, i.e. one could create a
custom logic that will enable constants collapsing as needed, so that
same queries with different number of constants in an array will be
hashed into the same record.

But there is a limitation in how such queries will be normalized
afterwards — to reduce level of surprise it's necessary to display the
fact that a certain query in fact had more constants that are showed in
pgss record. Ideally LocationLen needs to carry some bits of information
on what exactly could be skipped, and generate_normalized_query needs to
understand that, both are not reachable for an external module with
custom query id logic (without replicating significant part of the
existing code). Hence, a new version of the patch.

---

From: Dmitry Dolgov <[email-masked]>
Date: Wed, 16 Jun 2021 16:02:12 +0200

> On Tue, Jun 15, 2021 at 05:18:50PM +0200, Dmitry Dolgov wrote:

Forgot to mention a couple of people who already reviewed the patch.

---

From: Dmitry Dolgov <[email-masked]>
Date: Thu, 30 Sep 2021 15:49:30 +0200

>On Wed, Jun 16, 2021 at 04:02:12PM +0200, Dmitry Dolgov wrote:
>

And now for something completely different, here is a new patch version.
It contains a small fix for one problem we've found during testing (one
path code was incorrectly assuming find_const_walker results).

---

From: Zhihong Yu <[email-masked]>
Date: Thu, 30 Sep 2021 08:03:16 -0700

Hi,

bq. and at position further that specified threshold.

 that specified threshold -> than specified threshold

Cheers

---

From: Dmitry Dolgov <[email-masked]>
Date: Thu, 30 Sep 2021 17:09:57 +0200

> On Thu, Sep 30, 2021 at 08:03:16AM -0700, Zhihong Yu wrote:

You mean in the patch commit message, nowhere else, right? Yep, my spell
checker didn't catch that, thanks for noticing!