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