On Tue, Sep 26, 2023 at 3:32 PM David Rowley <dgrowleyml@gmail.com> wrote: > > find_base_rel() could be made more robust for free by just casting the > relid and simple_rel_array_size to uint32 while checking that relid < > root->simple_rel_array_size. The 0th element should be NULL anyway, > so "if (rel)" should let relid==0 calls through and allow that to > ERROR still. I see that just changes a "jle" to "jnb" vs adding an > additional jump for Ranier's version. [1]
That's a good suggestion.
I am fine with find_base_rel() as it is today as well. But future-proofing it seems to be fine too.
> > It seems worth not making find_base_rel() more expensive than it is > today as commonly we just reference root->simple_rel_array[n] directly > anyway because it's cheaper. It would be nice if we didn't add further > overhead to find_base_rel() as this would make the case for using > PlannerInfo.simple_rel_array directly even stronger.
I am curious, is the overhead in find_base_rel() impacting overall performance?
I believe the solution (casting to unsigned) seems better.
So +1.
As suggested, casting is the best option that does not add any overhead and improves the robustness of the find_base_rel function. I propose patch v1, with the additional addition of fixing the find_base_rel_ignore_join function,
which despite not appearing in Coverity reports, suffers from the same problem.
Taking advantage, I also propose a scope reduction,
as well as the const of the root parameter, which is very appropriate.
