Re: [PATCH] Add support for SAOP in the optimizer for partial index paths

I wrote a 'larger' more contrived example (SAOP-saturation-test.sql)
yesterday to evaluate any performance overheads and it actually
exposed a shortcoming of the original patch. I've added two more
patches to address that. The new patches are both optimisations and
functionality improvements whereby previously I'd overlooked where
only the synthesized equality clause was matched.

I've attached here the script I used, though it isn't part of the
patchset. Having run it many times against both master and my local
branch with the 5 patches on, I can see that both the planner and
execution timings are consistent and indeed better with the new
partial index support for SAOP and the optimiser chooses the same path
as the original chain of ORs present in both branches (established
code).

Jim

On Sun, 7 Jun 2026 at 18:20, Jim Vanns <james(dot)vanns(at)gmail(dot)com> wrote:
>
> 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