Horton

Re: heapgettup refactoring

David Rowley <dgrowleyml@gmail.com>

View thread View on postgr.es
From: David Rowley <dgrowleyml@gmail.com>
To: David Rowley <dgrowleyml@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>, Andres Freund <andres@anarazel.de>, Pg Hackers <pgsql-hackers@postgresql.org>
Date: 2023-02-03T02:26:40Z
Lists: pgsql-hackers 
On Fri, 3 Feb 2023 at 06:23, Melanie Plageman <melanieplageman@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

Commits

  1. Remove stray duplicated comment in heapam.h

  2. More refactoring of heapgettup() and heapgettup_pagemode()

  3. Run pgindent on heapam.c

  4. Push lpp variable closer to usage in heapgetpage()

  5. Variable renaming in preparation for refactoring

  6. Turn HeapKeyTest macro into inline function

  7. Remove unused include

  8. Remove redundant breaks in HeapTupleSatisfiesVisibility