Re: heapgettup refactoring

On Thu, Feb 02, 2023 at 07:00:37PM +1300, David Rowley wrote:
> I've attached a version of the next patch in the series. I admit to
> making a couple of adjustments. I couldn't bring myself to remove the
> snapshot local variable in this commit. We can deal with that when we
> come to it in whichever patch that needs to be changed in.

That seems fine to keep the diff easy to understand. Also,
heapgettup_pagemode() didn't have a snapshot local variable either.

> The only other thing I really did was question your use of rs_cindex
> to store the last OffsetNumber. I ended up adding a new field which
> slots into the padding between a bool and BlockNumber field named
> rs_coffset for this purpose. I noticed what Andres wrote [1] earlier
> in this thread about that, so thought we should move away from looking
> at the last tuple to get this number
>
> [1] https://postgr.es/m/20221101010948.hsf33emgnwzvil4a@awork3.anarazel.de

So, what Andres had said was:

> Hm. Finding the next offset via rs_ctup doesn't seem quite right. For one,
> it's not actually that cheap to extract the offset from an ItemPointer because
> of the the way we pack it into ItemPointerData.

Because I was doing this in an earlier version:

> > + HeapTuple tuple = &(scan->rs_ctup);

And then in the later part of the code got tuple->t_self.

I did this because the code in master does this:

lineoff = /* previous offnum */
Min(lines,
OffsetNumberPrev(ItemPointerGetOffsetNumber(&(tuple->t_self))));

So I figured it was the same. Based on Andres' feedback, I switched to
saving the offset number in the scan descriptor and figured I could
reuse rs_cindex since it is larger than an OffsetNumber.

Your code also switches to saving the OffsetNumber -- just in a separate
variable of OffsetNumber type. I am fine with this if it your rationale
is that it is not a good idea to store a smaller number in a larger
datatype. However, the benefit I saw in reusing rs_cindex is that we
could someday converge the code for heapgettup() and
heapgettup_pagemode() even more. Even in my final refactor, there is a
lot of duplicate code between the two.

- Melanie