Subject: heapgettup refactoring

From: Melanie Plageman <[email-masked]>
Date: Mon, 31 Oct 2022 14:37:39 -0400

Hi,

Attached is a patchset to refactor heapgettup(), heapgettup_pagemode(),
and heapgetpage(). heapgettup() and heapgettup_pagemode() have a lot of
duplicated code, confusingly nested if statements, and unnecessary local
variables. While working on a feature for the AIO/DIO patchset, I
noticed that it was difficult to add new code to heapgettup() and
heapgettup_pagemode() because of how the functions are written.

I've taken a stab at refactoring them -- without generating less
efficient code or causing regressions. I'm interested if people find it
more readable and if those with more assembly expertise see issues (new
branches added which are not highly predictable, etc). I took a look at
the assembly for those symbols compiled at O2 but am not experienced
enough at analysis to come to any conclusions.

- Melanie

---

From: Justin Pryzby <[email-masked]>
Date: Mon, 31 Oct 2022 17:37:44 -0500

FYI:

[18:51:54.707] ../src/backend/access/heap/heapam.c(720): warning C4098: 'heapgettup': 'void' function returning a value
[18:51:54.707] ../src/backend/access/heap/heapam.c(850): warning C4098: 'heapgettup_pagemode': 'void' function returning a value

For some reason, MSVC is the only one to complain, and cfbot doesn't
currently tell you about it.  I have a patch to show that, which I'll
send $later.

---

From: Andres Freund <[email-masked]>
Date: Mon, 31 Oct 2022 18:09:48 -0700

Hi,

> heapgettup_pagemode() because of how the functions are written.

Thanks for working on this - the current state is quite painful.

> From cde2d6720f4f5ab2531c22ad4a5f0d9e6ec1039d Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <[email-masked]>
> Date: Wed, 26 Oct 2022 20:00:34 -0400
> Subject: [PATCH v1 1/3] Remove breaks in HeapTupleSatisfiesVisibility
>
> breaks in HeapTupleSatisfiesVisibility were superfluous
> ---
>  src/backend/access/heap/heapam_visibility.c | 7 -------
>  1 file changed, 7 deletions(-)
>
> diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
> index 6e33d1c881..dd5d5da190 100644
> --- a/src/backend/access/heap/heapam_visibility.c
> +++ b/src/backend/access/heap/heapam_visibility.c
> @@ -1769,25 +1769,18 @@ HeapTupleSatisfiesVisibility(HeapTuple htup, Snapshot snapshot, Buffer buffer)
>  	{
>  		case SNAPSHOT_MVCC:
>  			return HeapTupleSatisfiesMVCC(htup, snapshot, buffer);
> -			break;
>  		case SNAPSHOT_SELF:
>  			return HeapTupleSatisfiesSelf(htup, snapshot, buffer);
> -			break;
>  		case SNAPSHOT_ANY:
>  			return HeapTupleSatisfiesAny(htup, snapshot, buffer);
> -			break;
>  		case SNAPSHOT_TOAST:
>  			return HeapTupleSatisfiesToast(htup, snapshot, buffer);
> -			break;
>  		case SNAPSHOT_DIRTY:
>  			return HeapTupleSatisfiesDirty(htup, snapshot, buffer);
> -			break;
>  		case SNAPSHOT_HISTORIC_MVCC:
>  			return HeapTupleSatisfiesHistoricMVCC(htup, snapshot, buffer);
> -			break;
>  		case SNAPSHOT_NON_VACUUMABLE:
>  			return HeapTupleSatisfiesNonVacuumable(htup, snapshot, buffer);
> -			break;
>  	}

Not sure what the author of this code, a certain Mr Freund, was thinking when
he added those returns...

> From 9d8b01960463dc64ff5b111d523ff80fce3017af Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <[email-masked]>
> Date: Mon, 31 Oct 2022 13:40:06 -0400
> Subject: [PATCH v1 2/3] Turn HeapKeyTest macro into function
>
> This should always be inlined appropriately now. It is easier to read as
> a function. Also, remove unused include in catcache.c.
> ---
>  src/backend/access/heap/heapam.c   | 10 ++--
>  src/backend/utils/cache/catcache.c |  1 -
>  src/include/access/valid.h         | 76 ++++++++++++------------------
>  3 files changed, 36 insertions(+), 51 deletions(-)
>
> diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
> index 12be87efed..1c995faa12 100644
> --- a/src/backend/access/heap/heapam.c
> +++ b/src/backend/access/heap/heapam.c
> @@ -716,8 +716,10 @@ heapgettup(HeapScanDesc scan,
>  													snapshot);
>
>  				if (valid && key != NULL)
> -					HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
> -								nkeys, key, valid);
> +				{
> +					valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
> +								nkeys, key);
> +				}
>
>  				if (valid)
>  				{

superfluous parens.

> --- a/src/include/access/valid.h
> +++ b/src/include/access/valid.h
> @@ -19,51 +19,35 @@
>   *
>   *		Test a heap tuple to see if it satisfies a scan key.
>   */
> -#define HeapKeyTest(tuple, \
> -					tupdesc, \
> -					nkeys, \
> -					keys, \
> -					result) \
> -do \
> -{ \
> -	/* Use underscores to protect the variables passed in as parameters */ \
> -	int			__cur_nkeys = (nkeys); \
> -	ScanKey		__cur_keys = (keys); \
> - \
> -	(result) = true; /* may change */ \
> -	for (; __cur_nkeys--; __cur_keys++) \
> -	{ \
> -		Datum	__atp; \
> -		bool	__isnull; \
> -		Datum	__test; \
> - \
> -		if (__cur_keys->sk_flags & SK_ISNULL) \
> -		{ \
> -			(result) = false; \
> -			break; \
> -		} \
> - \
> -		__atp = heap_getattr((tuple), \
> -							 __cur_keys->sk_attno, \
> -							 (tupdesc), \
> -							 &__isnull); \
> - \
> -		if (__isnull) \
> -		{ \
> -			(result) = false; \
> -			break; \
> -		} \
> - \
> -		__test = FunctionCall2Coll(&__cur_keys->sk_func, \
> -								   __cur_keys->sk_collation, \
> -								   __atp, __cur_keys->sk_argument); \
> - \
> -		if (!DatumGetBool(__test)) \
> -		{ \
> -			(result) = false; \
> -			break; \
> -		} \
> -	} \
> -} while (0)
> +static inline bool
> +HeapKeyTest(HeapTuple tuple, TupleDesc tupdesc, int nkeys, ScanKey keys)
> +{
> +	int cur_nkeys = nkeys;
> +	ScanKey cur_key = keys;
> +
> +	for (; cur_nkeys--; cur_key++)
> +	{
> +		Datum atp;
> +		bool isnull;
> +		Datum test;
> +
> +		if (cur_key->sk_flags & SK_ISNULL)
> +			return false;
> +
> +		atp = heap_getattr(tuple, cur_key->sk_attno, tupdesc, &isnull);
> +
> +		if (isnull)
> +			return false;
> +
> +		test = FunctionCall2Coll(&cur_key->sk_func,
> +								cur_key->sk_collation,
> +								atp, cur_key->sk_argument);
> +
> +		if (!DatumGetBool(test))
> +			return false;
> +	}
> +
> +	return true;
> +}

Seems like a simple and nice win in readability.

I recall looking at this in the past and thinking that there was some
additional subtlety here, but I can't see what that'd be.

> From a894ce38c488df6546392b9f3bd894b67edf951e Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <[email-masked]>
> Date: Mon, 31 Oct 2022 13:40:29 -0400
> Subject: [PATCH v1 3/3] Refactor heapgettup* and heapgetpage
>
> Simplify heapgettup(), heapgettup_pagemode(), and heapgetpage(). All
> three contained several unnecessary local variables, duplicate code, and
> nested if statements. Streamlining these improves readability and
> extensibility.

It'd be nice to break this into slightly smaller chunks.

> +
> +static inline void heapgettup_no_movement(HeapScanDesc scan)
> +{

FWIW, for function definitions we keep the return type (and with that also the
the "static inline") on a separate line.

> +	ItemId		lpp;
> +	OffsetNumber lineoff;
> +	BlockNumber page;
> +	Page dp;
> +	HeapTuple	tuple = &(scan->rs_ctup);
> +	/*
> +	* ``no movement'' scan direction: refetch prior tuple
> +	*/
> +
> +	/* Since the tuple was previously fetched, needn't lock page here */
> +	if (!scan->rs_inited)
> +	{
> +		Assert(!BufferIsValid(scan->rs_cbuf));
> +		tuple->t_data = NULL;
> +		return;

Is it possible to have a no-movement scan with an uninitialized scan? That
doesn't really seem to make sense. At least that's how I understand the
explanation for NoMovement nearby:
 * dir == NoMovementScanDirection means "re-fetch the tuple indicated
 * by scan->rs_ctup".

We can't have a rs_ctup without an already started scan, no?

Looks like this is pre-existing code that you just moved, but it still seems
wrong.

> +	}
> +	page = ItemPointerGetBlockNumber(&(tuple->t_self));
> +	if (page != scan->rs_cblock)
> +		heapgetpage((TableScanDesc) scan, page);

We have a
	BlockNumber page;
and
	Page		dp;
in this code which seems almost intentionally confusing. This again is a
pre-existing issue but perhaps we could clean it up first?

> +static inline Page heapgettup_continue_page(HeapScanDesc scan, BlockNumber page, ScanDirection dir,
> +		int *linesleft, OffsetNumber *lineoff)
> +{
> +	HeapTuple	tuple = &(scan->rs_ctup);

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.

> +	Page dp = BufferGetPage(scan->rs_cbuf);
> +	TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);

Newlines between definitions and code :)

Perhaps worth asserting that the scan is initialized and that rs_cbuf is valid?

> +	if (ScanDirectionIsForward(dir))
> +	{
> +		*lineoff = OffsetNumberNext(ItemPointerGetOffsetNumber(&(tuple->t_self)));
> +		*linesleft = PageGetMaxOffsetNumber(dp) - (*lineoff) + 1;

We can't access PageGetMaxOffsetNumber etc without holding a lock on the
page. It's not immediately obvious that that is held in all paths.

> +static inline BlockNumber heapgettup_initial_page(HeapScanDesc scan, ScanDirection dir)
> +{
> +	Assert(!ScanDirectionIsNoMovement(dir));
> +	Assert(!scan->rs_inited);

Is there a reason we couldn't set rs_inited in here, rather than reapeating
that in all callers?

ISTM that this function should deal with the
			/*
			 * return null immediately if relation is empty
			 */

logic, I think you now are repeating that check on every call to heapgettup().

> @@ -511,182 +711,55 @@ heapgettup(HeapScanDesc scan,
>  		   ScanKey key)
>  {
>  	HeapTuple	tuple = &(scan->rs_ctup);
> -	Snapshot	snapshot = scan->rs_base.rs_snapshot;
> -	bool		backward = ScanDirectionIsBackward(dir);
>  	BlockNumber page;
> -	bool		finished;
>  	Page		dp;
> -	int			lines;
>  	OffsetNumber lineoff;
>  	int			linesleft;
> -	ItemId		lpp;
> +
> +	if (ScanDirectionIsNoMovement(dir))
> +		return heapgettup_no_movement(scan);

Maybe add an unlikely() - this path is barely ever used...

>  	/*
> -	 * calculate next starting lineoff, given scan direction
> +	 * return null immediately if relation is empty
>  	 */
> -	if (ScanDirectionIsForward(dir))
> +	if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
>  	{

As mentioned above, I don't think we should repeat the nblocks check on every
call.

> +		page = scan->rs_cblock;
> +		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
> +		dp = heapgettup_continue_page(scan, page, dir, &linesleft, &lineoff);
> +		goto continue_page;
>  	}
>
>  	/*
>  	 * advance the scan until we find a qualifying tuple or run out of stuff
>  	 * to scan
>  	 */
> -	lpp = PageGetItemId(dp, lineoff);
> -	for (;;)
> +	while (page != InvalidBlockNumber)
>  	{
> +		heapgetpage((TableScanDesc) scan, page);
> +		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
> +		dp = heapgettup_start_page(scan, page, dir, &linesleft, &lineoff);
> +	continue_page:

I don't like the goto continue_page at all.  Seems that the paths leading here
should call LockBuffer(), heapgettup_start_page() etc?  Possibly a do {} while
() loop could do the trick as well.

Greetings,

Andres Freund

---

From: Melanie Plageman <[email-masked]>
Date: Fri, 4 Nov 2022 11:51:03 -0400

Thanks for the review!
Attached is v2 with feedback addressed.

> > +                                     valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),

fixed.

> It'd be nice to break this into slightly smaller chunks.

I can do that. Since incorporating feedback will be harder once I break
it up into smaller chunks, I'm inclined to wait to do so until I know
that the structure I have now is the one we will go with. (I know smaller
chunks will make it more reviewable.)

> the "static inline") on a separate line.

Fixed

> Looks like this is pre-existing code that you just moved, but it still seems

Changed to an assert

> pre-existing issue but perhaps we could clean it up first?

in attached
page -> block
dp -> page
in basically all locations in heapam.c (should that be its own commit?)

> of the the way we pack it into ItemPointerData.

So, it was like this before [1].
What about saving the lineoff in rs_cindex.

It is smaller, but that seems okay, right?

> Newlines between definitions and code :)

k

> Perhaps worth asserting that the scan is initialized and that rs_cbuf is valid?

indeed.

> page. It's not immediately obvious that that is held in all paths.

In heapgettup() I call LockBuffer() before invoking
heapgettup_continue_page() and heapgettup_start_page() which are the
ones doing this.

I did have big plans for using the continue_page and start_page
functions in heapgettup_pagemode() as well, but since I'm not doing that
now, I can add in an expectation that the lock is held.

I added a comment saying the caller is responsible for acquiring the
lock if needed. I thought of adding an assert, but I don't see that
being done outside of bufmgr.c

    BufferDesc *bufHdr = GetBufferDescriptor(buffer - 1);
    Assert(LWLockHeldByMe(BufferDescriptorGetContentLock(bufHdr)));

> Is there a reason we couldn't set rs_inited in here, rather than reapeating

I wasn't sure if future callers or existing callers in the future may
need to do steps other than what is in heapgettup_initial_page() before
setting rs_inited. I thought of the responsibility of
heapgettup_initial_page() as returning the initial page to start the
scan. If it is going to do all initialization steps, perhaps the name
should change? I thought having a function that says it does
initialization of the scan might be confusing since initscan() also
exists.

> logic, I think you now are repeating that check on every call to heapgettup().

So, that's a good point. If I move setting rs_inited inside of
heapgettup_initial_page(), then I can also easily move the empty table
check inside there too.

I don't want to set rs_inited before every return in
heapgettup_initial_page(). Do you think there are any issues with
setting it at the top of the function?

I thought about setting it at the very top (even before checking if the
relation is empty) Is it okay to set it before the empty table check?
rs_inited will be set to false at the bottom before returning. But,
maybe this will be an issue in other callers of
heapgettup_initial_page()?

Anyway, I have changed it in attached v2.

> Maybe add an unlikely() - this path is barely ever used...

done.

> () loop could do the trick as well.

I don't see how a  do while loop would solve help with the problem.
We need to check if the block number is valid after getting a block
assignment before doing heapgetpage() (e.g. after
heapgettup_initial_page() and after heapgettup_advance_page()).

Removing the goto continue_page means adding the heapgettpage(),
heapgettup_start_page(), etc code block in two places now (both after
heapgettup_initial_page() and after heapgettup_advance_page()) and, in
both locations we have to add an if statement to check if the block is
valid. I feel like this makes the function longer and harder to
understand. Keeping the loop as short as possible makes it clear what it
is doing. I think that with an appropriate warning comment, the goto
continue_page is clearer and easier to understand. To me, starting a
page at the top of the outer loop, then looping through the lines in the
page and is the structure that makes the most sense.

- Melanie

[1] https://github.com/postgres/postgres/blob/master/src/backend/access/heap/heapam.c#L572

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 16 Nov 2022 16:49:00 +0100

> Attached is v2 with feedback addressed.

Your 0001 had already been pushed.

I have pushed your 0002.

I have also pushed the renaming of page -> block, dp -> page separately. 
  This should reduce the size of your 0003 a bit.

Please produce an updated version of the 0003 page for further review.

---

From: Melanie Plageman <[email-masked]>
Date: Wed, 30 Nov 2022 17:34:50 -0500

> Please produce an updated version of the 0003 page for further review.

Thanks for looking at this!
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.

- Melanie
From 2e63656e75b00b24d286533ae8c8ef291876d4a8 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Wed, 30 Nov 2022 14:18:08 -0500
Subject: [PATCH v3 5/7] Add heapgettup() helpers

Add heapgettup_start_page() and heapgettup_continue_page(), helper
functions for heapgettup().
---
 src/backend/access/heap/heapam.c | 93 +++++++++++++++++++++-----------
 1 file changed, 61 insertions(+), 32 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index a4365beee1..f9b2d5cadb 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -575,6 +575,65 @@ heapgettup_initial_page(HeapScanDesc scan, ScanDirection dir)
 	return scan->rs_nblocks - 1;
 }
 
+static inline Page
+heapgettup_start_page(HeapScanDesc scan, BlockNumber block, ScanDirection dir,
+		int *linesleft, OffsetNumber *lineoff)
+{
+	Page page;
+
+	Assert(scan->rs_inited);
+	Assert(BufferIsValid(scan->rs_cbuf));
+
+	/* Caller is responsible for ensuring buffer is locked if needed */
+	page = BufferGetPage(scan->rs_cbuf);
+
+	TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
+
+	*linesleft = PageGetMaxOffsetNumber((Page) page) - FirstOffsetNumber + 1;
+
+	if (ScanDirectionIsForward(dir))
+		*lineoff = FirstOffsetNumber;
+	else
+		*lineoff = (OffsetNumber) (*linesleft);
+
+	/* lineoff now references the physically previous or next tid */
+	return page;
+}
+
+static inline Page
+heapgettup_continue_page(HeapScanDesc scan, BlockNumber block, ScanDirection
+		dir, int *linesleft, OffsetNumber *lineoff)
+{
+	Page page;
+
+	Assert(scan->rs_inited);
+	Assert(BufferIsValid(scan->rs_cbuf));
+
+	/* Caller is responsible for ensuring buffer is locked if needed */
+	page = BufferGetPage(scan->rs_cbuf);
+
+	TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
+
+	if (ScanDirectionIsForward(dir))
+	{
+		*lineoff = OffsetNumberNext(scan->rs_cindex);
+		*linesleft = PageGetMaxOffsetNumber(page) - (*lineoff) + 1;
+	}
+	else
+	{
+		/*
+		* The previous returned tuple may have been vacuumed since the
+		* previous scan when we use a non-MVCC snapshot, so we must
+		* re-establish the lineoff <= PageGetMaxOffsetNumber(page)
+		* invariant
+		*/
+		*lineoff = Min(PageGetMaxOffsetNumber(page), OffsetNumberPrev(scan->rs_cindex));
+		*linesleft = *lineoff;
+	}
+	/* block and lineoff now reference the physically next tid */
+	return page;
+}
+
 /* ----------------
  *		heapgettup - fetch next heap tuple
  *
@@ -632,45 +691,15 @@ heapgettup(HeapScanDesc scan,
 		}
 
 		heapgetpage((TableScanDesc) scan, block);
-
 		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
-		page = BufferGetPage(scan->rs_cbuf);
-		TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
-
-		linesleft = PageGetMaxOffsetNumber((Page) page) - FirstOffsetNumber + 1;
-
-		if (ScanDirectionIsForward(dir))
-			lineoff = FirstOffsetNumber;
-		else
-			lineoff = (OffsetNumber) linesleft;
+		page = heapgettup_start_page(scan, block, dir, &linesleft, &lineoff);
 	}
 	else
 	{
 		block = scan->rs_cblock; /* current page */
 
 		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
-
-		page = BufferGetPage(scan->rs_cbuf);
-		TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
-
-		if (ScanDirectionIsForward(dir))
-		{
-			lineoff = OffsetNumberNext(scan->rs_cindex);
-			/* block and lineoff now reference the physically next tid */
-			linesleft = PageGetMaxOffsetNumber(page) - lineoff + 1;
-		}
-		else
-		{
-			/*
-			 * The previous returned tuple may have been vacuumed since the
-			 * previous scan when we use a non-MVCC snapshot, so we must
-			 * re-establish the lineoff <= PageGetMaxOffsetNumber(page)
-			 * invariant
-			 */
-			lineoff = Min(PageGetMaxOffsetNumber(page), OffsetNumberPrev(scan->rs_cindex));
-			/* block and lineoff now reference the physically previous tid */
-			linesleft = lineoff;
-		}
+		page = heapgettup_continue_page(scan, block, dir, &linesleft, &lineoff);
 	}
 
 	/*
-- 
2.34.1

From 8d4106f9f045999c5b3432fc19ee8f216b85df2d Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Wed, 30 Nov 2022 10:42:12 -0500
Subject: [PATCH v3 2/7] Push lpp variable closer to usage in heapgetpage()

---
 src/backend/access/heap/heapam.c | 41 ++++++++++++++++----------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index f15296a67a..3da9c81c5b 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -382,7 +382,6 @@ heapgetpage(TableScanDesc sscan, BlockNumber block)
 	int			lines;
 	int			ntup;
 	OffsetNumber lineoff;
-	ItemId		lpp;
 	bool		all_visible;
 
 	Assert(block < scan->rs_nblocks);
@@ -451,31 +450,31 @@ heapgetpage(TableScanDesc sscan, BlockNumber block)
 	 */
 	all_visible = PageIsAllVisible(page) && !snapshot->takenDuringRecovery;
 
-	for (lineoff = FirstOffsetNumber, lpp = PageGetItemId(page, lineoff);
-		 lineoff <= lines;
-		 lineoff++, lpp++)
+	for (lineoff = FirstOffsetNumber; lineoff <= lines; lineoff++)
 	{
-		if (ItemIdIsNormal(lpp))
-		{
-			HeapTupleData loctup;
-			bool		valid;
+		HeapTupleData loctup;
+		bool		valid;
 
-			loctup.t_tableOid = RelationGetRelid(scan->rs_base.rs_rd);
-			loctup.t_data = (HeapTupleHeader) PageGetItem(page, lpp);
-			loctup.t_len = ItemIdGetLength(lpp);
-			ItemPointerSet(&(loctup.t_self), block, lineoff);
+		ItemId	lpp = PageGetItemId(page, lineoff);
 
-			if (all_visible)
-				valid = true;
-			else
-				valid = HeapTupleSatisfiesVisibility(&loctup, snapshot, buffer);
+		if (!ItemIdIsNormal(lpp))
+			continue;
 
-			HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
-												&loctup, buffer, snapshot);
+		loctup.t_tableOid = RelationGetRelid(scan->rs_base.rs_rd);
+		loctup.t_data = (HeapTupleHeader) PageGetItem(page, lpp);
+		loctup.t_len = ItemIdGetLength(lpp);
+		ItemPointerSet(&(loctup.t_self), block, lineoff);
 
-			if (valid)
-				scan->rs_vistuples[ntup++] = lineoff;
-		}
+		if (all_visible)
+			valid = true;
+		else
+			valid = HeapTupleSatisfiesVisibility(&loctup, snapshot, buffer);
+
+		HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
+											&loctup, buffer, snapshot);
+
+		if (valid)
+			scan->rs_vistuples[ntup++] = lineoff;
 	}
 
 	LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
-- 
2.34.1

From 2ddb8123f1a8d1d6938c15608a57d09958cfecfd Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Wed, 30 Nov 2022 17:06:43 -0500
Subject: [PATCH v3 3/7] Add heapgettup_initial_page() helper

---
 src/backend/access/heap/heapam.c | 215 ++++++++++++-------------------
 1 file changed, 82 insertions(+), 133 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 3da9c81c5b..142bcc0a1c 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -523,6 +523,58 @@ heapgettup_no_movement(HeapScanDesc scan)
 	return;
 }
 
+static inline BlockNumber
+heapgettup_initial_page(HeapScanDesc scan, ScanDirection dir)
+{
+	Assert(!ScanDirectionIsNoMovement(dir));
+	Assert(!scan->rs_inited);
+
+	 /* return null immediately if relation is empty */
+	if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
+		return InvalidBlockNumber;
+
+	scan->rs_inited = true;
+
+	/* forward and serial */
+	if (ScanDirectionIsForward(dir) && scan->rs_base.rs_parallel == NULL)
+		return scan->rs_startblock;
+
+	/* forward and parallel */
+	if (ScanDirectionIsForward(dir))
+	{
+		table_block_parallelscan_startblock_init(scan->rs_base.rs_rd,
+				scan->rs_parallelworkerdata,
+				(ParallelBlockTableScanDesc) scan->rs_base.rs_parallel);
+
+		return table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
+				scan->rs_parallelworkerdata,
+				(ParallelBlockTableScanDesc) scan->rs_base.rs_parallel);
+	}
+
+	/* backward parallel scan not supported */
+	Assert(scan->rs_base.rs_parallel == NULL);
+
+	/*
+	 * Disable reporting to syncscan logic in a backwards scan; it's
+	 * not very likely anyone else is doing the same thing at the same
+	 * time, and much more likely that we'll just bollix things for
+	 * forward scanners.
+	 */
+	scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC;
+
+	/*
+	 * Start from last page of the scan.  Ensure we take into account
+	 * rs_numblocks if it's been adjusted by heap_setscanlimits().
+	 */
+	if (scan->rs_numblocks != InvalidBlockNumber)
+		return (scan->rs_startblock + scan->rs_numblocks - 1) % scan->rs_nblocks;
+
+	if (scan->rs_startblock > 0)
+		return scan->rs_startblock - 1;
+
+	return scan->rs_nblocks - 1;
+}
+
 /* ----------------
  *		heapgettup - fetch next heap tuple
  *
@@ -569,48 +621,20 @@ heapgettup(HeapScanDesc scan,
 		return;
 	}
 
-	/*
-	 * calculate next starting lineoff, given scan direction
-	 */
 	if (ScanDirectionIsForward(dir))
 	{
 		if (!scan->rs_inited)
 		{
-			/*
-			 * return null immediately if relation is empty
-			 */
-			if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
+			block = heapgettup_initial_page(scan, dir);
+
+			if (block == InvalidBlockNumber)
 			{
 				Assert(!BufferIsValid(scan->rs_cbuf));
 				tuple->t_data = NULL;
 				return;
 			}
-			if (scan->rs_base.rs_parallel != NULL)
-			{
-				ParallelBlockTableScanDesc pbscan =
-				(ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
-				ParallelBlockTableScanWorker pbscanwork =
-				scan->rs_parallelworkerdata;
-
-				table_block_parallelscan_startblock_init(scan->rs_base.rs_rd,
-														 pbscanwork, pbscan);
-
-				block = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
-														  pbscanwork, pbscan);
-
-				/* Other processes might have already finished the scan. */
-				if (block == InvalidBlockNumber)
-				{
-					Assert(!BufferIsValid(scan->rs_cbuf));
-					tuple->t_data = NULL;
-					return;
-				}
-			}
-			else
-				block = scan->rs_startblock; /* first page */
 			heapgetpage((TableScanDesc) scan, block);
-			lineoff = FirstOffsetNumber;	/* first offnum */
-			scan->rs_inited = true;
+			lineoff = FirstOffsetNumber;
 		}
 		else
 		{
@@ -632,60 +656,36 @@ heapgettup(HeapScanDesc scan,
 	else
 	{
 		Assert(backward);
-		/* backward parallel scan not supported */
-		Assert(scan->rs_base.rs_parallel == NULL);
 
 		if (!scan->rs_inited)
 		{
-			/*
-			 * return null immediately if relation is empty
-			 */
-			if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
+			block = heapgettup_initial_page(scan, dir);
+
+			if (block == InvalidBlockNumber)
 			{
 				Assert(!BufferIsValid(scan->rs_cbuf));
 				tuple->t_data = NULL;
 				return;
 			}
 
-			/*
-			 * Disable reporting to syncscan logic in a backwards scan; it's
-			 * not very likely anyone else is doing the same thing at the same
-			 * time, and much more likely that we'll just bollix things for
-			 * forward scanners.
-			 */
-			scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC;
-
-			/*
-			 * Start from last page of the scan.  Ensure we take into account
-			 * rs_numblocks if it's been adjusted by heap_setscanlimits().
-			 */
-			if (scan->rs_numblocks != InvalidBlockNumber)
-				block = (scan->rs_startblock + scan->rs_numblocks - 1) % scan->rs_nblocks;
-			else if (scan->rs_startblock > 0)
-				block = scan->rs_startblock - 1;
-			else
-				block = scan->rs_nblocks - 1;
 			heapgetpage((TableScanDesc) scan, block);
+			LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+
+			page = BufferGetPage(scan->rs_cbuf);
+			TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
+			lines = PageGetMaxOffsetNumber(page);
+			lineoff = lines;	/* final offnum */
 		}
 		else
 		{
 			/* continue from previously returned page/tuple */
 			block = scan->rs_cblock; /* current page */
-		}
-
-		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+			LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
 
-		page = BufferGetPage(scan->rs_cbuf);
-		TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
-		lines = PageGetMaxOffsetNumber(page);
+			page = BufferGetPage(scan->rs_cbuf);
+			TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
+			lines = PageGetMaxOffsetNumber(page);
 
-		if (!scan->rs_inited)
-		{
-			lineoff = lines;	/* final offnum */
-			scan->rs_inited = true;
-		}
-		else
-		{
 			/*
 			 * The previous returned tuple may have been vacuumed since the
 			 * previous scan when we use a non-MVCC snapshot, so we must
@@ -893,41 +893,17 @@ heapgettup_pagemode(HeapScanDesc scan,
 	{
 		if (!scan->rs_inited)
 		{
-			/*
-			 * return null immediately if relation is empty
-			 */
-			if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
+			block = heapgettup_initial_page(scan, dir);
+
+			if (block == InvalidBlockNumber)
 			{
 				Assert(!BufferIsValid(scan->rs_cbuf));
 				tuple->t_data = NULL;
 				return;
 			}
-			if (scan->rs_base.rs_parallel != NULL)
-			{
-				ParallelBlockTableScanDesc pbscan =
-				(ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
-				ParallelBlockTableScanWorker pbscanwork =
-				scan->rs_parallelworkerdata;
-
-				table_block_parallelscan_startblock_init(scan->rs_base.rs_rd,
-														 pbscanwork, pbscan);
-
-				block = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
-														  pbscanwork, pbscan);
 
-				/* Other processes might have already finished the scan. */
-				if (block == InvalidBlockNumber)
-				{
-					Assert(!BufferIsValid(scan->rs_cbuf));
-					tuple->t_data = NULL;
-					return;
-				}
-			}
-			else
-				block = scan->rs_startblock; /* first page */
 			heapgetpage((TableScanDesc) scan, block);
 			lineindex = 0;
-			scan->rs_inited = true;
 		}
 		else
 		{
@@ -946,58 +922,31 @@ heapgettup_pagemode(HeapScanDesc scan,
 	else
 	{
 		Assert(backward);
-		/* backward parallel scan not supported */
-		Assert(scan->rs_base.rs_parallel == NULL);
 
 		if (!scan->rs_inited)
 		{
-			/*
-			 * return null immediately if relation is empty
-			 */
-			if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
+			block = heapgettup_initial_page(scan, dir);
+
+			if (block == InvalidBlockNumber)
 			{
 				Assert(!BufferIsValid(scan->rs_cbuf));
 				tuple->t_data = NULL;
 				return;
 			}
 
-			/*
-			 * Disable reporting to syncscan logic in a backwards scan; it's
-			 * not very likely anyone else is doing the same thing at the same
-			 * time, and much more likely that we'll just bollix things for
-			 * forward scanners.
-			 */
-			scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC;
-
-			/*
-			 * Start from last page of the scan.  Ensure we take into account
-			 * rs_numblocks if it's been adjusted by heap_setscanlimits().
-			 */
-			if (scan->rs_numblocks != InvalidBlockNumber)
-				block = (scan->rs_startblock + scan->rs_numblocks - 1) % scan->rs_nblocks;
-			else if (scan->rs_startblock > 0)
-				block = scan->rs_startblock - 1;
-			else
-				block = scan->rs_nblocks - 1;
 			heapgetpage((TableScanDesc) scan, block);
+			page = BufferGetPage(scan->rs_cbuf);
+			TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
+			lines = scan->rs_ntuples;
+			lineindex = lines - 1;
 		}
 		else
 		{
 			/* continue from previously returned page/tuple */
 			block = scan->rs_cblock; /* current page */
-		}
-
-		page = BufferGetPage(scan->rs_cbuf);
-		TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
-		lines = scan->rs_ntuples;
-
-		if (!scan->rs_inited)
-		{
-			lineindex = lines - 1;
-			scan->rs_inited = true;
-		}
-		else
-		{
+			page = BufferGetPage(scan->rs_cbuf);
+			TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
+			lines = scan->rs_ntuples;
 			lineindex = scan->rs_cindex - 1;
 		}
 		/* block and lineindex now reference the previous visible tid */
-- 
2.34.1

From 38d83796ff3f3060a8dcb244c85864279f080450 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Wed, 30 Nov 2022 10:20:30 -0500
Subject: [PATCH v3 1/7] Add no movement scan helper

No movement scan can be handled first in heapgettup() and
heapgettup_pagemode(). Refactor this case into its own helper function
to improve readability.
---
 src/backend/access/heap/heapam.c | 118 +++++++++++++++----------------
 1 file changed, 56 insertions(+), 62 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 747db50376..f15296a67a 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -484,6 +484,46 @@ heapgetpage(TableScanDesc sscan, BlockNumber block)
 	scan->rs_ntuples = ntup;
 }
 
+/*
+ * ``no movement'' scan direction: refetch prior tuple
+ */
+static inline void
+heapgettup_no_movement(HeapScanDesc scan)
+{
+	ItemId		lpp;
+	OffsetNumber lineoff;
+	BlockNumber page;
+	Page dp;
+	HeapTuple	tuple = &(scan->rs_ctup);
+
+	/* The scan must be init'd for there to be a current tuple (rs_ctup) */
+	Assert(scan->rs_inited);
+
+	/* Since the tuple was previously fetched, needn't lock page here */
+	page = ItemPointerGetBlockNumber(&(tuple->t_self));
+	if (page != scan->rs_cblock)
+		heapgetpage((TableScanDesc) scan, page);
+
+	/* Since the tuple was previously fetched, needn't lock page here */
+	dp = BufferGetPage(scan->rs_cbuf);
+	TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);
+	lineoff = scan->rs_cindex;
+	lpp = PageGetItemId(dp, lineoff);
+	Assert(ItemIdIsNormal(lpp));
+
+	tuple->t_data = (HeapTupleHeader) PageGetItem((Page) dp, lpp);
+	tuple->t_len = ItemIdGetLength(lpp);
+
+	/* check that rs_cindex is in sync if in pagemode */
+	Assert(!(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) ||
+			(scan->rs_cindex < scan->rs_ntuples));
+
+	Assert(!(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) ||
+			(lineoff == scan->rs_vistuples[scan->rs_cindex]));
+
+	return;
+}
+
 /* ----------------
  *		heapgettup - fetch next heap tuple
  *
@@ -524,6 +564,12 @@ heapgettup(HeapScanDesc scan,
 	int			linesleft;
 	ItemId		lpp;
 
+	if (unlikely(ScanDirectionIsNoMovement(dir)))
+	{
+		heapgettup_no_movement(scan);
+		return;
+	}
+
 	/*
 	 * calculate next starting lineoff, given scan direction
 	 */
@@ -584,8 +630,9 @@ heapgettup(HeapScanDesc scan,
 
 		linesleft = lines - lineoff + 1;
 	}
-	else if (backward)
+	else
 	{
+		Assert(backward);
 		/* backward parallel scan not supported */
 		Assert(scan->rs_base.rs_parallel == NULL);
 
@@ -654,34 +701,6 @@ heapgettup(HeapScanDesc scan,
 
 		linesleft = lineoff;
 	}
-	else
-	{
-		/*
-		 * ``no movement'' scan direction: refetch prior tuple
-		 */
-		if (!scan->rs_inited)
-		{
-			Assert(!BufferIsValid(scan->rs_cbuf));
-			tuple->t_data = NULL;
-			return;
-		}
-
-		block = ItemPointerGetBlockNumber(&(tuple->t_self));
-		if (block != scan->rs_cblock)
-			heapgetpage((TableScanDesc) scan, block);
-
-		/* Since the tuple was previously fetched, needn't lock page here */
-		page = BufferGetPage(scan->rs_cbuf);
-		TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
-		lineoff = ItemPointerGetOffsetNumber(&(tuple->t_self));
-		lpp = PageGetItemId(page, lineoff);
-		Assert(ItemIdIsNormal(lpp));
-
-		tuple->t_data = (HeapTupleHeader) PageGetItem(page, lpp);
-		tuple->t_len = ItemIdGetLength(lpp);
-
-		return;
-	}
 
 	/*
 	 * advance the scan until we find a qualifying tuple or run out of stuff
@@ -862,6 +881,12 @@ heapgettup_pagemode(HeapScanDesc scan,
 	int			linesleft;
 	ItemId		lpp;
 
+	if (unlikely(ScanDirectionIsNoMovement(dir)))
+	{
+		heapgettup_no_movement(scan);
+		return;
+	}
+
 	/*
 	 * calculate next starting lineindex, given scan direction
 	 */
@@ -919,8 +944,9 @@ heapgettup_pagemode(HeapScanDesc scan,
 
 		linesleft = lines - lineindex;
 	}
-	else if (backward)
+	else
 	{
+		Assert(backward);
 		/* backward parallel scan not supported */
 		Assert(scan->rs_base.rs_parallel == NULL);
 
@@ -979,38 +1005,6 @@ heapgettup_pagemode(HeapScanDesc scan,
 
 		linesleft = lineindex + 1;
 	}
-	else
-	{
-		/*
-		 * ``no movement'' scan direction: refetch prior tuple
-		 */
-		if (!scan->rs_inited)
-		{
-			Assert(!BufferIsValid(scan->rs_cbuf));
-			tuple->t_data = NULL;
-			return;
-		}
-
-		block = ItemPointerGetBlockNumber(&(tuple->t_self));
-		if (block != scan->rs_cblock)
-			heapgetpage((TableScanDesc) scan, block);
-
-		/* Since the tuple was previously fetched, needn't lock page here */
-		page = BufferGetPage(scan->rs_cbuf);
-		TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
-		lineoff = ItemPointerGetOffsetNumber(&(tuple->t_self));
-		lpp = PageGetItemId(page, lineoff);
-		Assert(ItemIdIsNormal(lpp));
-
-		tuple->t_data = (HeapTupleHeader) PageGetItem(page, lpp);
-		tuple->t_len = ItemIdGetLength(lpp);
-
-		/* check that rs_cindex is in sync */
-		Assert(scan->rs_cindex < scan->rs_ntuples);
-		Assert(lineoff == scan->rs_vistuples[scan->rs_cindex]);
-
-		return;
-	}
 
 	/*
-- 
2.34.1

From 4d63c4942028d96adb431d4cd6c35330ed6e76f0 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Wed, 30 Nov 2022 12:08:05 -0500
Subject: [PATCH v3 4/7] Streamline heapgettup*() for refactor

Flip initial logic to set local variables in order to make it easier to
add helper functions. Also, remove unneeded local variables.
---
 src/backend/access/heap/heapam.c | 168 ++++++++++---------------------
 src/include/access/heapam.h      |   6 +-
 2 files changed, 58 insertions(+), 116 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 142bcc0a1c..a4365beee1 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -610,7 +610,6 @@ heapgettup(HeapScanDesc scan,
 	BlockNumber block;
 	bool		finished;
 	Page		page;
-	int			lines;
 	OffsetNumber lineoff;
 	int			linesleft;
 	ItemId		lpp;
@@ -621,84 +620,57 @@ heapgettup(HeapScanDesc scan,
 		return;
 	}
 
-	if (ScanDirectionIsForward(dir))
+	if (!scan->rs_inited)
 	{
-		if (!scan->rs_inited)
-		{
-			block = heapgettup_initial_page(scan, dir);
+		block = heapgettup_initial_page(scan, dir);
 
-			if (block == InvalidBlockNumber)
-			{
-				Assert(!BufferIsValid(scan->rs_cbuf));
-				tuple->t_data = NULL;
-				return;
-			}
-			heapgetpage((TableScanDesc) scan, block);
-			lineoff = FirstOffsetNumber;
-		}
-		else
+		if (block == InvalidBlockNumber)
 		{
-			/* continue from previously returned page/tuple */
-			block = scan->rs_cblock; /* current page */
-			lineoff =			/* next offnum */
-				OffsetNumberNext(ItemPointerGetOffsetNumber(&(tuple->t_self)));
+			Assert(!BufferIsValid(scan->rs_cbuf));
+			tuple->t_data = NULL;
+			return;
 		}
 
-		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+		heapgetpage((TableScanDesc) scan, block);
 
+		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
 		page = BufferGetPage(scan->rs_cbuf);
 		TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
-		lines = PageGetMaxOffsetNumber(page);
-		/* block and lineoff now reference the physically next tid */
 
-		linesleft = lines - lineoff + 1;
+		linesleft = PageGetMaxOffsetNumber((Page) page) - FirstOffsetNumber + 1;
+
+		if (ScanDirectionIsForward(dir))
+			lineoff = FirstOffsetNumber;
+		else
+			lineoff = (OffsetNumber) linesleft;
 	}
 	else
 	{
-		Assert(backward);
-
-		if (!scan->rs_inited)
-		{
-			block = heapgettup_initial_page(scan, dir);
+		block = scan->rs_cblock; /* current page */
 
-			if (block == InvalidBlockNumber)
-			{
-				Assert(!BufferIsValid(scan->rs_cbuf));
-				tuple->t_data = NULL;
-				return;
-			}
+		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
 
-			heapgetpage((TableScanDesc) scan, block);
-			LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+		page = BufferGetPage(scan->rs_cbuf);
+		TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
 
-			page = BufferGetPage(scan->rs_cbuf);
-			TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
-			lines = PageGetMaxOffsetNumber(page);
-			lineoff = lines;	/* final offnum */
+		if (ScanDirectionIsForward(dir))
+		{
+			lineoff = OffsetNumberNext(scan->rs_cindex);
+			/* block and lineoff now reference the physically next tid */
+			linesleft = PageGetMaxOffsetNumber(page) - lineoff + 1;
 		}
 		else
 		{
-			/* continue from previously returned page/tuple */
-			block = scan->rs_cblock; /* current page */
-			LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
-
-			page = BufferGetPage(scan->rs_cbuf);
-			TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
-			lines = PageGetMaxOffsetNumber(page);
-
 			/*
 			 * The previous returned tuple may have been vacuumed since the
 			 * previous scan when we use a non-MVCC snapshot, so we must
 			 * re-establish the lineoff <= PageGetMaxOffsetNumber(page)
 			 * invariant
 			 */
-			lineoff =			/* previous offnum */
-				Min(lines,
-					OffsetNumberPrev(ItemPointerGetOffsetNumber(&(tuple->t_self))));
+			lineoff = Min(PageGetMaxOffsetNumber(page), OffsetNumberPrev(scan->rs_cindex));
+			/* block and lineoff now reference the physically previous tid */
+			linesleft = lineoff;
 		}
-		/* block and lineoff now reference the physically previous tid */
-
-		linesleft = lineoff;
 	}
 
 	/*
@@ -743,6 +715,7 @@ heapgettup(HeapScanDesc scan,
 				if (valid)
 				{
 					LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
+					scan->rs_cindex = lineoff;
 					return;
 				}
 			}
@@ -835,12 +808,11 @@ heapgettup(HeapScanDesc scan,
 
 		page = BufferGetPage(scan->rs_cbuf);
 		TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
-		lines = PageGetMaxOffsetNumber(page);
-		linesleft = lines;
+		linesleft = PageGetMaxOffsetNumber(page);
 		if (backward)
 		{
-			lineoff = lines;
-			lpp = PageGetItemId(page, lines);
+			lineoff = linesleft;
+			lpp = PageGetItemId(page, linesleft);
 		}
 		else
 		{
@@ -874,7 +846,6 @@ heapgettup_pagemode(HeapScanDesc scan,
 	BlockNumber block;
 	bool		finished;
 	Page		page;
-	int			lines;
 	int			lineindex;
 	OffsetNumber lineoff;
 	int			linesleft;
@@ -886,73 +857,41 @@ heapgettup_pagemode(HeapScanDesc scan,
 		return;
 	}
 
-	/*
-	 * calculate next starting lineindex, given scan direction
-	 */
-	if (ScanDirectionIsForward(dir))
+	if (!scan->rs_inited)
 	{
-		if (!scan->rs_inited)
-		{
-			block = heapgettup_initial_page(scan, dir);
+		block = heapgettup_initial_page(scan, dir);
 
-			if (block == InvalidBlockNumber)
-			{
-				Assert(!BufferIsValid(scan->rs_cbuf));
-				tuple->t_data = NULL;
-				return;
-			}
-
-			heapgetpage((TableScanDesc) scan, block);
-			lineindex = 0;
-		}
-		else
+		if (block == InvalidBlockNumber)
 		{
-			/* continue from previously returned page/tuple */
-			block = scan->rs_cblock; /* current page */
-			lineindex = scan->rs_cindex + 1;
+			Assert(!BufferIsValid(scan->rs_cbuf));
+			tuple->t_data = NULL;
+			return;
 		}
 
+		heapgetpage((TableScanDesc) scan, block);
 		page = BufferGetPage(scan->rs_cbuf);
 		TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
-		lines = scan->rs_ntuples;
-		/* block and lineindex now reference the next visible tid */
-
-		linesleft = lines - lineindex;
+		linesleft = scan->rs_ntuples;
+		lineindex = ScanDirectionIsForward(dir) ? 0 : linesleft - 1;
 	}
 	else
 	{
-		Assert(backward);
-
-		if (!scan->rs_inited)
-		{
-			block = heapgettup_initial_page(scan, dir);
+		/* continue from previously returned page/tuple */
+		block = scan->rs_cblock; /* current page */
 
-			if (block == InvalidBlockNumber)
-			{
-				Assert(!BufferIsValid(scan->rs_cbuf));
-				tuple->t_data = NULL;
-				return;
-			}
+		page = BufferGetPage(scan->rs_cbuf);
+		TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
 
-			heapgetpage((TableScanDesc) scan, block);
-			page = BufferGetPage(scan->rs_cbuf);
-			TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
-			lines = scan->rs_ntuples;
-			lineindex = lines - 1;
-		}
+		lineindex = scan->rs_cindex + dir;
+		if (ScanDirectionIsForward(dir))
+			linesleft = scan->rs_ntuples - lineindex;
 		else
-		{
-			/* continue from previously returned page/tuple */
-			block = scan->rs_cblock; /* current page */
-			page = BufferGetPage(scan->rs_cbuf);
-			TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
-			lines = scan->rs_ntuples;
-			lineindex = scan->rs_cindex - 1;
-		}
-		/* block and lineindex now reference the previous visible tid */
-
-		linesleft = lineindex + 1;
+			linesleft = scan->rs_cindex;
 	}
+	/*
+	* block and lineindex now reference the next or previous visible tid
+	*/
+
 
 	/*
 	 * advance the scan until we find a qualifying tuple or run out of stuff
@@ -1066,10 +1005,9 @@ heapgettup_pagemode(HeapScanDesc scan,
 
 		page = BufferGetPage(scan->rs_cbuf);
 		TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
-		lines = scan->rs_ntuples;
-		linesleft = lines;
+		linesleft = scan->rs_ntuples;
 		if (backward)
-			lineindex = lines - 1;
+			lineindex = linesleft - 1;
 		else
 			lineindex = 0;
 	}
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 810baaf9d0..8db8ae7624 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -71,8 +71,12 @@ typedef struct HeapScanDescData
 	 */
 	ParallelBlockTableScanWorkerData *rs_parallelworkerdata;
 
+	/*
+	 * current tuple's index in vistuples or current lineoff in page
+	 */
+	int			rs_cindex;
+
 	/* these fields only used in page-at-a-time mode and for bitmap scans */
-	int			rs_cindex;		/* current tuple's index in vistuples */
 	int			rs_ntuples;		/* number of visible tuples on page */
 	OffsetNumber rs_vistuples[MaxHeapTuplesPerPage];	/* their offsets */
 }			HeapScanDescData;
-- 
2.34.1

From 5ed8a30fcb49779bf874d5b56e4bd5bf9189a00d Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Wed, 30 Nov 2022 15:38:44 -0500
Subject: [PATCH v3 7/7] Refactor heapgettup() and heapgettup_pagemode()

---
 src/backend/access/heap/heapam.c | 252 ++++++++++++-------------------
 1 file changed, 93 insertions(+), 159 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index f1081ab529..8900490f2c 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -732,13 +732,10 @@ heapgettup(HeapScanDesc scan,
 		   ScanKey key)
 {
 	HeapTuple	tuple = &(scan->rs_ctup);
-	Snapshot	snapshot = scan->rs_base.rs_snapshot;
-	bool		backward = ScanDirectionIsBackward(dir);
 	BlockNumber block;
 	Page		page;
 	OffsetNumber lineoff;
 	int			linesleft;
-	ItemId		lpp;
 
 	if (unlikely(ScanDirectionIsNoMovement(dir)))
 	{
@@ -750,32 +747,32 @@ heapgettup(HeapScanDesc scan,
 	{
 		block = heapgettup_initial_page(scan, dir);
 
-		if (block == InvalidBlockNumber)
-		{
-			Assert(!BufferIsValid(scan->rs_cbuf));
-			tuple->t_data = NULL;
-			return;
-		}
-
-		heapgetpage((TableScanDesc) scan, block);
-		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
-		page = heapgettup_start_page(scan, block, dir, &linesleft, &lineoff);
+		/*
+		 * If parallel and other processes have already finished the scan, the
+		 * returned block is expected to be InvalidBlockNumber. In this case,
+		 * ensure that the backend is not sitting on a valid buffer.
+		 */
+		Assert(block != InvalidBlockNumber || !BufferIsValid(scan->rs_cbuf));
 	}
 	else
 	{
 		block = scan->rs_cblock; /* current page */
-
 		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
 		page = heapgettup_continue_page(scan, block, dir, &linesleft, &lineoff);
+		goto continue_page;
 	}
 
 	/*
 	 * advance the scan until we find a qualifying tuple or run out of stuff
 	 * to scan
 	 */
-	lpp = PageGetItemId(page, lineoff);
-	for (;;)
+	while (block != InvalidBlockNumber)
 	{
+		heapgetpage((TableScanDesc) scan, block);
+		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+		page = heapgettup_start_page(scan, block, dir, &linesleft, &lineoff);
+	continue_page:
+
 		/*
 		 * Only continue scanning the page while we have lines left.
 		 *
@@ -783,53 +780,40 @@ heapgettup(HeapScanDesc scan,
 		 * PageGetMaxOffsetNumber(); both for forward scans when we resume the
 		 * table scan, and for when we start scanning a new page.
 		 */
-		while (linesleft > 0)
+		for (; linesleft > 0; linesleft--, lineoff += dir)
 		{
-			if (ItemIdIsNormal(lpp))
-			{
-				bool		valid;
+			bool	visible;
+			ItemId lpp = PageGetItemId(page, lineoff);
 
-				tuple->t_data = (HeapTupleHeader) PageGetItem(page, lpp);
-				tuple->t_len = ItemIdGetLength(lpp);
-				ItemPointerSet(&(tuple->t_self), block, lineoff);
-
-				/*
-				 * if current tuple qualifies, return it.
-				 */
-				valid = HeapTupleSatisfiesVisibility(tuple,
-													 snapshot,
-													 scan->rs_cbuf);
+			if (!ItemIdIsNormal(lpp))
+				continue;
 
-				HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
-													tuple, scan->rs_cbuf,
-													snapshot);
+			tuple->t_data = (HeapTupleHeader) PageGetItem((Page) page, lpp);
+			tuple->t_len = ItemIdGetLength(lpp);
+			ItemPointerSet(&(tuple->t_self), scan->rs_cblock, lineoff);
 
-				if (valid && key != NULL)
-					valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
-										nkeys, key);
+			/*
+			* if current tuple qualifies, return it.
+			* otherwise move to the next item on the block
+			*/
+			visible = HeapTupleSatisfiesVisibility(tuple,
+													scan->rs_base.rs_snapshot,
+													scan->rs_cbuf);
+
+			HeapCheckForSerializableConflictOut(visible, scan->rs_base.rs_rd,
+												tuple, scan->rs_cbuf,
+												scan->rs_base.rs_snapshot);
+
+			if (!visible)
+				continue;
 
-				if (valid)
-				{
-					LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
-					scan->rs_cindex = lineoff;
-					return;
-				}
-			}
+			if (key && !HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
+						nkeys, key))
+				continue;
 
-			/*
-			 * otherwise move to the next item on the page
-			 */
-			--linesleft;
-			if (backward)
-			{
-				--lpp;			/* move back in this page's ItemId array */
-				--lineoff;
-			}
-			else
-			{
-				++lpp;			/* move forward in this page's ItemId array */
-				++lineoff;
-			}
+			LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
+			scan->rs_cindex = lineoff;
+			return;
 		}
 
 		/*
@@ -839,39 +823,16 @@ heapgettup(HeapScanDesc scan,
 		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
 
 		block = heapgettup_advance_page(scan, block, dir);
+	}
 
-		/*
-		 * return NULL if we've exhausted all the pages
-		 */
-		if (block == InvalidBlockNumber)
-		{
-			if (BufferIsValid(scan->rs_cbuf))
-				ReleaseBuffer(scan->rs_cbuf);
-			scan->rs_cbuf = InvalidBuffer;
-			scan->rs_cblock = InvalidBlockNumber;
-			tuple->t_data = NULL;
-			scan->rs_inited = false;
-			return;
-		}
-
-		heapgetpage((TableScanDesc) scan, block);
-
-		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+	/* end of scan */
+	if (BufferIsValid(scan->rs_cbuf))
+		ReleaseBuffer(scan->rs_cbuf);
 
-		page = BufferGetPage(scan->rs_cbuf);
-		TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
-		linesleft = PageGetMaxOffsetNumber(page);
-		if (backward)
-		{
-			lineoff = linesleft;
-			lpp = PageGetItemId(page, linesleft);
-		}
-		else
-		{
-			lineoff = FirstOffsetNumber;
-			lpp = PageGetItemId(page, FirstOffsetNumber);
-		}
-	}
+	scan->rs_cbuf = InvalidBuffer;
+	scan->rs_cblock = InvalidBlockNumber;
+	tuple->t_data = NULL;
+	scan->rs_inited = false;
 }
 
 /* ----------------
@@ -894,13 +855,10 @@ heapgettup_pagemode(HeapScanDesc scan,
 					ScanKey key)
 {
 	HeapTuple	tuple = &(scan->rs_ctup);
-	bool		backward = ScanDirectionIsBackward(dir);
 	BlockNumber block;
 	Page		page;
 	int			lineindex;
-	OffsetNumber lineoff;
 	int			linesleft;
-	ItemId		lpp;
 
 	if (unlikely(ScanDirectionIsNoMovement(dir)))
 	{
@@ -912,18 +870,12 @@ heapgettup_pagemode(HeapScanDesc scan,
 	{
 		block = heapgettup_initial_page(scan, dir);
 
-		if (block == InvalidBlockNumber)
-		{
-			Assert(!BufferIsValid(scan->rs_cbuf));
-			tuple->t_data = NULL;
-			return;
-		}
-
-		heapgetpage((TableScanDesc) scan, block);
-		page = BufferGetPage(scan->rs_cbuf);
-		TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
-		linesleft = scan->rs_ntuples;
-		lineindex = ScanDirectionIsForward(dir) ? 0 : linesleft - 1;
+		/*
+		 * If parallel and other processes have already finished the scan, the
+		 * returned block is expected to be InvalidBlockNumber. In this case,
+		 * ensure that the backend is not sitting on a valid buffer.
+		 */
+		Assert(block != InvalidBlockNumber || !BufferIsValid(scan->rs_cbuf));
 	}
 	else
 	{
@@ -938,84 +890,66 @@ heapgettup_pagemode(HeapScanDesc scan,
 			linesleft = scan->rs_ntuples - lineindex;
 		else
 			linesleft = scan->rs_cindex;
-	}
-	/*
-	* block and lineindex now reference the next or previous visible tid
-	*/
+		/* block and lineindex now reference the next or previous visible tid */
 
+		goto continue_page;
+	}
 
 	/*
 	 * advance the scan until we find a qualifying tuple or run out of stuff
 	 * to scan
 	 */
-	for (;;)
+	while (block != InvalidBlockNumber)
 	{
-		while (linesleft > 0)
+		heapgetpage((TableScanDesc) scan, block);
+		page = BufferGetPage(scan->rs_cbuf);
+		TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
+		linesleft = scan->rs_ntuples;
+		lineindex = ScanDirectionIsForward(dir) ? 0 : linesleft - 1;
+
+		/* block and lineindex now reference the next or previous visible tid */
+
+	continue_page:
+
+		for (; linesleft > 0; linesleft--, lineindex += dir)
 		{
+			ItemId		lpp;
+			OffsetNumber lineoff;
+
 			lineoff = scan->rs_vistuples[lineindex];
 			lpp = PageGetItemId(page, lineoff);
 			Assert(ItemIdIsNormal(lpp));
 
-			tuple->t_data = (HeapTupleHeader) PageGetItem(page, lpp);
+			tuple->t_data = (HeapTupleHeader) PageGetItem((Page) page, lpp);
 			tuple->t_len = ItemIdGetLength(lpp);
 			ItemPointerSet(&(tuple->t_self), block, lineoff);
 
 			/*
-			 * if current tuple qualifies, return it.
-			 */
-			if (key != NULL)
-			{
-				bool		valid;
-
-				valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
-									nkeys, key);
-				if (valid)
-				{
-					scan->rs_cindex = lineindex;
-					return;
-				}
-			}
-			else
-			{
-				scan->rs_cindex = lineindex;
-				return;
-			}
+			* if current tuple qualifies, return it.
+			* otherwise move to the next item on the block
+			*/
+			if (key && !HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
+						nkeys, key))
+				continue;
 
-			/*
-			 * otherwise move to the next item on the page
-			 */
-			--linesleft;
-			if (backward)
-				--lineindex;
-			else
-				++lineindex;
+			scan->rs_cindex = lineindex;
+			return;
 		}
 
-		block = heapgettup_advance_page(scan, block, dir);
 		/*
-		 * return NULL if we've exhausted all the pages
+		 * if we get here, it means we've exhausted the items on this page and
+		 * it's time to move to the next.
 		 */
-		if (block == InvalidBlockNumber)
-		{
-			if (BufferIsValid(scan->rs_cbuf))
-				ReleaseBuffer(scan->rs_cbuf);
-			scan->rs_cbuf = InvalidBuffer;
-			scan->rs_cblock = InvalidBlockNumber;
-			tuple->t_data = NULL;
-			scan->rs_inited = false;
-			return;
-		}
-
-		heapgetpage((TableScanDesc) scan, block);
-
-		page = BufferGetPage(scan->rs_cbuf);
-		TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
-		linesleft = scan->rs_ntuples;
-		if (backward)
-			lineindex = linesleft - 1;
-		else
-			lineindex = 0;
+		block = heapgettup_advance_page(scan, block, dir);
 	}
+
+	/* end of scan */
+	if (BufferIsValid(scan->rs_cbuf))
+		ReleaseBuffer(scan->rs_cbuf);
+	scan->rs_cbuf = InvalidBuffer;
+	scan->rs_cblock = InvalidBlockNumber;
+	tuple->t_data = NULL;
+	scan->rs_inited = false;
 }

-- 
2.34.1

From bc7332551da42cc1798b23f422c8d06f9057713a Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Wed, 30 Nov 2022 15:06:34 -0500
Subject: [PATCH v3 6/7] Add heapgettup_advance_page()

---
 src/backend/access/heap/heapam.c | 168 +++++++++++++------------------
 1 file changed, 72 insertions(+), 96 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index f9b2d5cadb..f1081ab529 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -634,6 +634,74 @@ heapgettup_continue_page(HeapScanDesc scan, BlockNumber block, ScanDirection
 	return page;
 }
 
+static inline BlockNumber
+heapgettup_advance_page(HeapScanDesc scan, BlockNumber block, ScanDirection dir)
+{
+	if (ScanDirectionIsBackward(dir))
+	{
+		if (block == scan->rs_startblock)
+			return InvalidBlockNumber;
+
+		if (scan->rs_numblocks != InvalidBlockNumber)
+		{
+			if (--scan->rs_numblocks == 0)
+				return InvalidBlockNumber;
+		}
+
+		if (block == 0)
+			block = scan->rs_nblocks;
+
+		block--;
+
+		return block;
+	}
+	else if (scan->rs_base.rs_parallel != NULL)
+	{
+		Assert(ScanDirectionIsForward(dir));
+
+		block = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
+				scan->rs_parallelworkerdata, (ParallelBlockTableScanDesc)
+				scan->rs_base.rs_parallel);
+
+		return block;
+	}
+	else
+	{
+		Assert(ScanDirectionIsForward(dir));
+
+		block++;
+
+		if (block >= scan->rs_nblocks)
+			block = 0;
+
+		if (block == scan->rs_startblock)
+			return InvalidBlockNumber;
+
+		if (scan->rs_numblocks != InvalidBlockNumber)
+		{
+			if (--scan->rs_numblocks == 0)
+				return InvalidBlockNumber;
+		}
+
+		/*
+		* Report our new scan position for synchronization purposes. We
+		* don't do that when moving backwards, however. That would just
+		* mess up any other forward-moving scanners.
+		*
+		* Note: we do this before checking for end of scan so that the
+		* final state of the position hint is back at the start of the
+		* rel.  That's not strictly necessary, but otherwise when you run
+		* the same query multiple times the starting position would shift
+		* a little bit backwards on every invocation, which is confusing.
+		* We don't guarantee any specific ordering in general, though.
+		*/
+		if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
+			ss_report_location(scan->rs_base.rs_rd, block);
+
+		return block;
+	}
+}
+
 /* ----------------
  *		heapgettup - fetch next heap tuple
  *
@@ -667,7 +735,6 @@ heapgettup(HeapScanDesc scan,
 	Snapshot	snapshot = scan->rs_base.rs_snapshot;
 	bool		backward = ScanDirectionIsBackward(dir);
 	BlockNumber block;
-	bool		finished;
 	Page		page;
 	OffsetNumber lineoff;
 	int			linesleft;
@@ -771,56 +838,12 @@ heapgettup(HeapScanDesc scan,
 		 */
 		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
 
-		/*
-		 * advance to next/prior page and detect end of scan
-		 */
-		if (backward)
-		{
-			finished = (block == scan->rs_startblock) ||
-				(scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false);
-			if (block == 0)
-				block = scan->rs_nblocks;
-			block--;
-		}
-		else if (scan->rs_base.rs_parallel != NULL)
-		{
-			ParallelBlockTableScanDesc pbscan =
-			(ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
-			ParallelBlockTableScanWorker pbscanwork =
-			scan->rs_parallelworkerdata;
-
-			block = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
-													 pbscanwork, pbscan);
-			finished = (block == InvalidBlockNumber);
-		}
-		else
-		{
-			block++;
-			if (block >= scan->rs_nblocks)
-				block = 0;
-			finished = (block == scan->rs_startblock) ||
-				(scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false);
-
-			/*
-			 * Report our new scan position for synchronization purposes. We
-			 * don't do that when moving backwards, however. That would just
-			 * mess up any other forward-moving scanners.
-			 *
-			 * Note: we do this before checking for end of scan so that the
-			 * final state of the position hint is back at the start of the
-			 * rel.  That's not strictly necessary, but otherwise when you run
-			 * the same query multiple times the starting position would shift
-			 * a little bit backwards on every invocation, which is confusing.
-			 * We don't guarantee any specific ordering in general, though.
-			 */
-			if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
-				ss_report_location(scan->rs_base.rs_rd, block);
-		}
+		block = heapgettup_advance_page(scan, block, dir);
 
 		/*
 		 * return NULL if we've exhausted all the pages
 		 */
-		if (finished)
+		if (block == InvalidBlockNumber)
 		{
 			if (BufferIsValid(scan->rs_cbuf))
 				ReleaseBuffer(scan->rs_cbuf);
@@ -873,7 +896,6 @@ heapgettup_pagemode(HeapScanDesc scan,
 	HeapTuple	tuple = &(scan->rs_ctup);
 	bool		backward = ScanDirectionIsBackward(dir);
 	BlockNumber block;
-	bool		finished;
 	Page		page;
 	int			lineindex;
 	OffsetNumber lineoff;
@@ -969,57 +991,11 @@ heapgettup_pagemode(HeapScanDesc scan,
 				++lineindex;
 		}
 
-		/*
-		 * if we get here, it means we've exhausted the items on this page and
-		 * it's time to move to the next.
-		 */
-		if (backward)
-		{
-			finished = (block == scan->rs_startblock) ||
-				(scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false);
-			if (block == 0)
-				block = scan->rs_nblocks;
-			block--;
-		}
-		else if (scan->rs_base.rs_parallel != NULL)
-		{
-			ParallelBlockTableScanDesc pbscan =
-			(ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
-			ParallelBlockTableScanWorker pbscanwork =
-			scan->rs_parallelworkerdata;
-
-			block = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
-													 pbscanwork, pbscan);
-			finished = (block == InvalidBlockNumber);
-		}
-		else
-		{
-			block++;
-			if (block >= scan->rs_nblocks)
-				block = 0;
-			finished = (block == scan->rs_startblock) ||
-				(scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false);
-
-			/*
-			 * Report our new scan position for synchronization purposes. We
-			 * don't do that when moving backwards, however. That would just
-			 * mess up any other forward-moving scanners.
-			 *
-			 * Note: we do this before checking for end of scan so that the
-			 * final state of the position hint is back at the start of the
-			 * rel.  That's not strictly necessary, but otherwise when you run
-			 * the same query multiple times the starting position would shift
-			 * a little bit backwards on every invocation, which is confusing.
-			 * We don't guarantee any specific ordering in general, though.
-			 */
-			if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
-				ss_report_location(scan->rs_base.rs_rd, block);
-		}
-
+		block = heapgettup_advance_page(scan, block, dir);
 		/*
 		 * return NULL if we've exhausted all the pages
 		 */
-		if (finished)
+		if (block == InvalidBlockNumber)
 		{
 			if (BufferIsValid(scan->rs_cbuf))
 				ReleaseBuffer(scan->rs_cbuf);
-- 
2.34.1