Oh. I see my CF status suggests a wrong format. Trying git
format-patch here instead of git-diff. My bad...
On Sun, 7 Jun 2026 at 17:30, Jim Vanns <james(dot)vanns(at)gmail(dot)com> wrote:
>
> Just adding the same patch rebased atop today's master at
> 89eafad297a9b01ad77cfc1ab93a433e0af894b0 before I attempt my first
> commit-fest entry. I've been sitting on this for a year now but I
> think I missed the PG19 boat? So I targeted PG20 in the CF entry.
>
> Jim
>
>
> On Sun, 1 Mar 2026 at 11:22, Jim Vanns <james(dot)vanns(at)gmail(dot)com> wrote:
> >
> > OK, I've had a considerable refactor which hopefully addresses all your points.
> >
> > The function now proceeds as follows;
> >
> > 1) Preparatory/rudimentary checks outside of main loop (OR-based SAOP
> > clause, array dimensionality, element count etc.)
> > 2) Pre-filter bitmap indices for partials or planner implied
> > 3) For each element IN() now look for best choice index via
> > compare_path_costs (not just first as before)
> > 3a) Check clause fits general index structure (affecting all elements)
> > 3b) Check index matches predicate value/element
> > 4) Build bitmap OR path for this candidate
> > 5) Tests now moved to existing bitmapopts sql/out
> >
> > I've extended the existing test too to include the multiple-choice
> > path for the planner as you suggested, which this patch should now
> > handle (before it was greedy).
> >
> > It's rebased on top of the current master
> > (aecc558666ad62fbecb08ff7af1394656811a581) to remain
> > relevant/up-to-date.
> >
> > I've not yet tested the performance of it (preferring
> > correctness/acceptance first!) in the face of many indexes and 100
> > elements in the array but before I do so, is there a best place for
> > this kind of test in the source tree? Somewhere beneath src/test
> > again? I looked but couldn't see an obvious place (bar the regression
> > tests).
> >
> > Cheers,
> >
> > Jim
> >
> >
> > Jim
> >
> > On Mon, 12 Jan 2026 at 00:54, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> > >
> > > On Sun, 11 Jan 2026 at 06:03, Jim Vanns <james(dot)vanns(at)gmail(dot)com> wrote:
> > > > Before I continue with the other suggestions of consolidating the test
> > > > and benchmarking, I've made the code change you suggested and used a
> > > > bitmap for recording positions in the list of candidate indexes. Can
> > > > you check and make sure I'm on the right track?
> > >
> > > Just a quick look;
> > >
> > > 1. There doesn't seem to be any consideration that there may be many
> > > partial indexes which are suitable for the SAOP element:
> > >
> > > drop table if exists t;
> > > create table t (a int);
> > > insert into t select x/1000 from generate_series(1,1000000)x;
> > > create index t_eq_1 on t (a) where a = 1;
> > > create index t_eq_2 on t (a) where a = 2;
> > > create index t_eq_3 on t (a) where a = 3;
> > >
> > > create index t_le_2 on t (a) where a <= 2;
> > >
> > > explain select * from t where a in(1,2,3); -- Uses t_le_2 twice rather
> > > than the other two more suitable indexes.
> > >
> > > drop index t_le_2;
> > > explain select * from t where a in(1,2,3); -- What I'd expect the
> > > above query to produce.
> > >
> > > See: compare_path_costs_fuzzily()
> > >
> > > 2. Is there any point in trying the index again when this condition is
> > > true: if (!clauseset.nonempty). Since you'll be looking for the same
> > > column for the next element, shouldn't you do bms_del_member() on that
> > > index? Then put an "if (bms_is_empty(suitable_indexes)) break;" before
> > > the while loop so that you don't needlessly process the entire SAOP
> > > array when you run out of suitable indexes.
> > >
> > > 3. Styistically, instead of using int index_pos, you can use
> > > foreach_current_index(idx_lc).
> > >
> > > 4. I think the following code puts too much faith into there only
> > > being 1 path produced. From a quick skim of the current code in
> > > build_index_paths(), because you're requesting ST_BITMAPSCAN, we don't
> > > seem to ever produce more than 1 path, but if that changed, then your
> > > code would make the list contain too many paths.
> > >
> > > per_saop_paths = list_concat(per_saop_paths, indexpaths);
> > >
> > > 5. Minor detail, but there's a bit of inconsistency in how you're
> > > checking for empty Lists. The preferred way is: list != NIL.
> > >
> > > 6. Are you sure you want to use predOK == true indexes? Do you have a
> > > case where this new code can produce a better plan than if the predOK
> > > index was used directly by the existing Path generation code? If so,
> > > please provide examples.
> > >
> > > David