Date: Fri, 17 Feb 2023 16:46:02 +0100
From: Dmitry Dolgov <9erthalion6@gmail.com>
To: David Geier <geidav.pg@gmail.com>
Cc: Sergei Kornilov <sk@zsrv.org>, Michael Paquier <michael@paquier.xyz>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Marcos Pegoraro <marcos@f10.com.br>,
	vignesh C <vignesh21@gmail.com>,
	Robert Haas <robertmhaas@gmail.com>, 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>,
	Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: pg_stat_statements and "IN" conditions
Message-ID: <20230217154602.uf3a3cgqeoeysycz@erthalion.local>
References: <Y98GsRJoCM4RrcWp@paquier.xyz>
 <20230205113346.gmbf5ycnukuhnmqa@erthalion.local>
 <2420105.1675612952@sss.pgh.pa.us>
 <20230205195600.e3bhwszzm5kdyyxl@erthalion.local>
 <966631675800892@znolar4klagewshm.sas.yp-c.yandex.net>
 <20230209150238.rdtwruh5i34hdag5@ddolgov.remote.csb>
 <dda65b89-4ebf-261c-de9c-7b91ab9e4047@gmail.com>
 <20230211104707.grsicemegr7d3mgh@erthalion.local>
 <20230211120820.y7krq37jubnjsa5b@erthalion.local>
 <07919318-6c35-a13f-6a1d-98686a3e38de@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <07919318-6c35-a13f-6a1d-98686a3e38de@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/20230217154602.uf3a3cgqeoeysycz%40erthalion.local>
Precedence: bulk

> On Wed, Feb 15, 2023 at 08:51:56AM +0100, David Geier wrote:
> Hi,
>
> On 2/11/23 13:08, Dmitry Dolgov wrote:
> > > On Sat, Feb 11, 2023 at 11:47:07AM +0100, Dmitry Dolgov wrote:
> > >
> > > The original version of the patch was doing all of this, i.e. handling
> > > numerics, Param nodes, RTE_VALUES. The commentary about
> > > find_const_walker in tests is referring to a part of that, that was
> > > dealing with evaluation of expression to see if it could be reduced to a
> > > constant.
> > >
> > > Unfortunately there was a significant push back from reviewers because
> > > of those features. That's why I've reduced the patch to it's minimally
> > > useful version, having in mind re-implementing them as follow-up patches
> > > in the future. This is the reason as well why I left tests covering all
> > > this missing functionality -- as breadcrumbs to already discovered
> > > cases, important for the future extensions.
> > I'd like to elaborate on this a bit and remind about the origins of the
> > patch, as it's lost somewhere in the beginning of the thread. The idea
> > is not pulled out of thin air, everything is coming from our attempts to
> > improve one particular monitoring infrastructure in a real commercial
> > setting. Every covered use case and test in the original proposal was a
> > result of field trials, when some application-side library or ORM was
> > responsible for gigabytes of data in pgss, chocking the monitoring agent.
>
> Thanks for the clarifications. I didn't mean to contend the usefulness of
> the patch and I wasn't aware that you already jumped through the loops of
> handling Param, etc.

No worries, I just wanted to emphasize that we've already collected
quite some number of use cases.

> Seems like supporting only constants is a good starting
> point. The only thing that is likely confusing for users is that NUMERICs
> (and potentially constants of other types) are unsupported. Wouldn't it be
> fairly simple to support them via something like the following?
>
>     is_const(element) || (is_coercion(element) && is_const(element->child))

It definitely makes sense to implement that, although I don't think it's
going to be acceptable to do that via directly listing conditions an
element has to satisfy. It probably has to be more flexible, sice we
would like to extend it in the future. My plan is to address this in a
follow-up patch, when the main mechanism is approved. Would you agree
with this approach?