v2 attached
On Fri, Jan 27, 2023 at 6:28 PM David Rowley <dgrowle...@gmail.com> wrote:
>
> On Sat, 28 Jan 2023 at 12:15, Tom Lane <t...@sss.pgh.pa.us> wrote:
> > /*
> > * Determine the net effect of two direction specifications.
> > * This relies on having ForwardScanDirection = +1, BackwardScanDirection =
> > -1,
> > * and will probably not do what you want if applied to any other values.
> > */
> > #define CombineScanDirections(a, b) ((a) * (b))
> >
> > The main thing this'd buy us is being able to grep for uses of the
> > trick. If it's written as just multiplication, good luck being
> > able to find what's depending on that, should you ever need to.
>
> Yeah, I think the multiplication macro is a good way of doing it.
> Having the definition of it close to the ScanDirection enum's
> definition is likely a very good idea so that anyone adjusting the
> enum values is more likely to notice that it'll cause an issue. A
> small note on the enum declaration about the -1, +1 values being
> exploited in various places might be a good idea too. I see v6-0006 in
> [1] further exploits this, so that's further reason to document that.
>
> My personal preference would have been to call it
> ScanDirectionCombine, so the naming is more aligned to the 4 other
> macro names that start with ScanDirection in sdir.h, but I'm not going
> to fuss over it.
I've gone with this macro name.
I've also updated comments Tom mentioned and removed the test.
As for the asserts, I was at a bit of a loss as to where to put an
assert which will make it clear that heapgettup() and
heapgettup_pagemode() do not handle NoMovementScanDirection but was
at a higher level of the executor. Do we not have to accommodate the
direction changing from tuple to tuple? If we don't expect the plan node
direction to change during execution, then why recalculate
estate->es_direction for each invocation of Index/SeqNext()?
As such, in this version I've put the asserts in heapgettup() and
heapgettup_pagemode().
I also realized that it doesn't really make sense to assert about the
index scan direction in ExecInitIndexOnlyScan() and ExecInitIndexScan()
-- so I've moved the assertion to planner when we make the index plan
from the path. I'm not sure if it is needed.
- Melanie
From d6aae3a73864aaaf84b4dc00ff299c2e8b4a5729 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Mon, 30 Jan 2023 09:49:54 -0500
Subject: [PATCH v2] remove nomovement scandir
---
src/backend/access/heap/heapam.c | 71 ++----------------------
src/backend/commands/explain.c | 3 -
src/backend/executor/nodeIndexonlyscan.c | 11 +---
src/backend/executor/nodeIndexscan.c | 11 +---
src/backend/optimizer/path/indxpath.c | 8 +--
src/backend/optimizer/plan/createplan.c | 14 +++++
src/backend/optimizer/util/pathnode.c | 5 +-
src/include/access/sdir.h | 11 +++-
src/include/nodes/pathnodes.h | 6 +-
9 files changed, 41 insertions(+), 99 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index e6024a980b..10aaeb14aa 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -490,9 +490,6 @@ heapgetpage(TableScanDesc sscan, BlockNumber block)
* tuple as indicated by "dir"; return the next tuple in scan->rs_ctup,
* or set scan->rs_ctup.t_data = NULL if no more tuples.
*
- * dir == NoMovementScanDirection means "re-fetch the tuple indicated
- * by scan->rs_ctup".
- *
* Note: the reason nkeys/key are passed separately, even though they are
* kept in the scan descriptor, is that the caller may not want us to check
* the scankeys.
@@ -523,6 +520,8 @@ heapgettup(HeapScanDesc scan,
int linesleft;
ItemId lpp;
+ Assert(dir != NoMovementScanDirection);
+
/*
* calculate next starting lineoff, given scan direction
*/
@@ -583,7 +582,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 +652,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 +832,8 @@ heapgettup_pagemode(HeapScanDesc scan,
int linesleft;
ItemId lpp;
+ Assert(dir != NoMovementScanDirection);
+
/*
* calculate next starting lineindex, given scan direction
*/
@@ -918,7 +891,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 +951,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
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index a0311ce9dc..d38311fa7e 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..e75b3bb8e1 100644
--- a/src/backend/executor/nodeIndexonlyscan.c
+++ b/src/backend/executor/nodeIndexonlyscan.c
@@ -70,15 +70,10 @@ 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 = ScanDirectionCombine(estate->es_direction,
+ ((IndexOnlyScan *) node->ss.ps.plan)->indexorderdir);
scandesc = node->ioss_ScanDesc;
econtext = node->ss.ps.ps_ExprContext;
slot = node->ss.ss_ScanTupleSlot;
diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c
index f1ced9ff0f..1c1c558b22 100644
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -90,15 +90,10 @@ 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 = ScanDirectionCombine(estate->es_direction,
+ ((IndexScan *) node->ss.ps.plan)->indexorderdir);
scandesc = node->iss_ScanDesc;
econtext = node->ss.ps.ps_ExprContext;
slot = node->ss.ss_ScanTupleSlot;
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index e13c8f1914..751fa2edfe 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/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index cd68942af0..1302d25e54 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -5512,6 +5512,13 @@ make_indexscan(List *qptlist,
IndexScan *node = makeNode(IndexScan);
Plan *plan = &node->scan.plan;
+ /*
+ * Only forward and backward index scan directions are meaningful to the
+ * executor. Unordered indexes will always have ForwardScanDirection.
+ */
+ Assert(indexscandir == ForwardScanDirection ||
+ indexscandir == BackwardScanDirection);
+
plan->targetlist = qptlist;
plan->qual = qpqual;
plan->lefttree = NULL;
@@ -5542,6 +5549,13 @@ make_indexonlyscan(List *qptlist,
IndexOnlyScan *node = makeNode(IndexOnlyScan);
Plan *plan = &node->scan.plan;
+ /*
+ * Only forward and backward index scan directions are meaningful to the
+ * executor. Unordered indexes will always have ForwardScanDirection.
+ */
+ Assert(indexscandir == ForwardScanDirection ||
+ indexscandir == BackwardScanDirection);
+
plan->targetlist = qptlist;
plan->qual = qpqual;
plan->lefttree = NULL;
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 4478036bb6..dd5236682f 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/sdir.h b/src/include/access/sdir.h
index 16cb06c709..bd79d977ec 100644
--- a/src/include/access/sdir.h
+++ b/src/include/access/sdir.h
@@ -16,8 +16,8 @@
/*
- * ScanDirection was an int8 for no apparent reason. I kept the original
- * values because I'm not sure if I'll break anything otherwise. -ay 2/95
+ * These enum values were originally int8 values. Using -1, 0, and 1 as their
+ * values conveniently mirrors their semantic value when used during execution.
*/
typedef enum ScanDirection
{
@@ -26,6 +26,13 @@ typedef enum ScanDirection
ForwardScanDirection = 1
} ScanDirection;
+/*
+ * Determine the net effect of two direction specifications.
+ * This relies on having ForwardScanDirection = +1, BackwardScanDirection = -1,
+ * and will probably not do what you want if applied to any other values.
+ */
+#define ScanDirectionCombine(a, b) ((a) * (b))
+
/*
* ScanDirectionIsValid
* True iff scan direction is valid.
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 2d1d8f4bcd..cdb2a06409 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -1611,10 +1611,8 @@ typedef struct Path
* 'indexscandir' is one of:
* ForwardScanDirection: forward scan of an ordered index
* BackwardScanDirection: backward scan of an ordered index
- * NoMovementScanDirection: scan of an unordered index, or don't care
- * (The executor doesn't care whether it gets ForwardScanDirection or
- * NoMovementScanDirection for an indexscan, but the planner wants to
- * distinguish ordered from unordered indexes for building pathkeys.)
+ * NoMovementScanDirection for indexscandir is meaningless for the executor, so
+ * unordered indexes will always set ForwardScanDirection.
*
* 'indextotalcost' and 'indexselectivity' are saved in the IndexPath so that
* we need not recompute them when considering using the same index in a
--
2.37.2
