Thread: Re: Avoid possible deference NULL pointer (src/backend/optimizer/path/allpaths.c)
Re: Avoid possible deference NULL pointer (src/backend/optimizer/path/allpaths.c)
From
Daniel Gustafsson
Date:
> On 5 Jan 2025, at 00:29, Ranier Vilela <ranier.vf@gmail.com> wrote: > > Hi. > > Per Coverity. > > All call sites of function *get_cheapest_path_for_pathkeys* checks > for NULL returns. > > So, it is highly likely that the function will return NULL. > > IMO, the Assert in this particular call, is not fully effective. > > Fix removing the Assert and always check if the return is NULL. Yet the author wrote an Assert here (over a decade ago), so rather than blindly changing that it seems reasonable to motivate a patch like this with an investigation on what the Assert means here. The fact that Coverity complains is far from conclusive evidence that something is wrong. -- Daniel Gustafsson
Re: Avoid possible deference NULL pointer (src/backend/optimizer/path/allpaths.c)
From
Ranier Vilela
Date:
Hi.
Em qua., 5 de fev. de 2025 às 14:08, Daniel Gustafsson <daniel@yesql.se> escreveu:
> On 5 Jan 2025, at 00:29, Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Hi.
>
> Per Coverity.
>
> All call sites of function *get_cheapest_path_for_pathkeys* checks
> for NULL returns.
>
> So, it is highly likely that the function will return NULL.
>
> IMO, the Assert in this particular call, is not fully effective.
>
> Fix removing the Assert and always check if the return is NULL.
Yet the author wrote an Assert here (over a decade ago), so rather than blindly
changing that it seems reasonable to motivate a patch like this with an
investigation on what the Assert means here. The fact that Coverity complains
is far from conclusive evidence that something is wrong.
This is evidence that we do not have reports about this bug.
In any case, I think it's very unsafe for the future to trust that a function that returns NULL
will never return in this particular case, don't you think?
best regards,
Ranier Vilela
Re: Avoid possible deference NULL pointer (src/backend/optimizer/path/allpaths.c)
From
Daniel Gustafsson
Date:
On 5 Feb 2025, at 18:34, Ranier Vilela <ranier.vf@gmail.com> wrote:Em qua., 5 de fev. de 2025 às 14:08, Daniel Gustafsson <daniel@yesql.se> escreveu:
Yet the author wrote an Assert here (over a decade ago), so rather than blindly
changing that it seems reasonable to motivate a patch like this with an
investigation on what the Assert means here. The fact that Coverity complains
is far from conclusive evidence that something is wrong.This is evidence that we do not have reports about this bug.
Before that can be stated it needs to be determined if this is a bug, this
thread has not done that yet.
--
Daniel Gustafsson
Daniel Gustafsson
Re: Avoid possible deference NULL pointer (src/backend/optimizer/path/allpaths.c)
From
Tom Lane
Date:
Daniel Gustafsson <daniel@yesql.se> writes:
> On 5 Feb 2025, at 18:34, Ranier Vilela <ranier.vf@gmail.com> wrote:
>> This is evidence that we do not have reports about this bug.
> Before that can be stated it needs to be determined if this is a bug, this
> thread has not done that yet.
It's not a bug. Since the call specifies NIL pathkeys (meaning it
doesn't care about sort order) and does not insist on a parallel-safe
path, there should always be a path that satisfies it. The only
way it could fail to find a path is if the rel's pathlist is entirely
empty, a case already rejected by the sole caller.
Moreover, the argument that we might not have gotten a report is not
credible. In a production build without an Assert, the next line
would still dump core just as effectively if the result were NULL.
While the proposed patch doesn't break anything, it's removing a
logic cross-check that was put there intentionally. So I don't
find it to be an improvement.
regards, tom lane
Re: Avoid possible deference NULL pointer (src/backend/optimizer/path/allpaths.c)
From
Ilia Evdokimov
Date:
On 05.02.2025 21:56, Tom Lane wrote:
It's not a bug. Since the call specifies NIL pathkeys (meaning it doesn't care about sort order) and does not insist on a parallel-safe path, there should always be a path that satisfies it. The only way it could fail to find a path is if the rel's pathlist is entirely empty, a case already rejected by the sole caller. Moreover, the argument that we might not have gotten a report is not credible. In a production build without an Assert, the next line would still dump core just as effectively if the result were NULL. While the proposed patch doesn't break anything, it's removing a logic cross-check that was put there intentionally. So I don't find it to be an improvement. regards, tom lane
Ah, if this Assert was intentionally added to ensure that a path must be always found under the given conditions, and that any issues with this can be detected in the right place, then I agree. The patch likely makes worse.
--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.