Re: [PATCH] reduce page overlap of GiST indexes built using sorted method

[ blast-from-the-past department ]

Alexander Korotkov <aekorotkov(at)gmail(dot)com> writes:
> On Wed, Jan 26, 2022 at 7:07 PM sergei sh. <sshoulbakov(at)kontur(dot)io> wrote:
>> Sorted build method description has been added in GiST README.

> Thank you for the revision. This patch looks good to me. I've
> slightly adjusted comments and formatting and wrote the commit
> message.
> I'm going to push this if no objections.

(This became commit f1ea98a79.) I have no idea why Coverity
suddenly got upset with this code more than four years later,
but it's unhappy today:

/srv/coverity/git/pgsql-git/postgresql/src/backend/access/gist/gistbuild.c: 432 in gist_indexsortbuild()
426 /*
427 * Write out the partially full non-root pages.
428 *
429 * Keep in mind that flush can build a new root. If number of pages is > 1
430 * then new root is required.
431 */
>>> CID 1699895: Null pointer dereferences (FORWARD_NULL)
>>> Dereferencing null pointer "levelstate".
432 while (levelstate->parent != NULL || levelstate->current_page != 0)
433 {
434 GistSortedBuildLevelState *parent;
435
436 gist_indexsortbuild_levelstate_flush(state, levelstate);
437 parent = levelstate->parent;

On its face, this complaint is correct: f1ea98a79's addition of
"|| levelstate->current_page != 0" to the while condition means there
is a path by which levelstate can become set to NULL at the bottom of
this loop, and then the next while-condition evaluation crashes.

Given the lack of field complaints, I suppose it's impossible that
current_page != 0 when parent is null, but if so the added condition
is useless and we could remove it again with no ill effect. I thought
of proposing that we change the while condition to

while (levelstate != NULL &&
(levelstate->parent != NULL || levelstate->current_page != 0))

but that doesn't actually help, because the code after the while
loop will also segfault if levelstate is null. So I'm not sure what
the current_page test was meant to accomplish, but it cannot ever have
been reached and returned true.

Thoughts?

regards, tom lane