Re: pg_stat_statements and "IN" conditions

> On Sat, Feb 11, 2023 at 11:03:36AM +0100, David Geier wrote:
> Hi,
>
> On 2/9/23 16:02, Dmitry Dolgov wrote:
> > > Unfortunately, rebase is needed again due to recent changes in queryjumblefuncs ( 9ba37b2cb6a174b37fc51d0649ef73e56eae27fc )
> I reviewed the last patch applied to some commit from Feb. 4th.

Thanks for looking. Few quick answers about high-level questions below,
the rest I'll incorporate in the new version.

> - There's a comment about find_const_walker(). I cannot find that function
> anywhere. What am I missing?
>
> [...]
>
> - Don't you intend to use the NUMERIC data column in SELECT * FROM
> test_merge_numeric WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10)? Otherwise,
> the test is identical to previous test cases and you're not checking for
> what happens with NUMERICs which are wrapped in FuncExpr because of the
> implicit coercion.
>
> - Don't we want to extend IsConstList() to allow a list of all implicitly
> coerced constants? It's inconsistent that otherwise e.g. NUMERICs don't
> work.
>
> [...]
>
> - Prepared statements are not supported as they contain INs with Param
> instead of Const nodes. While less likely, I've seen applications that use
> prepared statements in conjunction with queries generated through a UI which
> ended up with tons of prepared queries with different number of elements in
> the IN clause. Not necessarily something that must go into this patch but
> maybe worth thinking about.

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.

> - Why do we actually only want to merge constants? Why don't we ignore the
> type of element in the IN and merge whatever is there? Is this because the
> original jumbling logic as of now only has support for constants?
>
> - Ideally we would even remove duplicates. That would even improve
> cardinality estimation but I guess we don't want to spend the cycles on
> doing so in the planner?

I believe these points are beyond the patch goals, as it's less clear
(at least to me) if it's safe or desirable to do so.