heapgettup refactoring

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.

Hi,

On 2022-10-31 14:37:39 -0400, Melanie Plageman wrote:

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.

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

From cde2d6720f4f5ab2531c22ad4a5f0d9e6ec1039d Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
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 <melanieplageman@gmail.com>
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 <melanieplageman@gmail.com>
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

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

On Mon, Oct 31, 2022 at 9:09 PM Andres Freund <andres@anarazel.de> wrote:

From 9d8b01960463dc64ff5b111d523ff80fce3017af Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
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.

fixed.

From a894ce38c488df6546392b9f3bd894b67edf951e Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
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.

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.)

+
+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.

Fixed

+     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.

Changed to an assert

+     }
+     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?

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

+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.

So, it was like this before [1]https://github.com/postgres/postgres/blob/master/src/backend/access/heap/heapam.c#L572.
What about saving the lineoff in rs_cindex.

It is smaller, but that seems okay, right?

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

Newlines between definitions and code :)

k

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

indeed.

+     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.

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)));

+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?

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.

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().

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.

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

done.

+             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.

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

On 04.11.22 16:51, Melanie Plageman wrote:

Thanks for the review!
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.

On Wed, Nov 16, 2022 at 10:49 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 04.11.22 16:51, Melanie Plageman wrote:

Thanks for the review!
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.

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

On 30.11.22 23:34, Melanie Plageman wrote:

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.

To keep this moving along a bit, I have committed your 0002, which I
think is a nice little improvement on its own.

On Mon, Jan 2, 2023 at 5:23 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 30.11.22 23:34, Melanie Plageman wrote:

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.

To keep this moving along a bit, I have committed your 0002, which I
think is a nice little improvement on its own.

Thanks!
I've attached a rebased patchset - v4.

I also changed heapgettup_no_movement() to noinline (instead of inline).
David Rowley pointed out that this might make more sense given how
comparatively rare no movement scans are.

- Melanie

On 03.01.23 21:39, Melanie Plageman wrote:

On 30.11.22 23:34, Melanie Plageman wrote:

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.

To keep this moving along a bit, I have committed your 0002, which I
think is a nice little improvement on its own.

Thanks!
I've attached a rebased patchset - v4.

I also changed heapgettup_no_movement() to noinline (instead of inline).
David Rowley pointed out that this might make more sense given how
comparatively rare no movement scans are.

Ok, let's look through these patches starting from the top then.

v4-0001-Add-no-movement-scan-helper.patch

This makes sense overall; there is clearly some duplicate code that can
be unified.

It appears that during your rebasing you have effectively reverted your
earlier changes that have been committed as
8e1db29cdbbd218ab6ba53eea56624553c3bef8c. You should undo that.

I don't understand the purpose of the noinline maker. If it's not
necessary to inline, we can just leave it off, but there is no need to
outright prevent inlining AFAICT.

I don't know why you changed the if/else sequences. Before, the
sequence was effectively

if (forward)
{
...
}
else if (backward)
{
...
}
else
{
/* it's no movement */
}

Now it's changed to

if (no movement)
{
...
return;
}

if (forward)
{
...
}
else
{
Assert(backward);
...
}

Sure, that's the same thing, but it looks less elegant to me.

On Thu, Jan 5, 2023 at 8:52 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

Ok, let's look through these patches starting from the top then.

v4-0001-Add-no-movement-scan-helper.patch

This makes sense overall; there is clearly some duplicate code that can
be unified.

It appears that during your rebasing you have effectively reverted your
earlier changes that have been committed as
8e1db29cdbbd218ab6ba53eea56624553c3bef8c. You should undo that.

Thanks. I think I have addressed this.
I've attached a rebased v5.

I don't understand the purpose of the noinline maker. If it's not
necessary to inline, we can just leave it off, but there is no need to
outright prevent inlining AFAICT.

I have removed it.

I don't know why you changed the if/else sequences. Before, the
sequence was effectively

if (forward)
{
...
}
else if (backward)
{
...
}
else
{
/* it's no movement */
}

Now it's changed to

if (no movement)
{
...
return;
}

if (forward)
{
...
}
else
{
Assert(backward);
...
}

Sure, that's the same thing, but it looks less elegant to me.

In this commit, you could keep the original ordering of if statements. I
preferred no movement scan first because then backwards and forwards
scans' code is physically closer to the rest of the code without the
intrusion of the no movement scan code.

Ultimately, the refactor (in later patches) flips the ordering of if
statements at the top from
if (scan direction)
to
if (initial or continue)
and this isn't a very interesting distinction for no movement scans. By
dealing with no movement scan at the top, I didn't have to handle no
movement scans in the initial and continue branches in the new structure.

Also, I will note that patches 4-6 at least and perhaps 4-7 do not make
sense as separate commits. I separated them for ease of review. Each is
correct and passes tests but is not really an improvement without the
others.

- Melanie

On 10.01.23 21:31, Melanie Plageman wrote:

On Thu, Jan 5, 2023 at 8:52 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

Ok, let's look through these patches starting from the top then.

v4-0001-Add-no-movement-scan-helper.patch

This makes sense overall; there is clearly some duplicate code that can
be unified.

It appears that during your rebasing you have effectively reverted your
earlier changes that have been committed as
8e1db29cdbbd218ab6ba53eea56624553c3bef8c. You should undo that.

Thanks. I think I have addressed this.
I've attached a rebased v5.

In your v2 patch, you remove these assertions:

- /* check that rs_cindex is in sync */
- Assert(scan->rs_cindex < scan->rs_ntuples);
- Assert(lineoff == scan->rs_vistuples[scan->rs_cindex]);

Is that intentional?

I don't see any explanation, or some other equivalent code appearing
elsewhere to replace this.

On Thu, 19 Jan 2023 at 00:04, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

In your v2 patch, you remove these assertions:

- /* check that rs_cindex is in sync */
- Assert(scan->rs_cindex < scan->rs_ntuples);
- Assert(lineoff == scan->rs_vistuples[scan->rs_cindex]);

Is that intentional?

I don't see any explanation, or some other equivalent code appearing
elsewhere to replace this.

I guess it's because those asserts are not relevant unless
heapgettup_no_movement() is being called from heapgettup_pagemode().
Maybe they can be put back along the lines of:

Assert((scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) == 0 ||
scan->rs_cindex < scan->rs_ntuples);
Assert((scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) == 0 || lineoff ==
scan->rs_vistuples[scan->rs_cindex]);

but it probably would be cleaner to just do an: if
(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) { Assert(...);
Assert(...}; }

The only issue I see with that is that we don't seem to have anywhere
in the regression tests that call heapgettup_no_movement() when
rs_flags have SO_ALLOW_PAGEMODE. At least, adding an elog(NOTICE) to
heapgettup() just before calling heapgettup_no_movement() does not
seem to cause make check to fail. I wonder if any series of SQL
commands would allow us to call heapgettup_no_movement() from
heapgettup()?

I think heapgettup_no_movement() also needs a header comment more
along the lines of:

/*
* heapgettup_no_movement
* Helper function for NoMovementScanDirection direction for
heapgettup() and
* heapgettup_pagemode.
*/

I pushed the pgindent stuff that v5-0001 did along with some additions
to typedefs.list so that further runs could be done more easily as
changes are made to these patches.

David

Thanks for taking a look!

On Mon, Jan 23, 2023 at 6:08 AM David Rowley <dgrowleyml@gmail.com> wrote:

On Thu, 19 Jan 2023 at 00:04, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

In your v2 patch, you remove these assertions:

- /* check that rs_cindex is in sync */
- Assert(scan->rs_cindex < scan->rs_ntuples);
- Assert(lineoff == scan->rs_vistuples[scan->rs_cindex]);

Is that intentional?

I don't see any explanation, or some other equivalent code appearing
elsewhere to replace this.

I guess it's because those asserts are not relevant unless
heapgettup_no_movement() is being called from heapgettup_pagemode().
Maybe they can be put back along the lines of:

Assert((scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) == 0 ||
scan->rs_cindex < scan->rs_ntuples);
Assert((scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) == 0 || lineoff ==
scan->rs_vistuples[scan->rs_cindex]);

but it probably would be cleaner to just do an: if
(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) { Assert(...);
Assert(...}; }

I prefer the first method and have implemented that in attached v6.

The only issue I see with that is that we don't seem to have anywhere
in the regression tests that call heapgettup_no_movement() when
rs_flags have SO_ALLOW_PAGEMODE. At least, adding an elog(NOTICE) to
heapgettup() just before calling heapgettup_no_movement() does not
seem to cause make check to fail. I wonder if any series of SQL
commands would allow us to call heapgettup_no_movement() from
heapgettup()?

So, the places in which we set scan direction to no movement include:
- explain analyze on a ctas with no data
EXPLAIN ANALYZE CREATE TABLE foo AS SELECT 1 WITH NO DATA;
However, in standard_ExecutorRun() we only call ExecutePlan() if the
ScanDirection is not no movement, so this wouldn't hit our code
- PortalRunSelect
- PersistHoldablePortal()

I can't say I know enough about portals currently to design a test that
will hit this code, but I will poke around some more.

I think heapgettup_no_movement() also needs a header comment more
along the lines of:

/*
* heapgettup_no_movement
* Helper function for NoMovementScanDirection direction for
heapgettup() and
* heapgettup_pagemode.
*/

I've added a comment but I didn't include the function name in it -- I
find it repetitive when the comments above functions do that -- however,
I'm not strongly attached to that.

I pushed the pgindent stuff that v5-0001 did along with some additions
to typedefs.list so that further runs could be done more easily as
changes are made to these patches.

Cool!

- Melanie

On Tue, Jan 24, 2023 at 04:17:23PM -0500, Melanie Plageman wrote:

Thanks for taking a look!

On Mon, Jan 23, 2023 at 6:08 AM David Rowley <dgrowleyml@gmail.com> wrote:

On Thu, 19 Jan 2023 at 00:04, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

In your v2 patch, you remove these assertions:

- /* check that rs_cindex is in sync */
- Assert(scan->rs_cindex < scan->rs_ntuples);
- Assert(lineoff == scan->rs_vistuples[scan->rs_cindex]);

Is that intentional?

I don't see any explanation, or some other equivalent code appearing
elsewhere to replace this.

I guess it's because those asserts are not relevant unless
heapgettup_no_movement() is being called from heapgettup_pagemode().
Maybe they can be put back along the lines of:

Assert((scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) == 0 ||
scan->rs_cindex < scan->rs_ntuples);
Assert((scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) == 0 || lineoff ==
scan->rs_vistuples[scan->rs_cindex]);

but it probably would be cleaner to just do an: if
(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) { Assert(...);
Assert(...}; }

I prefer the first method and have implemented that in attached v6.

The only issue I see with that is that we don't seem to have anywhere
in the regression tests that call heapgettup_no_movement() when
rs_flags have SO_ALLOW_PAGEMODE. At least, adding an elog(NOTICE) to
heapgettup() just before calling heapgettup_no_movement() does not
seem to cause make check to fail. I wonder if any series of SQL
commands would allow us to call heapgettup_no_movement() from
heapgettup()?

So, the places in which we set scan direction to no movement include:
- explain analyze on a ctas with no data
EXPLAIN ANALYZE CREATE TABLE foo AS SELECT 1 WITH NO DATA;
However, in standard_ExecutorRun() we only call ExecutePlan() if the
ScanDirection is not no movement, so this wouldn't hit our code
- PortalRunSelect
- PersistHoldablePortal()

I can't say I know enough about portals currently to design a test that
will hit this code, but I will poke around some more.

I don't think we can write a test for this afterall. I've started
another thread on the topic over here:

/messages/by-id/CAAKRu_bvkhka0CZQun28KTqhuUh5ZqY=_T8QEqZqOL02rpi2bw@mail.gmail.com

"On Wed, 25 Jan 2023 at 10:17, Melanie Plageman
<melanieplageman@gmail.com> wrote:

I've added a comment but I didn't include the function name in it -- I
find it repetitive when the comments above functions do that -- however,
I'm not strongly attached to that.

I think the general format for header comments is:

/*
* <function name>
*\t\t<brief summary of what function does>
*
* [Further details]
*/

We've certainly got places that don't follow that, but I don't think
that's any reason to have no comment or invent some new format.

heapam.c seems to have some other format where we do: "<function name>
- <brief summary of what function does>". I generally just try to copy
the style from the surrounding code. I think generally, people won't
argue if you follow the style from the surrounding code, but there'd
be exceptions to that, I'm sure.

I'll skip further review of 0001 here as the whole
ScanDirectionNoMovement case is being discussed on the other thread.

v6-0002:

1. heapgettup_initial_block() needs a header comment to mention what
it does and what it returns. It would be good to make it obvious that
it returns InvalidBlockNumber when there are no blocks to scan.

2. After heapgettup_initial_block(), you're checking "if (block ==
InvalidBlockNumber). It might be worth a mention something like

/*
* Check if we got to the end of the scan already. This could happen for
* an empty relation or if parallel workers scanned everything before we
* got a chance.
*/

the backward scan comment wouldn't mention parallel workers.

v6-0003:

3. Can you explain why you removed the snapshot local variable in heapgettup()?

4. I think it might be a good idea to use unlikely() in if
(!scan->rs_inited). The idea is to help coax the compiler into moving
that code off to a cold path. That's likely especially important if
heapgettup_initial_block is inlined, which I see it is marked as.

v6-0004:

5. heapgettup_start_page() and heapgettup_continue_page() both need a
header comment to explain what they do and what the inputs and output
arguments are.

6. I'm not too sure what the following comment means:

/* block and lineoff now reference the physically next tid */

"block" is just a parameter to the function and its value is not
adjusted. The comment makes it sound like something was changed.

(I think these might be just not well updated from having split this
out from the 0006 patch as the same comment makes more sense in 0006)

v6-0005:

7. heapgettup_advance_block() needs a header comment.

8. Is there a reason why heapgettup_advance_block() handle backward
scans first? I'd expect you should just follow the lead of the other
functions and do ScanDirectionIsForward first.

v6-0006

David

v7 attached

On Fri, Jan 27, 2023 at 10:34 PM David Rowley <dgrowleyml@gmail.com> wrote:

"On Wed, 25 Jan 2023 at 10:17, Melanie Plageman
<melanieplageman@gmail.com> wrote:

I've added a comment but I didn't include the function name in it -- I
find it repetitive when the comments above functions do that -- however,
I'm not strongly attached to that.

I think the general format for header comments is:

/*
* <function name>
*\t\t<brief summary of what function does>
*
* [Further details]
*/

We've certainly got places that don't follow that, but I don't think
that's any reason to have no comment or invent some new format.

heapam.c seems to have some other format where we do: "<function name>
- <brief summary of what function does>". I generally just try to copy
the style from the surrounding code. I think generally, people won't
argue if you follow the style from the surrounding code, but there'd
be exceptions to that, I'm sure.

I have followed the same convention as the other functions in heapam.c
in the various helper functions comments I've added in this version.

v6-0002:

1. heapgettup_initial_block() needs a header comment to mention what
it does and what it returns. It would be good to make it obvious that
it returns InvalidBlockNumber when there are no blocks to scan.

I've done this.

2. After heapgettup_initial_block(), you're checking "if (block ==
InvalidBlockNumber). It might be worth a mention something like

/*
* Check if we got to the end of the scan already. This could happen for
* an empty relation or if parallel workers scanned everything before we
* got a chance.
*/

the backward scan comment wouldn't mention parallel workers.

I've done this as well.

v6-0003:

3. Can you explain why you removed the snapshot local variable in heapgettup()?

In the subsequent commit, the helpers I add call TestForOldSnapshot(),
and I didn't want to pass in the snapshot as a separate parameter since
I already need to pass the scan descriptor. I thought it was confusing
to have a local variable (snapshot) used in some places and the one in
the scan used in others. This "streamlining" commit also reduces the
number of times the snapshot variable is used, making it less necessary
to have a local variable.

I didn't remove the snapshot local variable in the same commit as adding
the helpers because I thought it made the diff harder to understand (for
review, the final commit should likely not be separate patches).

4. I think it might be a good idea to use unlikely() in if
(!scan->rs_inited). The idea is to help coax the compiler into moving
that code off to a cold path. That's likely especially important if
heapgettup_initial_block is inlined, which I see it is marked as.

I've gone ahead and added unlikely. However, should I perhaps skip
inlining the heapgettup_initial_block() function?

v6-0004:

5. heapgettup_start_page() and heapgettup_continue_page() both need a
header comment to explain what they do and what the inputs and output
arguments are.

I've added these. I've also removed an unused parameter to both, block.

6. I'm not too sure what the following comment means:

/* block and lineoff now reference the physically next tid */

"block" is just a parameter to the function and its value is not
adjusted. The comment makes it sound like something was changed.

(I think these might be just not well updated from having split this
out from the 0006 patch as the same comment makes more sense in 0006)

Yes, that is true. I've updated it to just mention lineoff.

v6-0005:

7. heapgettup_advance_block() needs a header comment.

8. Is there a reason why heapgettup_advance_block() handle backward
scans first? I'd expect you should just follow the lead of the other
functions and do ScanDirectionIsForward first.

The reason I do this is that backwards scans cannot be parallel, so
handling backwards scans first let me return, then handle parallel
scans, then forward scans. This reduced the level of nesting/if
statements for all of the code in this function.

- Melanie

On Tue, 31 Jan 2023 at 12:18, Melanie Plageman
<melanieplageman@gmail.com> wrote:

On Fri, Jan 27, 2023 at 10:34 PM David Rowley <dgrowleyml@gmail.com> wrote:

4. I think it might be a good idea to use unlikely() in if
(!scan->rs_inited). The idea is to help coax the compiler into moving
that code off to a cold path. That's likely especially important if
heapgettup_initial_block is inlined, which I see it is marked as.

I've gone ahead and added unlikely. However, should I perhaps skip
inlining the heapgettup_initial_block() function?

I'm not sure of the exact best combination of functions to mark as
inline. I did try the v7 patchset from 0002 to 0006 on top of c2891175
and I found that the performance is slightly better after removing
inline from all 4 of the helper functions. However, I think if we do
unlikely() and the function is moved into the cold path then it
matters less if it's inlined.

create table a (a int);
insert into a select x from generate_series(1,1000000)x;
vacuum freeze a;

$ cat seqscan.sql
select * from a where a = 0;
$ cat countall.sql
select count(*) from a;

seqscan.sql filters out all rows and countall.sql returns all rows and
does an aggregate so we don't have to return all those in the query.

max_parallel_workers_per_gather=0;

master
$ psql -c "select pg_prewarm('a')" postgres > /dev/null && for i in
{1..3}; do pgbench -n -f seqscan.sql -M prepared -T 10 postgres | grep
tps; done
tps = 25.464091 (without initial connection time)
tps = 25.117001 (without initial connection time)
tps = 25.141646 (without initial connection time)

$ psql -c "select pg_prewarm('a')" postgres > /dev/null && for i in
{1..3}; do pgbench -n -f countall.sql -M prepared -T 10 postgres |
grep tps; done
tps = 27.906307 (without initial connection time)
tps = 27.527580 (without initial connection time)
tps = 27.563035 (without initial connection time)

master + v7
$ psql -c "select pg_prewarm('a')" postgres > /dev/null && for i in
{1..3}; do pgbench -n -f seqscan.sql -M prepared -T 10 postgres | grep
tps; done
tps = 25.920370 (without initial connection time)
tps = 25.680052 (without initial connection time)
tps = 24.988895 (without initial connection time)

$ psql -c "select pg_prewarm('a')" postgres > /dev/null && for i in
{1..3}; do pgbench -n -f countall.sql -M prepared -T 10 postgres |
grep tps; done
tps = 33.783122 (without initial connection time)
tps = 33.248571 (without initial connection time)
tps = 33.512984 (without initial connection time)

master + v7 + inline removed
$ psql -c "select pg_prewarm('a')" postgres > /dev/null && for i in
{1..3}; do pgbench -n -f seqscan.sql -M prepared -T 10 postgres | grep
tps; done
tps = 27.680115 (without initial connection time)
tps = 26.418562 (without initial connection time)
tps = 26.166800 (without initial connection time)

$ psql -c "select pg_prewarm('a')" postgres > /dev/null && for i in
{1..3}; do pgbench -n -f countall.sql -M prepared -T 10 postgres |
grep tps; done
tps = 33.948588 (without initial connection time)
tps = 33.684966 (without initial connection time)
tps = 33.946700 (without initial connection time)

You can see that v7 helps countall.sql quite a bit. It seems to also
help a little bit with seqscan.sql. v7 + inline removed makes
seqscan.sql a decent amount faster than both master and master + v7.

David

On Tue, 31 Jan 2023 at 12:18, Melanie Plageman
<melanieplageman@gmail.com> wrote:

v7 attached

I've been looking over the v7-0002 patch today and I did make a few
adjustments to heapgettup_initial_block() as I would prefer to see the
branching of each of all these helper functions follow the pattern of:

if (<forward scan>)
{
if (<parallel scan>)
<parallel stuff>
else
<serial stuff>
}
else
{
<backwards serial stuff>
}

which wasn't quite what the function was doing.

Along the way, I noticed that 0002 has a subtle bug that does not seem
to be present once the remaining patches are applied. I think I'm
happier to push these along the lines of how you have them in the
patches, so I've held off pushing for now due to the bug and the
change I had to make to fix it.

The problem is around the setting of scan->rs_inited = true; you've
moved that into heapgettup_initial_block() and you've correctly not
initialised the scan for empty tables when you return
InvalidBlockNumber, however, you've not correctly considered the fact
that table_block_parallelscan_nextpage() could also return
InvalidBlockNumber if the parallel workers manage to grab all of the
blocks before the current process gets the first block. I don't know
for sure, but it looks like this could cause problems when
heapgettup() or heapgettup_pagemode() got called again for a rescan.
We'd have returned the NULL tuple to indicate that no further tuples
exist, but we'll have left rs_inited set to true which looks like
it'll cause issues.

I wondered if it might be better to do the scan->rs_inited = true; in
heapgettup() and heapgettup_pagemode() instead. The attached v8 patch
does it this way. Despite this fixing that bug, I think this might be
a slightly better division of duties.

If you're ok with the attached (and everyone else is too), then I can
push it in the (NZ) morning. The remaining patches would need to be
rebased due to my changes.

David

On Thu, Feb 02, 2023 at 12:21:20AM +1300, David Rowley wrote:

On Tue, 31 Jan 2023 at 12:18, Melanie Plageman
<melanieplageman@gmail.com> wrote:

v7 attached

I've been looking over the v7-0002 patch today and I did make a few
adjustments to heapgettup_initial_block() as I would prefer to see the
branching of each of all these helper functions follow the pattern of:

if (<forward scan>)
{
if (<parallel scan>)
<parallel stuff>
else
<serial stuff>
}
else
{
<backwards serial stuff>
}

which wasn't quite what the function was doing.

I'm fine with this. One code comment about the new version inline.

Along the way, I noticed that 0002 has a subtle bug that does not seem
to be present once the remaining patches are applied. I think I'm
happier to push these along the lines of how you have them in the
patches, so I've held off pushing for now due to the bug and the
change I had to make to fix it.

The problem is around the setting of scan->rs_inited = true; you've
moved that into heapgettup_initial_block() and you've correctly not
initialised the scan for empty tables when you return
InvalidBlockNumber, however, you've not correctly considered the fact
that table_block_parallelscan_nextpage() could also return
InvalidBlockNumber if the parallel workers manage to grab all of the
blocks before the current process gets the first block. I don't know
for sure, but it looks like this could cause problems when
heapgettup() or heapgettup_pagemode() got called again for a rescan.
We'd have returned the NULL tuple to indicate that no further tuples
exist, but we'll have left rs_inited set to true which looks like
it'll cause issues.

Ah, yes. In the later patches in the series, I handle all end of scan
cases (regardless of whether or not there was a beginning) in a single
place at the end of the function. There I release the buffer and reset
all state -- including setting rs_inited to false. So, that made it okay
to set rs_inited to true in heapgettup_initial_block().

When splitting it up, I made a mistake and missed the case you
mentioned. Thanks for catching that!

FWIW, I like setting rs_inited in heapgettup_initial_block() better in
the final refactor, but I agree with you that in this patch on its own
it is better in the body of heapgettup() and heapgettup_pagemode().

I wondered if it might be better to do the scan->rs_inited = true; in
heapgettup() and heapgettup_pagemode() instead. The attached v8 patch
does it this way. Despite this fixing that bug, I think this might be
a slightly better division of duties.

LGTM.

From cbd37463bdaa96afed4c7c739c8e91b770a9f8a7 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Wed, 1 Feb 2023 19:35:16 +1300
Subject: [PATCH v8] Refactor heapam.c adding heapgettup_initial_block function

Here we adjust heapgettup() and heapgettup_pagemode() to move the code
that fetches the first block out into a helper function. This removes
some code duplication.

Author: Melanie Plageman
Reviewed-by: David Rowley
Discussion: /messages/by-id/CAAKRu_bvkhka0CZQun28KTqhuUh5ZqY=_T8QEqZqOL02rpi2bw@mail.gmail.com
---
src/backend/access/heap/heapam.c | 225 ++++++++++++++-----------------
1 file changed, 103 insertions(+), 122 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 0a8bac25f5..40168cc9ca 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -483,6 +483,67 @@ heapgetpage(TableScanDesc sscan, BlockNumber block)
scan->rs_ntuples = ntup;
}

+/*
+ * heapgettup_initial_block - return the first BlockNumber to scan
+ *
+ * Returns InvalidBlockNumber when there are no blocks to scan.  This can
+ * occur with empty tables and in parallel scans when parallel workers get all
+ * of the pages before we can get a chance to get our first page.
+ */
+static BlockNumber
+heapgettup_initial_block(HeapScanDesc scan, ScanDirection dir)
+{
+	Assert(!scan->rs_inited);
+
+	/* When there are no pages to scan, return InvalidBlockNumber */
+	if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
+		return InvalidBlockNumber;
+
+	if (ScanDirectionIsForward(dir))
+	{
+		/* serial scan */
+		if (scan->rs_base.rs_parallel == NULL)
+			return scan->rs_startblock;

I believe this else is superfluous since we returned above.

+		else
+		{
+			/* parallel scan */
+			table_block_parallelscan_startblock_init(scan->rs_base.rs_rd,
+													 scan->rs_parallelworkerdata,
+													 (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel);
+
+			/* may return InvalidBlockNumber if there are no more blocks */
+			return table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
+													 scan->rs_parallelworkerdata,
+													 (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel);
+		}
+	}

...

@@ -889,62 +892,40 @@ heapgettup_pagemode(HeapScanDesc scan,
-		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 */

I think this is an unintentional diff.

+ /* block and lineindex now reference the previous visible tid */
linesleft = lineindex + 1;
}

- Melanie

On Thu, 2 Feb 2023 at 10:12, Melanie Plageman <melanieplageman@gmail.com> wrote:

FWIW, I like setting rs_inited in heapgettup_initial_block() better in
the final refactor, but I agree with you that in this patch on its own
it is better in the body of heapgettup() and heapgettup_pagemode().

We can reconsider that when we get to that patch. It just felt a bit
ugly to add an InvalidBlockNumber check after calling
table_block_parallelscan_nextpage()

I believe this else is superfluous since we returned above.

TBH, that's on purpose. I felt that it just looked better that way as
the code all fitted onto my screen. I think if the function was
longer and people had to scroll down to read it, it can often be
better to return and reduce the nesting. This allows you to mentally
not that a certain case is handled above. However, since all these
helper functions seem to fit onto a screen without too much trouble,
it just seems better (to me) if they all follow the format that I
mentioned earlier. I might live to regret that as we often see
get-rid-of-useless-else-clause patches coming up. I'm starting to
wonder if someone's got some alarm that goes off every time one gets
committed, but we'll see. I'd much rather have consistency between the
helper functions than save a few bytes on tab characters. It would be
different if the indentation were shifting things too far right, but
that's not going to happen in a function that all fits onto a screen
at once.

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. 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]/messages/by-id/20221101010948.hsf33emgnwzvil4a@awork3.anarazel.de earlier in this thread
about that, so thought we should move away from looking at the last
tuple to get this number

I've attached the rebased and updated patch.

David

[1]: /messages/by-id/20221101010948.hsf33emgnwzvil4a@awork3.anarazel.de