On Thu, Jan 5, 2023 at 8:52 AM Peter Eisentraut
<peter.eisentr...@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
From 9b51959c483812c30ca848b66a0e6e3a807ab03f Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Mon, 9 Jan 2023 17:33:43 -0500
Subject: [PATCH v5 3/7] Add heapgettup_initial_block() helper
---
src/backend/access/heap/heapam.c | 212 ++++++++++++-------------------
1 file changed, 82 insertions(+), 130 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index c80b547809..b9a1aac3ca 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -521,6 +521,57 @@ heapgettup_no_movement(HeapScanDesc scan)
}
+static inline BlockNumber
+heapgettup_initial_block(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
*
@@ -574,41 +625,16 @@ heapgettup(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_block(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
{
@@ -630,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_block(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
@@ -891,41 +893,16 @@ 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_block(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
{
@@ -944,60 +921,35 @@ 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_block(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 */
linesleft = lineindex + 1;
--
2.34.1
From e657f3dbe734788749cf86d4ff38201a188f1a93 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Fri, 6 Jan 2023 12:44:03 -0500
Subject: [PATCH v5 1/7] pgindent heapgettup* functions
---
src/backend/access/heap/heapam.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 63c4f01f0f..4df19fe76c 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -561,7 +561,7 @@ heapgettup(HeapScanDesc scan,
}
}
else
- block = scan->rs_startblock; /* first page */
+ block = scan->rs_startblock; /* first page */
heapgetpage((TableScanDesc) scan, block);
lineoff = FirstOffsetNumber; /* first offnum */
scan->rs_inited = true;
@@ -569,7 +569,7 @@ heapgettup(HeapScanDesc scan,
else
{
/* continue from previously returned page/tuple */
- block = scan->rs_cblock; /* current page */
+ block = scan->rs_cblock; /* current page */
lineoff = /* next offnum */
OffsetNumberNext(ItemPointerGetOffsetNumber(&(tuple->t_self)));
}
@@ -623,7 +623,7 @@ heapgettup(HeapScanDesc scan,
else
{
/* continue from previously returned page/tuple */
- block = scan->rs_cblock; /* current page */
+ block = scan->rs_cblock; /* current page */
}
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
@@ -769,7 +769,7 @@ heapgettup(HeapScanDesc scan,
scan->rs_parallelworkerdata;
block = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
+ pbscanwork, pbscan);
finished = (block == InvalidBlockNumber);
}
else
@@ -899,7 +899,7 @@ heapgettup_pagemode(HeapScanDesc scan,
}
}
else
- block = scan->rs_startblock; /* first page */
+ block = scan->rs_startblock; /* first page */
heapgetpage((TableScanDesc) scan, block);
lineindex = 0;
scan->rs_inited = true;
@@ -907,7 +907,7 @@ heapgettup_pagemode(HeapScanDesc scan,
else
{
/* continue from previously returned page/tuple */
- block = scan->rs_cblock; /* current page */
+ block = scan->rs_cblock; /* current page */
lineindex = scan->rs_cindex + 1;
}
@@ -958,7 +958,7 @@ heapgettup_pagemode(HeapScanDesc scan,
else
{
/* continue from previously returned page/tuple */
- block = scan->rs_cblock; /* current page */
+ block = scan->rs_cblock; /* current page */
}
page = BufferGetPage(scan->rs_cbuf);
@@ -1078,7 +1078,7 @@ heapgettup_pagemode(HeapScanDesc scan,
scan->rs_parallelworkerdata;
block = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
+ pbscanwork, pbscan);
finished = (block == InvalidBlockNumber);
}
else
--
2.34.1
From 732e240633d98665546e5d40bc722b9199247993 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Mon, 9 Jan 2023 18:48:49 -0500
Subject: [PATCH v5 4/7] Streamline heapgettup*() for refactor
Backward and forward scan share much of the page acquisition setup code.
They differ only in calculating where in the page the current tuple is.
Consolidate the common page acquisition code to pave the way for helper
functions.
---
src/backend/access/heap/heapam.c | 185 ++++++++++---------------------
src/include/access/heapam.h | 6 +-
2 files changed, 64 insertions(+), 127 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index b9a1aac3ca..7998304bea 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -602,12 +602,10 @@ heapgettup(HeapScanDesc scan,
ScanKey key)
{
HeapTuple tuple = &(scan->rs_ctup);
- Snapshot snapshot = scan->rs_base.rs_snapshot;
bool backward = ScanDirectionIsBackward(dir);
BlockNumber block;
bool finished;
Page page;
- int lines;
OffsetNumber lineoff;
int linesleft;
ItemId lpp;
@@ -618,89 +616,61 @@ heapgettup(HeapScanDesc scan,
return;
}
- /*
- * calculate next starting lineoff, given scan direction
- */
- if (ScanDirectionIsForward(dir))
+ if (!scan->rs_inited)
{
- if (!scan->rs_inited)
- {
- block = heapgettup_initial_block(scan, dir);
+ block = heapgettup_initial_block(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 */
+ TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd,
+ page);
- linesleft = lines - lineoff + 1;
+ linesleft = PageGetMaxOffsetNumber(page) - FirstOffsetNumber + 1;
+
+ if (ScanDirectionIsForward(dir))
+ lineoff = FirstOffsetNumber;
+ else
+ lineoff = (OffsetNumber) linesleft;
}
else
{
- Assert(backward);
-
- if (!scan->rs_inited)
- {
- block = heapgettup_initial_block(scan, dir);
-
- if (block == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
+ block = scan->rs_cblock;
- heapgetpage((TableScanDesc) scan, block);
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+ LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+ page = BufferGetPage(scan->rs_cbuf);
+ TestForOldSnapshot(scan->rs_base.rs_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);
+ 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));
+ linesleft = lineoff;
}
- /* block and lineoff now reference the physically previous tid */
-
- linesleft = lineoff;
}
+
+
/*
* advance the scan until we find a qualifying tuple or run out of stuff
* to scan
@@ -729,12 +699,12 @@ heapgettup(HeapScanDesc scan,
* if current tuple qualifies, return it.
*/
valid = HeapTupleSatisfiesVisibility(tuple,
- snapshot,
+ scan->rs_base.rs_snapshot,
scan->rs_cbuf);
HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
tuple, scan->rs_cbuf,
- snapshot);
+ scan->rs_base.rs_snapshot);
if (valid && key != NULL)
valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
@@ -743,6 +713,7 @@ heapgettup(HeapScanDesc scan,
if (valid)
{
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
+ scan->rs_cindex = lineoff;
return;
}
}
@@ -834,13 +805,14 @@ heapgettup(HeapScanDesc scan,
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
page = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
- lines = PageGetMaxOffsetNumber(page);
- linesleft = lines;
+
+ TestForOldSnapshot(scan->rs_base.rs_snapshot,
+ scan->rs_base.rs_rd, page);
+ 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,75 +857,38 @@ 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_block(scan, dir);
+ block = heapgettup_initial_block(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_block(scan, dir);
-
- if (block == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
+ /* continue from previously returned page/tuple */
+ block = scan->rs_cblock;
+ 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;
}
+
/*
* advance the scan until we find a qualifying tuple or run out of stuff
* to scan
@@ -1067,10 +1001,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 417108f1e0..5a4df34e48 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -72,8 +72,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 6964bd9001f6eb0992d783321bc4264b78d8a741 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Fri, 6 Jan 2023 12:39:38 -0500
Subject: [PATCH v5 2/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 | 116 ++++++++++++++-----------------
1 file changed, 54 insertions(+), 62 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 4df19fe76c..c80b547809 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -483,6 +483,44 @@ heapgetpage(TableScanDesc sscan, BlockNumber block)
scan->rs_ntuples = ntup;
}
+
+/*
+* ``no movement'' scan direction: refetch prior tuple
+*/
+static void
+heapgettup_no_movement(HeapScanDesc scan)
+{
+ BlockNumber block;
+ OffsetNumber lineoff;
+ ItemId lpp;
+ Page page;
+ HeapTuple tuple = &(scan->rs_ctup);
+
+ 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);
+
+ return;
+}
+
+
/* ----------------
* heapgettup - fetch next heap tuple
*
@@ -523,6 +561,12 @@ heapgettup(HeapScanDesc scan,
int linesleft;
ItemId lpp;
+ if (unlikely(ScanDirectionIsNoMovement(dir)))
+ {
+ heapgettup_no_movement(scan);
+ return;
+ }
+
/*
* calculate next starting lineoff, given scan direction
*/
@@ -583,8 +627,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);
@@ -653,34 +698,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
@@ -861,6 +878,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
*/
@@ -918,8 +941,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);
@@ -978,38 +1002,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;
- }
/*
* advance the scan until we find a qualifying tuple or run out of stuff
--
2.34.1
From ab4f2b84dde9f54f133852e3978e9a7252fb5516 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Tue, 10 Jan 2023 14:06:56 -0500
Subject: [PATCH v5 5/7] Add heapgettup() helpers
Add heapgettup_start_page() and heapgettup_continue_page(), helper
functions for heapgettup().
---
src/backend/access/heap/heapam.c | 91 +++++++++++++++++++++-----------
1 file changed, 60 insertions(+), 31 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 7998304bea..bd3bc3e52f 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -572,6 +572,64 @@ heapgettup_initial_block(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
*
@@ -630,43 +688,14 @@ heapgettup(HeapScanDesc scan,
heapgetpage((TableScanDesc) scan, block);
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
- page = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd,
- page);
-
- linesleft = PageGetMaxOffsetNumber(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;
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
- 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;
- }
+ page = heapgettup_continue_page(scan, block, dir, &linesleft, &lineoff);
}
--
2.34.1
From 247748906fd877506fbcc5c1c4e905b360499539 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Tue, 10 Jan 2023 14:30:21 -0500
Subject: [PATCH v5 7/7] Refactor heapgettup() and heapgettup_pagemode()
---
src/backend/access/heap/heapam.c | 244 ++++++++++++-------------------
1 file changed, 91 insertions(+), 153 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 65c05d1b15..675f003bee 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -728,12 +728,10 @@ heapgettup(HeapScanDesc scan,
ScanKey key)
{
HeapTuple tuple = &(scan->rs_ctup);
- bool backward = ScanDirectionIsBackward(dir);
BlockNumber block;
Page page;
OffsetNumber lineoff;
int linesleft;
- ItemId lpp;
if (unlikely(ScanDirectionIsNoMovement(dir)))
{
@@ -745,17 +743,13 @@ heapgettup(HeapScanDesc scan,
{
block = heapgettup_initial_block(scan, dir);
- if (block == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
-
- heapgetpage((TableScanDesc) scan, block);
+ /*
+ * 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));
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
- page = heapgettup_start_page(scan, block, dir, &linesleft, &lineoff);
}
else
{
@@ -763,6 +757,7 @@ heapgettup(HeapScanDesc scan,
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
page = heapgettup_continue_page(scan, block, dir, &linesleft, &lineoff);
+ goto continue_page;
}
@@ -771,9 +766,13 @@ heapgettup(HeapScanDesc scan,
* 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.
*
@@ -781,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;
-
- 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,
- scan->rs_base.rs_snapshot,
- scan->rs_cbuf);
+ bool visible;
+ ItemId lpp = PageGetItemId(page, lineoff);
- HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
- tuple, scan->rs_cbuf,
- scan->rs_base.rs_snapshot);
+ if (!ItemIdIsNormal(lpp))
+ continue;
- if (valid && key != NULL)
- valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
- nkeys, key);
- if (valid)
- {
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
- scan->rs_cindex = lineoff;
- return;
- }
- }
+ tuple->t_data = (HeapTupleHeader) PageGetItem(page, lpp);
+ tuple->t_len = ItemIdGetLength(lpp);
+ ItemPointerSet(&(tuple->t_self), scan->rs_cblock, lineoff);
/*
- * otherwise move to the next item on the page
+ * if current tuple qualifies, return it.
*/
- --linesleft;
- if (backward)
- {
- --lpp; /* move back in this page's ItemId array */
- --lineoff;
- }
- else
- {
- ++lpp; /* move forward in this page's ItemId array */
- ++lineoff;
- }
+ 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 (key && !HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
+ nkeys, key))
+ continue;
+
+ LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
+ scan->rs_cindex = lineoff;
+ return;
}
/*
@@ -837,41 +823,16 @@ heapgettup(HeapScanDesc scan,
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
block = heapgettup_advance_block(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);
-
- page = BufferGetPage(scan->rs_cbuf);
+ /* end of scan */
+ if (BufferIsValid(scan->rs_cbuf))
+ ReleaseBuffer(scan->rs_cbuf);
- TestForOldSnapshot(scan->rs_base.rs_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,23 +870,18 @@ heapgettup_pagemode(HeapScanDesc scan,
{
block = heapgettup_initial_block(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
{
/* continue from previously returned page/tuple */
block = scan->rs_cblock;
+
page = BufferGetPage(scan->rs_cbuf);
TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
@@ -937,6 +890,9 @@ 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 */
+
+ goto continue_page;
}
@@ -944,75 +900,57 @@ heapgettup_pagemode(HeapScanDesc scan,
* 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 current tuple qualifies, return it. otherwise move to the
+ * next item on the block
*/
- 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 (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_block(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;
- }
+ block = heapgettup_advance_block(scan, block, dir);
+ }
- heapgetpage((TableScanDesc) scan, block);
+ /* 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;
- 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;
- }
}
--
2.34.1
From 1b56abd762108ab6d50b9f8d7ebd43dbae9329c3 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Tue, 10 Jan 2023 14:15:15 -0500
Subject: [PATCH v5 6/7] Add heapgettup_advance_block() helper
---
src/backend/access/heap/heapam.c | 167 +++++++++++++------------------
1 file changed, 72 insertions(+), 95 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index bd3bc3e52f..65c05d1b15 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -630,6 +630,74 @@ heapgettup_continue_page(HeapScanDesc scan, BlockNumber block, ScanDirection
return page;
}
+static inline BlockNumber
+heapgettup_advance_block(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
*
@@ -662,7 +730,6 @@ heapgettup(HeapScanDesc scan,
HeapTuple tuple = &(scan->rs_ctup);
bool backward = ScanDirectionIsBackward(dir);
BlockNumber block;
- bool finished;
Page page;
OffsetNumber lineoff;
int linesleft;
@@ -769,56 +836,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_block(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;
@@ -965,57 +987,12 @@ 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_block(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
