Re: [PATCH] Add support for SAOP in the optimizer for partial index paths - Mailing list pgsql-hackers

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@gmail.com> wrote:
>
> On Sun, 11 Jan 2026 at 06:03, Jim Vanns <james.vanns@gmail.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