On Fri, 3 Feb 2023 at 06:23, Melanie Plageman <melanieplage...@gmail.com> wrote: > 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.
I was more concerned about the reuse of an unrelated field. I'm struggling to imagine why using the separate field would cause any issues around not being able to reduce the code duplication any more than we otherwise would. Surely in one case you need to get the offset by indexing the rs_vistuples[] array and the other is the offset directly. The only thing I can think of that would allow us not to have a condition there would be if we populated the rs_vistuples[] array with 1..n. I doubt should do that and if we did, we could just use the rs_cindex to index that without having to worry that we're using an unrelated field for something. I've pushed all but the final 2 patches now. David