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, <[email protected]> wrote: > On Sun, 11 Jan 2026 at 06:03, Jim Vanns <[email protected]> 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 >
