Re: heapgettup refactoring

On Tue, 31 Jan 2023 at 12:18, Melanie Plageman
<melanieplageman@gmail.com> wrote:
> v7 attached

I've been looking over the v7-0002 patch today and I did make a few
adjustments to heapgettup_initial_block() as I would prefer to see the
branching of each of all these helper functions follow the pattern of:

if (<forward scan>)
{
    if (<parallel scan>)
        <parallel stuff>
    else
        <serial stuff>
}
else
{
    <backwards serial stuff>
}

which wasn't quite what the function was doing.

Along the way, I noticed that 0002 has a subtle bug that does not seem
to be present once the remaining patches are applied.  I think I'm
happier to push these along the lines of how you have them in the
patches, so I've held off pushing for now due to the bug and the
change I had to make to fix it.

The problem is around the setting of scan->rs_inited = true; you've
moved that into heapgettup_initial_block() and you've correctly not
initialised the scan for empty tables when you return
InvalidBlockNumber, however, you've not correctly considered the fact
that table_block_parallelscan_nextpage() could also return
InvalidBlockNumber if the parallel workers manage to grab all of the
blocks before the current process gets the first block. I don't know
for sure, but it looks like this could cause problems when
heapgettup() or heapgettup_pagemode() got called again for a rescan.
We'd have returned the NULL tuple to indicate that no further tuples
exist, but we'll have left rs_inited set to true which looks like
it'll cause issues.

I wondered if it might be better to do the scan->rs_inited = true; in
heapgettup() and heapgettup_pagemode() instead. The attached v8 patch
does it this way. Despite this fixing that bug, I think this might be
a slightly better division of duties.

If you're ok with the attached (and everyone else is too), then I can
push it in the (NZ) morning.  The remaining patches would need to be
rebased due to my changes.

David