Re: heapgettup refactoring

On 03.01.23 21:39, Melanie Plageman wrote:
>> On 30.11.22 23:34, Melanie Plageman wrote:
>>> I have attached a patchset with only the code changes contained in the
>>> previous patch 0003. I have broken the refactoring down into many
>>> smaller pieces for ease of review.
>>
>> To keep this moving along a bit, I have committed your 0002, which I
>> think is a nice little improvement on its own.
> 
> Thanks!
> I've attached a rebased patchset - v4.
> 
> I also changed heapgettup_no_movement() to noinline (instead of inline).
> David Rowley pointed out that this might make more sense given how
> comparatively rare no movement scans are.

Ok, let's look through these patches starting from the top then.

v4-0001-Add-no-movement-scan-helper.patch

This makes sense overall; there is clearly some duplicate code that can 
be unified.

It appears that during your rebasing you have effectively reverted your 
earlier changes that have been committed as 
8e1db29cdbbd218ab6ba53eea56624553c3bef8c.  You should undo that.

I don't understand the purpose of the noinline maker.  If it's not 
necessary to inline, we can just leave it off, but there is no need to 
outright prevent inlining AFAICT.

I don't know why you changed the if/else sequences.  Before, the 
sequence was effectively

if (forward)
{
     ...
}
else if (backward)
{
     ...
}
else
{
     /* it's no movement */
}

Now it's changed to

if (no movement)
{
     ...
     return;
}

if (forward)
{
     ...
}
else
{
     Assert(backward);
     ...
}

Sure, that's the same thing, but it looks less elegant to me.