agora inbox for [email protected]
help / color / mirror / Atom feedFrom: Tomas Vondra <[email protected]>
Subject: [PATCH 2/2] review
Date: Sun, 28 Oct 2018 18:38:47 +0100
---
doc/src/sgml/ref/select.sgml | 6 ++--
src/backend/executor/nodeLimit.c | 52 +++++++++++++++++++--------------
src/backend/optimizer/plan/createplan.c | 25 +++++++---------
src/include/nodes/execnodes.h | 4 +--
src/include/nodes/parsenodes.h | 4 +--
src/include/nodes/plannodes.h | 2 +-
src/include/nodes/relation.h | 2 +-
7 files changed, 49 insertions(+), 46 deletions(-)
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index c81ac04108..b649b1ca7a 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -1408,9 +1408,9 @@ FETCH { FIRST | NEXT } [ <replaceable class="parameter">count</replaceable> ] {
If <replaceable class="parameter">count</replaceable> is
omitted in a <literal>FETCH</literal> clause, it defaults to 1.
<literal>ROW</literal>
- <literal>WITH TIES</literal> option is Used to return two or more rows
- that tie for last place in the limit results set according to <literal>ORDER BY</literal>
- clause (<literal>ORDER BY</literal> clause must be specified in this case).
+ <literal>WITH TIES</literal> option is used to return two or more rows
+ that tie for last place in the limit results set according to <literal>ORDER BY</literal>
+ clause (<literal>ORDER BY</literal> clause must be specified in this case).
and <literal>ROWS</literal> as well as <literal>FIRST</literal>
and <literal>NEXT</literal> are noise words that don't influence
the effects of these clauses.
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index 93f8813972..724a8e09c1 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -132,7 +132,8 @@ ExecLimit(PlanState *pstate)
* the state machine state to record having done so.
*/
if (!node->noCount &&
- node->position - node->offset >= node->count && node->limitOption == WITH_ONLY)
+ node->position - node->offset >= node->count &&
+ node->limitOption == WITH_ONLY)
{
node->lstate = LIMIT_WINDOWEND;
@@ -145,23 +146,24 @@ ExecLimit(PlanState *pstate)
return NULL;
}
-
else if (!node->noCount &&
- node->position - node->offset >= node->count && node->limitOption == WITH_TIES)
+ node->position - node->offset >= node->count &&
+ node->limitOption == WITH_TIES)
{
/*
- * Get next tuple from subplan, if any.
- */
+ * Get next tuple from subplan, if any.
+ */
slot = ExecProcNode(outerPlan);
if (TupIsNull(slot))
{
node->lstate = LIMIT_SUBPLANEOF;
return NULL;
}
+
/*
- * Test if the new tuple and the last tuple match.
- * If so we return the tuple.
- */
+ * Test if the new tuple and the last tuple match.
+ * If so we return the tuple.
+ */
econtext->ecxt_innertuple = slot;
econtext->ecxt_outertuple = node->last_slot;
if (ExecQualAndReset(node->eqfunction, econtext))
@@ -184,22 +186,25 @@ ExecLimit(PlanState *pstate)
return NULL;
}
- }else
- /*
- * Get next tuple from subplan, if any.
- */
- slot = ExecProcNode(outerPlan);
- if (TupIsNull(slot))
- {
- node->lstate = LIMIT_SUBPLANEOF;
- return NULL;
}
- if (node->limitOption == WITH_TIES)
+ else /* XXX So what exactly should be part of this else branch? */
{
- ExecCopySlot(node->last_slot, slot);
+ /*
+ * Get next tuple from subplan, if any.
+ */
+ slot = ExecProcNode(outerPlan);
+ if (TupIsNull(slot))
+ {
+ node->lstate = LIMIT_SUBPLANEOF;
+ return NULL;
+ }
+ if (node->limitOption == WITH_TIES)
+ {
+ ExecCopySlot(node->last_slot, slot);
+ }
+ node->subSlot = slot;
+ node->position++;
}
- node->subSlot = slot;
- node->position++;
}
else
{
@@ -356,7 +361,7 @@ recompute_limits(LimitState *node)
* previous time we got a different result.
*/
if(node->limitOption == WITH_ONLY)
- ExecSetTupleBound(compute_tuples_needed(node), outerPlanState(node));
+ ExecSetTupleBound(compute_tuples_needed(node), outerPlanState(node));
}
/*
@@ -433,6 +438,9 @@ ExecInitLimit(Limit *node, EState *estate, int eflags)
*/
limitstate->ps.ps_ProjInfo = NULL;
+ /*
+ * Initialize the equality evaluation, to detect ties.
+ */
if (node->limitOption == WITH_TIES)
{
TupleDesc scanDesc;
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 7e540849b4..62c05c7feb 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -2435,6 +2435,9 @@ create_limit_plan(PlannerInfo *root, LimitPath *best_path, int flags)
{
Limit *plan;
Plan *subplan;
+ int numsortkeys = 0;
+ AttrNumber *sortColIdx = NULL;
+ Oid *sortOperators = NULL;
/* Limit doesn't project, so tlist requirements pass through */
subplan = create_plan_recurse(root, best_path->subpath, flags);
@@ -2442,9 +2445,6 @@ create_limit_plan(PlannerInfo *root, LimitPath *best_path, int flags)
if (best_path->limitOption == WITH_TIES)
{
Query *parse = root->parse;
- int numsortkeys;
- AttrNumber *sortColIdx;
- Oid *sortOperators;
ListCell *l;
numsortkeys = list_length(parse->sortClause);
@@ -2461,19 +2461,14 @@ create_limit_plan(PlannerInfo *root, LimitPath *best_path, int flags)
sortOperators[numsortkeys] = sortcl->eqop;
numsortkeys++;
}
- plan = make_limit(subplan,
- best_path->limitOffset,
- best_path->limitCount,
- best_path->limitOption,
- numsortkeys, sortColIdx,
- sortOperators);
}
- else
- plan = make_limit(subplan,
- best_path->limitOffset,
- best_path->limitCount,
- best_path->limitOption,
- 0, NULL, NULL);
+
+ plan = make_limit(subplan,
+ best_path->limitOffset,
+ best_path->limitCount,
+ best_path->limitOption,
+ numsortkeys, sortColIdx,
+ sortOperators);
copy_generic_path_info(&plan->plan, (Path *) best_path);
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 212820d886..6f15ce8ea4 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -2242,7 +2242,7 @@ typedef struct LimitState
PlanState ps; /* its first field is NodeTag */
ExprState *limitOffset; /* OFFSET parameter, or NULL if none */
ExprState *limitCount; /* COUNT parameter, or NULL if none */
- LimitOption limitOption; /* limit specification type */
+ LimitOption limitOption; /* limit specification type */
int64 offset; /* current OFFSET value */
int64 count; /* current COUNT, if any */
bool noCount; /* if true, ignore count */
@@ -2250,7 +2250,7 @@ typedef struct LimitState
int64 position; /* 1-based index of last tuple returned */
TupleTableSlot *subSlot; /* tuple last obtained from subplan */
ExprState *eqfunction; /* tuple equality qual in case of WITH TIES OPTION */
- TupleTableSlot *last_slot;
+ TupleTableSlot *last_slot; /* slot for evaluation of ties */
} LimitState;
#endif /* EXECNODES_H */
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index ce10b55dcf..ce07a1a02d 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -159,7 +159,7 @@ typedef struct Query
Node *limitOffset; /* # of result tuples to skip (int8 expr) */
Node *limitCount; /* # of result tuples to return (int8 expr) */
- LimitOption limitOption; /* limit type */
+ LimitOption limitOption; /* LIMIT type [WITH TIES | WITH ONLY] */
List *rowMarks; /* a list of RowMarkClause's */
@@ -1574,7 +1574,7 @@ typedef struct SelectStmt
List *sortClause; /* sort clause (a list of SortBy's) */
Node *limitOffset; /* # of result tuples to skip */
Node *limitCount; /* # of result tuples to return */
- LimitOption limitOption; /* limit type */
+ LimitOption limitOption; /* limit type */
List *lockingClause; /* FOR UPDATE (list of LockingClause's) */
WithClause *withClause; /* WITH clause */
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 24b7e51a91..5e961a8f7b 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -945,7 +945,7 @@ typedef struct Limit
Plan plan;
Node *limitOffset; /* OFFSET parameter, or NULL if none */
Node *limitCount; /* COUNT parameter, or NULL if none */
- LimitOption limitOption; /* LIMIT with ties or exact number */
+ LimitOption limitOption; /* LIMIT with ties or exact number */
int numCols; /* number of columns to check for Similarity */
AttrNumber *uniqColIdx; /* their indexes in the target list */
Oid *uniqOperators; /* equality operators to compare with */
diff --git a/src/include/nodes/relation.h b/src/include/nodes/relation.h
index 2c7f4c71ea..f107d4f773 100644
--- a/src/include/nodes/relation.h
+++ b/src/include/nodes/relation.h
@@ -1737,7 +1737,7 @@ typedef struct LimitPath
Path *subpath; /* path representing input source */
Node *limitOffset; /* OFFSET parameter, or NULL if none */
Node *limitCount; /* COUNT parameter, or NULL if none */
- LimitOption limitOption; /* LIMIT with ties or exact number */
+ LimitOption limitOption; /* LIMIT with ties or exact number */
} LimitPath;
--
2.13.6
--------------08FEC5268EA8E45E5EE90A4F
Content-Type: text/x-patch;
name="0001-rebased-patch.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="0001-rebased-patch.patch"
view thread (10+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected]
Subject: Re: [PATCH 2/2] review
In-Reply-To: <no-message-id-228949@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox