v7 attached
On Fri, Jan 27, 2023 at 10:34 PM David Rowley <dgrowle...@gmail.com> wrote:
>
> "On Wed, 25 Jan 2023 at 10:17, Melanie Plageman
> <melanieplage...@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
From 62416fee879a1ea6a7ca7ec132227701d35d7468 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Mon, 9 Jan 2023 17:33:43 -0500
Subject: [PATCH v7 2/6] Add heapgettup_initial_block() helper
---
src/backend/access/heap/heapam.c | 226 ++++++++++++++-----------------
1 file changed, 102 insertions(+), 124 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 75f68b4e79..ff2e64822a 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -484,6 +484,67 @@ heapgetpage(TableScanDesc sscan, BlockNumber block)
}
+/*
+ * heapgettup_initial_block - helper for heapgettup() and heapgettup_pagemode()
+ *
+ * Determines and returns the first block to scan, given the scan direction and
+ * whether or not it is parallel. If the relation is empty or the current
+ * parallel worker finds the scan has already been completed by other workers,
+ * return InvalidBlockNumber.
+ *
+ * Note that this helper does set state in the scan descriptor.
+ */
+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
*
@@ -534,41 +595,21 @@ heapgettup(HeapScanDesc scan,
{
if (!scan->rs_inited)
{
+ block = heapgettup_initial_block(scan, dir);
+
/*
- * return null immediately if relation is empty
+ * Check if we have reached the end of the scan already. This
+ * could happen if the table is empty or if the other workers in a
+ * parallel scan have already finished the scan.
*/
- if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
+ 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
{
@@ -589,60 +630,39 @@ heapgettup(HeapScanDesc scan,
}
else
{
- /* backward parallel scan not supported */
- Assert(scan->rs_base.rs_parallel == NULL);
-
if (!scan->rs_inited)
{
+ block = heapgettup_initial_block(scan, dir);
+
/*
- * return null immediately if relation is empty
+ * Check if we have reached the end of the scan already. This
+ * could happen if the table is empty.
*/
- if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
+ 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
@@ -847,41 +867,21 @@ heapgettup_pagemode(HeapScanDesc scan,
{
if (!scan->rs_inited)
{
+ block = heapgettup_initial_block(scan, dir);
+
/*
- * return null immediately if relation is empty
+ * Check if we have reached the end of the scan already. This
+ * could happen if the table is empty or if the other workers in a
+ * parallel scan have already finished the scan.
*/
- if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
+ 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
{
@@ -899,60 +899,38 @@ heapgettup_pagemode(HeapScanDesc scan,
}
else
{
- /* backward parallel scan not supported */
- Assert(scan->rs_base.rs_parallel == NULL);
-
if (!scan->rs_inited)
{
+ block = heapgettup_initial_block(scan, dir);
+
/*
- * return null immediately if relation is empty
+ * Check if we have reached the end of the scan already. This
+ * could happen if the table is empty.
*/
- if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
+ 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.37.2
From 7b3359a4ddad7083602d607f3ab95210f86eb2ae Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Fri, 6 Jan 2023 12:39:38 -0500
Subject: [PATCH v7 1/6] Remove NoMovementScanDirection inconsistencies
Remove use of NoMovementScanDirection for index scans. Unordered indexes
will now always use ForwardScanDirection.
Remove unreachable code in heapgettup() and heapgettup_pagemode()
handling NoMovementScanDirection.
Add assertions in case table AMs try to use scan directions other than
forward and backward.
Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_bvkhka0CZQun28KTqhuUh5ZqY%3D_T8QEqZqOL02rpi2bw%40mail.gmail.com
---
src/backend/access/heap/heapam.c | 74 ++++--------------------
src/backend/commands/explain.c | 3 -
src/backend/executor/nodeIndexonlyscan.c | 17 +++---
src/backend/executor/nodeIndexscan.c | 17 +++---
src/backend/optimizer/path/indxpath.c | 8 +--
src/backend/optimizer/util/pathnode.c | 5 +-
src/include/access/tableam.h | 6 ++
src/test/regress/expected/hash_index.out | 27 +++++++++
src/test/regress/sql/hash_index.sql | 23 ++++++++
9 files changed, 88 insertions(+), 92 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index e6024a980b..75f68b4e79 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -483,6 +483,7 @@ heapgetpage(TableScanDesc sscan, BlockNumber block)
scan->rs_ntuples = ntup;
}
+
/* ----------------
* heapgettup - fetch next heap tuple
*
@@ -523,6 +524,9 @@ heapgettup(HeapScanDesc scan,
int linesleft;
ItemId lpp;
+ Assert(dir == ForwardScanDirection ||
+ dir == BackwardScanDirection);
+
/*
* calculate next starting lineoff, given scan direction
*/
@@ -583,7 +587,7 @@ heapgettup(HeapScanDesc scan,
linesleft = lines - lineoff + 1;
}
- else if (backward)
+ else
{
/* backward parallel scan not supported */
Assert(scan->rs_base.rs_parallel == NULL);
@@ -653,34 +657,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 +837,9 @@ heapgettup_pagemode(HeapScanDesc scan,
int linesleft;
ItemId lpp;
+ Assert(dir == ForwardScanDirection ||
+ dir == BackwardScanDirection);
+
/*
* calculate next starting lineindex, given scan direction
*/
@@ -918,7 +897,7 @@ heapgettup_pagemode(HeapScanDesc scan,
linesleft = lines - lineindex;
}
- else if (backward)
+ else
{
/* backward parallel scan not supported */
Assert(scan->rs_base.rs_parallel == NULL);
@@ -978,38 +957,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
@@ -1299,6 +1246,9 @@ heap_getnext(TableScanDesc sscan, ScanDirection direction)
{
HeapScanDesc scan = (HeapScanDesc) sscan;
+ Assert(direction == ForwardScanDirection ||
+ direction == BackwardScanDirection);
+
/*
* This is still widely used directly, without going through table AM, so
* add a safety check. It's possible we should, at a later point,
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 35c23bd27d..fbbf28cf06 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -3746,9 +3746,6 @@ ExplainIndexScanDetails(Oid indexid, ScanDirection indexorderdir,
case BackwardScanDirection:
scandir = "Backward";
break;
- case NoMovementScanDirection:
- scandir = "NoMovement";
- break;
case ForwardScanDirection:
scandir = "Forward";
break;
diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c
index 8c7da9ee60..de7c71890a 100644
--- a/src/backend/executor/nodeIndexonlyscan.c
+++ b/src/backend/executor/nodeIndexonlyscan.c
@@ -70,15 +70,7 @@ IndexOnlyNext(IndexOnlyScanState *node)
* extract necessary information from index scan node
*/
estate = node->ss.ps.state;
- direction = estate->es_direction;
- /* flip direction if this is an overall backward scan */
- if (ScanDirectionIsBackward(((IndexOnlyScan *) node->ss.ps.plan)->indexorderdir))
- {
- if (ScanDirectionIsForward(direction))
- direction = BackwardScanDirection;
- else if (ScanDirectionIsBackward(direction))
- direction = ForwardScanDirection;
- }
+ direction = estate->es_direction * ((IndexOnlyScan *) node->ss.ps.plan)->indexorderdir;
scandesc = node->ioss_ScanDesc;
econtext = node->ss.ps.ps_ExprContext;
slot = node->ss.ss_ScanTupleSlot;
@@ -503,6 +495,13 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
indexstate->ss.ps.state = estate;
indexstate->ss.ps.ExecProcNode = ExecIndexOnlyScan;
+ /*
+ * Only forward and backward are valid scan directions. Unordered indexes
+ * will always have ForwardScanDirection.
+ */
+ Assert(node->indexorderdir == ForwardScanDirection ||
+ node->indexorderdir == BackwardScanDirection);
+
/*
* Miscellaneous initialization
*
diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c
index f1ced9ff0f..403ace9c90 100644
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -90,15 +90,7 @@ IndexNext(IndexScanState *node)
* extract necessary information from index scan node
*/
estate = node->ss.ps.state;
- direction = estate->es_direction;
- /* flip direction if this is an overall backward scan */
- if (ScanDirectionIsBackward(((IndexScan *) node->ss.ps.plan)->indexorderdir))
- {
- if (ScanDirectionIsForward(direction))
- direction = BackwardScanDirection;
- else if (ScanDirectionIsBackward(direction))
- direction = ForwardScanDirection;
- }
+ direction = estate->es_direction * ((IndexScan *) node->ss.ps.plan)->indexorderdir;
scandesc = node->iss_ScanDesc;
econtext = node->ss.ps.ps_ExprContext;
slot = node->ss.ss_ScanTupleSlot;
@@ -916,6 +908,13 @@ ExecInitIndexScan(IndexScan *node, EState *estate, int eflags)
indexstate->ss.ps.state = estate;
indexstate->ss.ps.ExecProcNode = ExecIndexScan;
+ /*
+ * Only forward and backward are valid scan directions. Unordered indexes
+ * will always have ForwardScanDirection.
+ */
+ Assert(node->indexorderdir == ForwardScanDirection ||
+ node->indexorderdir == BackwardScanDirection);
+
/*
* Miscellaneous initialization
*
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index e9b784bcab..721a075201 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -1015,9 +1015,7 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel,
orderbyclauses,
orderbyclausecols,
useful_pathkeys,
- index_is_ordered ?
- ForwardScanDirection :
- NoMovementScanDirection,
+ ForwardScanDirection,
index_only_scan,
outer_relids,
loop_count,
@@ -1037,9 +1035,7 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel,
orderbyclauses,
orderbyclausecols,
useful_pathkeys,
- index_is_ordered ?
- ForwardScanDirection :
- NoMovementScanDirection,
+ ForwardScanDirection,
index_only_scan,
outer_relids,
loop_count,
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index f2bf68d33b..07f047cd8d 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -982,9 +982,8 @@ create_samplescan_path(PlannerInfo *root, RelOptInfo *rel, Relids required_outer
* 'indexorderbycols' is an integer list of index column numbers (zero based)
* the ordering operators can be used with.
* 'pathkeys' describes the ordering of the path.
- * 'indexscandir' is ForwardScanDirection or BackwardScanDirection
- * for an ordered index, or NoMovementScanDirection for
- * an unordered index.
+ * 'indexscandir' is either ForwardScanDirection or BackwardScanDirection.
+ * Unordered index types need not support BackwardScanDirection.
* 'indexonly' is true if an index-only scan is wanted.
* 'required_outer' is the set of outer relids for a parameterized path.
* 'loop_count' is the number of repetitions of the indexscan to factor into
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 3fb184717f..28abe819b0 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -1033,6 +1033,9 @@ extern void table_scan_update_snapshot(TableScanDesc scan, Snapshot snapshot);
static inline bool
table_scan_getnextslot(TableScanDesc sscan, ScanDirection direction, TupleTableSlot *slot)
{
+ Assert(direction == ForwardScanDirection ||
+ direction == BackwardScanDirection);
+
slot->tts_tableOid = RelationGetRelid(sscan->rs_rd);
/*
@@ -1096,6 +1099,9 @@ static inline bool
table_scan_getnextslot_tidrange(TableScanDesc sscan, ScanDirection direction,
TupleTableSlot *slot)
{
+ Assert(direction == ForwardScanDirection ||
+ direction == BackwardScanDirection);
+
/* Ensure table_beginscan_tidrange() was used. */
Assert((sscan->rs_flags & SO_TYPE_TIDRANGESCAN) != 0);
diff --git a/src/test/regress/expected/hash_index.out b/src/test/regress/expected/hash_index.out
index a2036a1597..fde395a7b6 100644
--- a/src/test/regress/expected/hash_index.out
+++ b/src/test/regress/expected/hash_index.out
@@ -40,6 +40,33 @@ CREATE INDEX hash_name_index ON hash_name_heap USING hash (random name_ops);
CREATE INDEX hash_txt_index ON hash_txt_heap USING hash (random text_ops);
CREATE INDEX hash_f8_index ON hash_f8_heap USING hash (random float8_ops)
WITH (fillfactor=60);
+CREATE OR REPLACE FUNCTION scan_dir(query text, scan_type text)
+RETURNS TEXT LANGUAGE plpgsql
+AS
+$$
+DECLARE
+ plan json;
+ node json;
+BEGIN
+ FOR plan IN
+ EXECUTE 'EXPLAIN (FORMAT ''json'') ' || query
+ LOOP
+ node := json_extract_path(plan, '0', 'Plan');
+ IF node->>'Node Type' = scan_type THEN
+ RETURN node->>'Scan Direction';
+ END IF;
+ END LOOP;
+ RETURN NULL;
+END;
+$$;
+-- Hash Indexes are unordered and thus only forward scan direction makes sense.
+SELECT 'Forward' = scan_dir(
+ $$ SELECT random FROM hash_i4_heap WHERE random = 1345971420; $$, 'Index Scan');
+ ?column?
+----------
+ t
+(1 row)
+
--
-- Also try building functional, expressional, and partial indexes on
-- tables that already contain data.
diff --git a/src/test/regress/sql/hash_index.sql b/src/test/regress/sql/hash_index.sql
index 527024f710..8a96f53612 100644
--- a/src/test/regress/sql/hash_index.sql
+++ b/src/test/regress/sql/hash_index.sql
@@ -53,6 +53,29 @@ CREATE INDEX hash_txt_index ON hash_txt_heap USING hash (random text_ops);
CREATE INDEX hash_f8_index ON hash_f8_heap USING hash (random float8_ops)
WITH (fillfactor=60);
+CREATE OR REPLACE FUNCTION scan_dir(query text, scan_type text)
+RETURNS TEXT LANGUAGE plpgsql
+AS
+$$
+DECLARE
+ plan json;
+ node json;
+BEGIN
+ FOR plan IN
+ EXECUTE 'EXPLAIN (FORMAT ''json'') ' || query
+ LOOP
+ node := json_extract_path(plan, '0', 'Plan');
+ IF node->>'Node Type' = scan_type THEN
+ RETURN node->>'Scan Direction';
+ END IF;
+ END LOOP;
+ RETURN NULL;
+END;
+$$;
+
+-- Hash Indexes are unordered and thus only forward scan direction makes sense.
+SELECT 'Forward' = scan_dir(
+ $$ SELECT random FROM hash_i4_heap WHERE random = 1345971420; $$, 'Index Scan');
--
-- Also try building functional, expressional, and partial indexes on
-- tables that already contain data.
--
2.37.2
From ed445757b9f4f20ae32afb58e5d207d196c63b54 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Tue, 10 Jan 2023 14:15:15 -0500
Subject: [PATCH v7 5/6] Add heapgettup_advance_block() helper
---
src/backend/access/heap/heapam.c | 183 +++++++++++++++----------------
1 file changed, 88 insertions(+), 95 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 223bb1d5d3..4ff3dc7138 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -622,6 +622,90 @@ heapgettup_continue_page(HeapScanDesc scan, ScanDirection dir, int *linesleft,
return page;
}
+
+/*
+ * heapgettup_advance_block - helper for heapgettup() and heapgettup_pagemode()
+ *
+ * Given the current block number, the scan direction, and various information
+ * contained in the scan descriptor, calculate which block to scan next and
+ * return it.
+ *
+ * This should not be called to determine the initial block number -- only for
+ * subsequent blocks.
+ *
+ * Note that this helper may modify scan->rs_numblocks and any state updated by
+ * table_block_parallelscan_nextpage().
+ *
+ */
+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
*
@@ -654,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;
@@ -763,56 +846,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);
@@ -867,7 +906,6 @@ heapgettup_pagemode(HeapScanDesc scan,
HeapTuple tuple = &(scan->rs_ctup);
bool backward = ScanDirectionIsBackward(dir);
BlockNumber block;
- bool finished;
Page page;
int lineindex;
OffsetNumber lineoff;
@@ -961,57 +999,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.37.2
From c0d7155fb0af55295b3bcd598c083b4690c538dd Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Tue, 10 Jan 2023 14:06:56 -0500
Subject: [PATCH v7 4/6] Add heapgettup() helpers
Add heapgettup_start_page() and heapgettup_continue_page(), helper
functions for heapgettup().
---
src/backend/access/heap/heapam.c | 110 ++++++++++++++++++++++---------
1 file changed, 79 insertions(+), 31 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 4e238d9558..223bb1d5d3 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -545,6 +545,83 @@ heapgettup_initial_block(HeapScanDesc scan, ScanDirection dir)
return scan->rs_nblocks - 1;
}
+
+/*
+ * heapgettup_start_page - helper for heapgettup()
+ *
+ * Given a scan descriptor with a valid buffer referenced in rs_cbuf containing
+ * a block which we have yet to scan, set our lineoff at the tuple we will
+ * start with and our linesleft to the number of tuples in the page. Return the
+ * page associated with that buffer.
+ */
+static inline Page
+heapgettup_start_page(HeapScanDesc scan, 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;
+}
+
+
+/*
+ * heapgettup_continue_page - helper for heapgettup()
+ *
+ * Given a scan descriptor with a valid buffer referenced in rs_cbuf containing
+ * a block from which we have already fetched some tuples, set the lineoff to
+ * the start of the next tuple to fetch and update linesleft. Return the page
+ * associated with that buffer.
+ */
+static inline Page
+heapgettup_continue_page(HeapScanDesc scan, 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;
+ }
+
+ /* lineoff now references the physically previous or next tid */
+ return page;
+}
+
/* ----------------
* heapgettup - fetch next heap tuple
*
@@ -605,43 +682,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, 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, dir, &linesleft, &lineoff);
}
--
2.37.2
From 17edae19944bc55d890c635872849a6156179d9b Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Mon, 9 Jan 2023 18:48:49 -0500
Subject: [PATCH v7 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 | 209 ++++++++++---------------------
src/include/access/heapam.h | 6 +-
2 files changed, 74 insertions(+), 141 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index ff2e64822a..4e238d9558 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -575,12 +575,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;
@@ -588,96 +586,66 @@ heapgettup(HeapScanDesc scan,
Assert(dir == ForwardScanDirection ||
dir == BackwardScanDirection);
- /*
- * calculate next starting lineoff, given scan direction
- */
- if (ScanDirectionIsForward(dir))
+ if (unlikely(!scan->rs_inited))
{
- if (!scan->rs_inited)
- {
- block = heapgettup_initial_block(scan, dir);
+ block = heapgettup_initial_block(scan, dir);
- /*
- * Check if we have reached the end of the scan already. This
- * could happen if the table is empty or if the other workers in a
- * parallel scan have already finished the scan.
- */
- if (block == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
- heapgetpage((TableScanDesc) scan, block);
- lineoff = FirstOffsetNumber;
- }
- else
+ /*
+ * Check if we have reached the end of the scan already. This could
+ * happen if the table is empty or if the other workers in a parallel
+ * scan have already finished the scan.
+ */
+ 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 = PageGetMaxOffsetNumber(page) - FirstOffsetNumber + 1;
- linesleft = lines - lineoff + 1;
+ if (ScanDirectionIsForward(dir))
+ lineoff = FirstOffsetNumber;
+ else
+ lineoff = (OffsetNumber) linesleft;
}
else
{
- if (!scan->rs_inited)
- {
- block = heapgettup_initial_block(scan, dir);
+ block = scan->rs_cblock;
- /*
- * Check if we have reached the end of the scan already. This
- * could happen if the table is empty.
- */
- if (block == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
-
- 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
@@ -706,12 +674,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),
@@ -720,6 +688,7 @@ heapgettup(HeapScanDesc scan,
if (valid)
{
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
+ scan->rs_cindex = lineoff;
return;
}
}
@@ -811,13 +780,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
{
@@ -851,7 +821,6 @@ heapgettup_pagemode(HeapScanDesc scan,
BlockNumber block;
bool finished;
Page page;
- int lines;
int lineindex;
OffsetNumber lineoff;
int linesleft;
@@ -860,82 +829,43 @@ heapgettup_pagemode(HeapScanDesc scan,
Assert(dir == ForwardScanDirection ||
dir == BackwardScanDirection);
- /*
- * calculate next starting lineindex, given scan direction
- */
- if (ScanDirectionIsForward(dir))
+ if (unlikely(!scan->rs_inited))
{
- if (!scan->rs_inited)
- {
- block = heapgettup_initial_block(scan, dir);
+ block = heapgettup_initial_block(scan, dir);
- /*
- * Check if we have reached the end of the scan already. This
- * could happen if the table is empty or if the other workers in a
- * parallel scan have already finished the scan.
- */
- if (block == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
- heapgetpage((TableScanDesc) scan, block);
- lineindex = 0;
- }
- else
+ /*
+ * Check if we have reached the end of the scan already. This could
+ * happen if the table is empty or if the other workers in a parallel
+ * scan have already finished the scan.
+ */
+ 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
{
- if (!scan->rs_inited)
- {
- block = heapgettup_initial_block(scan, dir);
-
- /*
- * Check if we have reached the end of the scan already. This
- * could happen if the table is empty.
- */
- 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
@@ -1048,10 +978,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.37.2
From 0dafb73195cb624c79ec6bf62e00b71117f36b5a Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Tue, 10 Jan 2023 14:30:21 -0500
Subject: [PATCH v7 6/6] Refactor heapgettup() and heapgettup_pagemode()
---
src/backend/access/heap/heapam.c | 235 +++++++++++--------------------
1 file changed, 81 insertions(+), 154 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 4ff3dc7138..e80ea377a2 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;
Assert(dir == ForwardScanDirection ||
dir == BackwardScanDirection);
@@ -755,17 +753,7 @@ heapgettup(HeapScanDesc scan,
* happen if the table is empty or if the other workers in a parallel
* scan have already finished the scan.
*/
- if (block == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
-
- heapgetpage((TableScanDesc) scan, block);
-
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
- page = heapgettup_start_page(scan, dir, &linesleft, &lineoff);
+ Assert(block != InvalidBlockNumber || !BufferIsValid(scan->rs_cbuf));
}
else
{
@@ -773,6 +761,7 @@ heapgettup(HeapScanDesc scan,
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
page = heapgettup_continue_page(scan, dir, &linesleft, &lineoff);
+ goto continue_page;
}
@@ -781,9 +770,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, dir, &linesleft, &lineoff);
+continue_page:
+
/*
* Only continue scanning the page while we have lines left.
*
@@ -791,53 +784,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);
+ bool visible;
+ ItemId lpp = PageGetItemId(page, lineoff);
- /*
- * if current tuple qualifies, return it.
- */
- valid = HeapTupleSatisfiesVisibility(tuple,
- scan->rs_base.rs_snapshot,
- scan->rs_cbuf);
-
- 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;
}
/*
@@ -847,41 +827,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;
}
/* ----------------
@@ -904,13 +859,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;
Assert(dir == ForwardScanDirection ||
dir == BackwardScanDirection);
@@ -924,23 +876,13 @@ heapgettup_pagemode(HeapScanDesc scan,
* happen if the table is empty or if the other workers in a parallel
* scan have already finished the scan.
*/
- 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;
+ 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);
@@ -949,6 +891,9 @@ heapgettup_pagemode(HeapScanDesc scan,
linesleft = scan->rs_ntuples - lineindex;
else
linesleft = scan->rs_cindex;
+ /* lineindex now references the next or previous visible tid */
+
+ goto continue_page;
}
@@ -956,75 +901,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;
+
+ /* lineindex now references 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.37.2
