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.hsf33emgnwzvi...@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
