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

Thanks David. Apologies for the tardy response. As you can see, I don't get
a whole lot of spare time at the moment. Is there some better or more
suitable reference code or docs I should be looking at to help address your
points? I'll try and take another pass soon.

Cheers

Jim

On Mon, 12 Jan 2026, 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
>