From: Tom Lane <tgl@sss.pgh.pa.us>
To: Dmitry Dolgov <9erthalion6@gmail.com>
cc: Zhihong Yu <zyu@yugabyte.com>, David Steele <david@pgmasters.net>,
        PostgreSQL-development <pgsql-hackers@postgresql.org>,
        Greg Stark <stark@mit.edu>,
        Pavel Trukhanov <pavel.trukhanov@gmail.com>
Subject: Re: pg_stat_statements and "IN" conditions
In-reply-to: <20210930150957.tz3h3unjyrzatw6e@localhost>
References: <20201226104635.uvzkftzu72zg5qux@localhost> <CALNJ-vQx7u-Nq_wVun72CVrHe4DcGfEurj_ikckJrRy2Pem=JA@mail.gmail.com> <20210105125230.etgmgixnn6fmpszl@localhost> <CALNJ-vRRrdXu9igzh5PFTBghK5hnQWF6gpntTPo_+Agq_TsT5Q@mail.gmail.com> <ec47aa60-0a7b-ea3a-222a-5c6a754c2def@pgmasters.net> <20210318155002.ggrgf2p2mvaodcox@localhost> <20210615151850.6nsue7z5xjhpytle@localhost> <20210616140212.kt3n5wi3alfz5i5d@localhost> <20210930134930.d4vzwhvxhfpk2zxt@localhost> <CALNJ-vSJvaJbcXr46JMyWFT5rO3BdaHO2E=xnAbLNDw1cyUJ-A@mail.gmail.com> <20210930150957.tz3h3unjyrzatw6e@localhost>
Comments: In-reply-to Dmitry Dolgov <9erthalion6@gmail.com>
	message dated "Thu, 30 Sep 2021 17:09:57 +0200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <4154684.1641337363.1@sss.pgh.pa.us>
Date: Tue, 04 Jan 2022 18:02:43 -0500
Message-ID: <4154685.1641337363@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/4154685.1641337363%40sss.pgh.pa.us>
Precedence: bulk

Dmitry Dolgov <9erthalion6@gmail.com> writes:
> 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).

I've been saying from day one that pushing the query-hashing code into the
core was a bad idea, and I think this patch perfectly illustrates why.
We can debate whether the rules proposed here are good for
pg_stat_statements or not, but it seems inevitable that they will be a
disaster for some other consumers of the query hash.  In particular,
dropping external parameters from the hash seems certain to break
something for somebody --- do you really think that a query with two int
parameters is equivalent to one with five float parameters for all
query-identifying purposes?

I can see the merits of allowing different numbers of IN elements
to be considered equivalent for pg_stat_statements, but this patch
seems to go far beyond that basic idea, and I fear the side-effects
will be very bad.

Also, calling eval_const_expressions in the query jumbler is flat
out unacceptable.  There is way too much code that could be reached
that way (more or less the entire executor, to start with).  I
don't have a lot of faith that it'd never modify the input tree,
either.

			regards, tom lane