Thread: BlockNumber initialized to InvalidBuffer?

Hackers,

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.

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.

Regards

Markus Wanner

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

On 07/11/2012 05:45 AM, Tom Lane wrote:
> 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.

I came to the same conclusion, yes. Looks like the additional asserts in
the attached patch all hold true.

As another minor improvement, it doesn't seem necessary to repeatedly
set the rootBlkno.

Regards

Markus Wanner

On 07/13/2012 11:33 AM, Markus Wanner wrote:
> As another minor improvement, it doesn't seem necessary to repeatedly
> set the rootBlkno.

Sorry, my mail program delivered an older version of the patch, which
didn't reflect that change. Here's what I intended to send.

Regards

Markus Wanner

Markus Wanner <markus@bluegap.ch> writes:
> On 07/13/2012 11:33 AM, Markus Wanner wrote:
>> As another minor improvement, it doesn't seem necessary to repeatedly
>> set the rootBlkno.

> Sorry, my mail program delivered an older version of the patch, which
> didn't reflect that change. Here's what I intended to send.

Applied to HEAD with a little bit of further tweaking.
        regards, tom lane