Thread

1. BlockNumber initialized to InvalidBuffer?

   Markus Wanner <markus@bluegap.ch> — 2012-07-10T13:07:56Z

   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
   

2. Re: BlockNumber initialized to InvalidBuffer?

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-07-11T03:45:15Z

   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
   
   

3. Re: BlockNumber initialized to InvalidBuffer?

   Markus Wanner <markus@bluegap.ch> — 2012-07-13T09:33:51Z

   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
   

4. Re: BlockNumber initialized to InvalidBuffer?

   Markus Wanner <markus@bluegap.ch> — 2012-07-13T11:39:25Z

   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
   

5. Re: BlockNumber initialized to InvalidBuffer?

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-07-13T15:38:19Z

   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