Em ter., 26 de set. de 2023 às 09:30, Ranier Vilela <ranier...@gmail.com> escreveu:
> Em ter., 26 de set. de 2023 às 07:34, Ashutosh Bapat < > ashutosh.bapat....@gmail.com> escreveu: > >> On Tue, Sep 26, 2023 at 3:32 PM David Rowley <dgrowle...@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? >> > It seems to me that it adds a LEA instruction. > https://godbolt.org/z/b4jK3PErE > > Although it doesn't seem like much, > 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. best regards, Ranier Vilela
v1-0001-Avoid-possible-out-of-bounds-access.patch
Description: Binary data
