Re: BlockNumber initialized to InvalidBuffer?

Markus Wanner <markus@bluegap.ch> writes:
> I stumbled across an initialization of a BlockNumber with InvalidBuffer,
> which seems strange to me, as the values for "invalid" of the two types
> are different, see attached patch.

That's certainly bogus ...

> In case the 'stack' argument passed to that function is not NULL, the
> variable in question gets overridden immediately, in which case it
> certainly doesn't matter. I don't know nor did I check whether or not it
> can ever be NULL. So this might not be a real issue at all.

... but AFAICS, ginInsertValue cannot be called with stack == NULL at
any of the existing call sites.  Moreover, if stack were NULL, the
function would do nothing, which seems to me to violate its API contract
to insert the given value into the index.

So I think a better fix is to Assert that the passed stack isn't
NULL, along the lines of
   GinBtreeStack *parent;   BlockNumber rootBlkno;   Page        page,               rpage,               lpage;
   /* extract root BlockNumber from stack */   Assert(stack != NULL);   parent = stack;   do   {       rootBlkno =
parent->blkno;      parent = parent->parent;   } while (parent);
 

I'm also inclined to think that the "while (stack)" coding of the rest
of it is wrong, misleading, or both, on precisely the same grounds: if
that loop ever did fall out at the test, the function would have failed
to honor its contract.  The only correct exit points are the "return"s
in the middle.

Comments?
        regards, tom lane