On Mon, Sep 25, 2023 at 7:14 PM Ranier Vilela <ranier...@gmail.com> wrote: > > Em seg., 25 de set. de 2023 às 08:23, Ashutosh Bapat > <ashutosh.bapat....@gmail.com> escreveu: >> >> On Sat, Sep 23, 2023 at 7:29 PM Ranier Vilela <ranier...@gmail.com> wrote: >> > >> > Hi, >> > >> > Per Coverity. >> > CID 1518088 (#2 of 2): Improper use of negative value (NEGATIVE_RETURNS) >> > >> > The function bms_singleton_member can returns a negative number. >> > >> > /* >> > * Get a child rel for rel2 with the relids. See above comments. >> > */ >> > if (rel2_is_simple) >> > { >> > int varno = bms_singleton_member(child_relids2); >> > >> > child_rel2 = find_base_rel(root, varno); >> > } >> > >> > It turns out that in the get_matching_part_pairs function (joinrels.c), >> > the return of bms_singleton_member is passed to the find_base_rel >> > function, which cannot receive a negative value. >> > >> > find_base_rel is protected by an Assertion, which effectively indicates >> > that the error does not occur in tests and in DEBUG mode. >> > >> > But this does not change the fact that bms_singleton_member can return a >> > negative value, which may occur on some production servers. >> > >> > Fix by changing the Assertion into a real test, to protect the >> > simple_rel_array array. >> >> Do you have a scenario where bms_singleton_member() actually returns a >> -ve number OR it's just a possibility. > > Just a possibility. > >> >> bms_make_singleton() has an >> assertion at the end: Assert(result >= 0); >> bms_make_singleton(), bms_add_member() explicitly forbids negative >> values. It looks like we have covered all the places which can add a >> negative value to a bitmapset. May be we are missing a place or two. >> It will be good to investigate it. > > I try to do the research, mostly, with runtime compilation. > As previously stated, the error does not appear in the tests. > That said, although Assert protects in most cases, that doesn't mean it can't > occur in a query, running on a server in production mode. > > Now thinking about what you said about Assertion in bms_make_singleton. > I think it's nonsense, no?
Sorry, I didn't write it correctly. bms_make_singleton() doesn't accept a negative integer and bms_get_singleton_member() and bms_singleton_member() has assertion at the end. Since there is no possibility of a negative integer making itself a part of bitmapset, the two functions Asserting instead of elog'ing is better. Assert are cheaper in production. > Why design a function that in DEBUG mode prohibits negative returns, but in > runtime mode allows it? > After all, why allow a negative return, if for all practical purposes this is > prohibited? You haven't given any proof that there's a possibility that a negative value may be returned. We are not allowing negative value being returned at all. > Regarding the find_base_rel function, it is nonsense to protect the array > with Assertion. > After all, we have already protected the upper limit with a real test, why > not also protect the lower limit. > The additional testing is cheap and makes perfect sense, making the function > more robust in production mode. > As an added bonus, modern compilers will probably be able to remove the > additional test if it deems it not necessary. > Furthermore, all protections that were added to protect find_base_real calls > can eventually be removed, > since find_base_real will accept parameters with negative values. However, I agree that changing find_base_rel() the way you have done in your patch is fine and mildly future-proof. +1 to that idea irrespective of what bitmapset functions do. -- Best Wishes, Ashutosh Bapat