Re: Making all nbtree entries unique by having heap TIDs participatein comparisons

On 18/03/2019 02:59, Peter Geoghegan wrote:
> On Sat, Mar 16, 2019 at 1:05 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>>> Actually, how about "rootsearch", or "rootdescend"? You're supposed to
>>> hyphenate "re-find", and so it doesn't really work as a function
>>> argument name.
>>
>> Works for me.
> 
> Attached is v18 of patch series, which calls the new verification
> option "rootdescend" verification.

Thanks!

I'm getting a regression failure from the 'create_table' test with this:

> --- /home/heikki/git-sandbox/postgresql/src/test/regress/expected/create_table.out      2019-03-11 14:41:41.382759197
+0200
> +++ /home/heikki/git-sandbox/postgresql/src/test/regress/results/create_table.out       2019-03-18 13:49:49.480249055
+0200
> @@ -413,18 +413,17 @@
>         c text,
>         d text
>  ) PARTITION BY RANGE (a oid_ops, plusone(b), c collate "default", d collate "C");
> +ERROR:  function plusone(integer) does not exist
> +HINT:  No function matches the given name and argument types. You might need to add explicit type casts.

Are you seeing that?

Looking at the patches 1 and 2 again:

I'm still not totally happy with the program flow and all the conditions 
in _bt_findsplitloc(). I have a hard time following which codepaths are 
taken when. I refactored that, so that there is a separate copy of the 
loop for V3 and V4 indexes. So, when the code used to be something like 
this:

_bt_findsplitloc(...)
{
     ...

     /* move right, if needed */
     for(;;)
     {
         /*
          * various conditions for when to stop. Different conditions
          * apply depending on whether it's a V3 or V4 index.
          */
     }

     ...
}

it is now:

_bt_findsplitloc(...)
{
     ...

     if (heapkeyspace)
     {
         /*
          * If 'checkingunique', move right to the correct page.
          */
         for (;;)
         {
             ...
         }
     }
     else
     {
         /*
          * Move right, until we find a page with enough space or "get
          * tired"
          */
         for (;;)
         {
             ...
         }
     }

     ...
}

I think this makes the logic easier to understand. Although there is 
some commonality, the conditions for when to move right on a V3 vs V4 
index are quite different, so it seems better to handle them separately. 
There is some code duplication, but it's not too bad. I moved the common 
code to step to the next page to a new helper function, _bt_stepright(), 
which actually seems like a good idea in any case.

See attached patches with those changes, plus some minor comment 
kibitzing. It's still failing the 'create_table' regression test, though.

- Heikki

PS. The commit message of the first patch needs updating, now that 
BTInsertState is different from BTScanInsert.