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

On 18/03/2019 02:59, Peter Geoghegan wrote:
> On Sat, Mar 16, 2019 at 1:05 PM Heikki Linnakangas <hlinnaka(at)iki(dot)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.