Обсуждение: Re: Avoid possible deference NULL pointer (src/backend/optimizer/path/allpaths.c)

> 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

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

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.