Thanks for taking a look!
On Mon, Jan 23, 2023 at 6:08 AM David Rowley <dgrowle...@gmail.com> wrote:
>
> On Thu, 19 Jan 2023 at 00:04, Peter Eisentraut
> <peter.eisentr...@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
From 5d2e8cb3388d9fa2d122db7ca8d9319f56abcaf9 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Tue, 10 Jan 2023 14:15:15 -0500
Subject: [PATCH v6 5/6] 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 08c4fb5228..15e4f3262b 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -638,6 +638,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
*
@@ -670,7 +738,6 @@ heapgettup(HeapScanDesc scan,
HeapTuple tuple = &(scan->rs_ctup);
bool backward = ScanDirectionIsBackward(dir);
BlockNumber block;
- bool finished;
Page page;
OffsetNumber lineoff;
int linesleft;
@@ -777,56 +844,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);
@@ -881,7 +904,6 @@ heapgettup_pagemode(HeapScanDesc scan,
HeapTuple tuple = &(scan->rs_ctup);
bool backward = ScanDirectionIsBackward(dir);
BlockNumber block;
- bool finished;
Page page;
int lineindex;
OffsetNumber lineoff;
@@ -973,57 +995,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
From 9e0f7dfa43e0980172d07c2341896f4cb7bf5a14 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Fri, 6 Jan 2023 12:39:38 -0500
Subject: [PATCH v6 1/6] 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 | 124 +++++++++++++++----------------
1 file changed, 62 insertions(+), 62 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index e6024a980b..ab783b2af1 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -483,6 +483,52 @@ heapgetpage(TableScanDesc sscan, BlockNumber block)
scan->rs_ntuples = ntup;
}
+
+/*
+ * helper function used by heapgettup() and heapgettup_pagemode() for
+ * NoMovementScanDirection scans.
+ * 'no movement' scans refetch the 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);
+
+ /* in pagemode, check that rs_cindex is in sync */
+ 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]);
+
+ return;
+}
+
+
/* ----------------
* heapgettup - fetch next heap tuple
*
@@ -523,6 +569,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 +635,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 +706,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 +886,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 +949,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 +1010,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 2ceae3bb5e615b5510efa6ce55ffebc0dec24ebd Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Mon, 9 Jan 2023 18:48:49 -0500
Subject: [PATCH v6 3/6] 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 8ec1df0426..a5c0ada4cd 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -610,12 +610,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;
@@ -626,89 +624,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
@@ -737,12 +707,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),
@@ -751,6 +721,7 @@ heapgettup(HeapScanDesc scan,
if (valid)
{
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
+ scan->rs_cindex = lineoff;
return;
}
}
@@ -842,13 +813,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
{
@@ -882,7 +854,6 @@ heapgettup_pagemode(HeapScanDesc scan,
BlockNumber block;
bool finished;
Page page;
- int lines;
int lineindex;
OffsetNumber lineoff;
int linesleft;
@@ -894,75 +865,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
@@ -1075,10 +1009,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 3ed3bf22d536823255a613c3ab9fe46eddfe1102 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Tue, 10 Jan 2023 14:06:56 -0500
Subject: [PATCH v6 4/6] 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 a5c0ada4cd..08c4fb5228 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -580,6 +580,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
*
@@ -638,43 +696,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 80efe06ee0945ba7762b7cc720df32d6e18dad04 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Mon, 9 Jan 2023 17:33:43 -0500
Subject: [PATCH v6 2/6] 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 ab783b2af1..8ec1df0426 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -529,6 +529,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
*
@@ -582,41 +633,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
{
@@ -638,60 +664,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
@@ -899,41 +901,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
{
@@ -952,60 +929,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 27f897e691e7bbae9504b3f3c151797345d0072a Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Tue, 10 Jan 2023 14:30:21 -0500
Subject: [PATCH v6 6/6] 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 15e4f3262b..aeb78e66c1 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -736,12 +736,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)))
{
@@ -753,17 +751,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
{
@@ -771,6 +765,7 @@ heapgettup(HeapScanDesc scan,
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
page = heapgettup_continue_page(scan, block, dir, &linesleft, &lineoff);
+ goto continue_page;
}
@@ -779,9 +774,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.
*
@@ -789,53 +788,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;
}
/*
@@ -845,41 +831,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;
}
/* ----------------
@@ -902,13 +863,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)))
{
@@ -920,23 +878,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);
@@ -945,6 +898,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;
}
@@ -952,75 +908,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
