Hi,
On 2019-03-29 12:04:50 +0300, Surafel Temesgen wrote:
> + if (node->limitOption == PERCENTAGE)
> + {
> + while (node->position - node->offset <
> node->count)
> + {
> + slot =
> MakeSingleTupleTableSlot(tupleDescriptor, &TTSOpsMinimalTuple);
> + if
> (tuplestore_gettupleslot(node->tupleStore, true, true, slot))
There's several blocks like this - how's this not going to be a resource
leak? As far as I can tell you're just going to create new slots, and
their previous contents aren't going to be cleared? I think there very
rarely are cases where an executor node's *Next function (or its
subsidiaries) creates slots. Normally you ought to create them *once* on
the *Init function.
You might not directly leak memory, because this is probably running in
a short lived context, but you'll leak tuple desc references etc. (Or if
it were a heap buffer slot, buffer pins).
> + /* In PERCENTAGE case the result is already in
> tuplestore */
> + if (node->limitOption == PERCENTAGE)
> + {
> + slot =
> MakeSingleTupleTableSlot(tupleDescriptor, &TTSOpsMinimalTuple);
> + if (tuplestore_gettupleslot(node->tupleStore,
> false, false, slot))
> + {
> + node->subSlot = slot;
> + node->lstate = LIMIT_INWINDOW;
> + }
> + else
> + elog(ERROR, "LIMIT subplan failed to
> run backwards");
> + }
> + else if (node->limitOption == EXACT_NUMBER)
> + {
> /*
> * Backing up from subplan EOF, so re-fetch previous
> tuple; there
> * should be one! Note previous tuple must be in
> window.
> @@ -194,6 +329,7 @@ ExecLimit(PlanState *pstate)
> node->subSlot = slot;
> node->lstate = LIMIT_INWINDOW;
> /* position does not change 'cause we didn't advance it
> before */
> + }
The indentation here looks wrong...
> break;
>
> case LIMIT_WINDOWEND:
> @@ -278,17 +414,29 @@ recompute_limits(LimitState *node)
> /* Interpret NULL count as no count (LIMIT ALL) */
> if (isNull)
> {
> - node->count = 0;
> + node->count = 1;
> node->noCount = true;
Huh?
> }
> else
> {
> - node->count = DatumGetInt64(val);
> - if (node->count < 0)
> - ereport(ERROR,
> -
> (errcode(ERRCODE_INVALID_ROW_COUNT_IN_LIMIT_CLAUSE),
> - errmsg("LIMIT must not be
> negative")));
> - node->noCount = false;
> + if (node->limitOption == PERCENTAGE)
> + {
> + /*
> + * We expect to return at least one row (unless
> there
> + * are no rows in the subplan), and we'll
> update this
> + * count later as we go.
> + */
> + node->count = 0;
> + node->percent = DatumGetFloat8(val);
> + }
> + else
> + {
> + node->count = DatumGetInt64(val);
> + if (node->count < 0)
> + ereport(ERROR,
> +
> (errcode(ERRCODE_INVALID_ROW_COUNT_IN_LIMIT_CLAUSE),
> + errmsg("LIMIT must not
> be negative")));
> + }
Shouldn't we error out with a negative count, regardless of PERCENT? Or
is that prevented elsewhere?
> }
> }
> else
> @@ -299,8 +447,10 @@ recompute_limits(LimitState *node)
> }
>
> /* Reset position to start-of-scan */
> - node->position = 0;
> + node->position = 0;;
unnecessary
> if (parse->sortClause)
> {
> - current_rel = create_ordered_paths(root,
> -
> current_rel,
> -
> final_target,
> -
> final_target_parallel_safe,
> -
> have_postponed_srfs ? -1.0 :
> -
> limit_tuples);
> +
> + /* In PERCENTAGE option there are no bound on the number of
> output tuples */
> + if (parse->limitOption == PERCENTAGE)
> + current_rel = create_ordered_paths(root,
> +
> current_rel,
> +
> final_target,
> +
> final_target_parallel_safe,
> +
> have_postponed_srfs ? -1.0 :
> +
> -1.0);
> + else
> + current_rel = create_ordered_paths(root,
> +
> current_rel,
> +
> final_target,
> +
> final_target_parallel_safe,
> +
> have_postponed_srfs ? -1.0 :
> +
> limit_tuples);
I'd rather not duplicate those two calls, and just have the limit_tuples
computation inside an if.
> offset_clause:
> @@ -15435,6 +15442,7 @@ reserved_keyword:
> | ONLY
> | OR
> | ORDER
> + | PERCENT
> | PLACING
> | PRIMARY
> | REFERENCES
Are we really ok with adding a new reserved keyword 'PERCENT' for this?
That seems awfully likely to already exist in columns etc. Is there a
chance we can use a less strong category?
Greetings,
Andres Freund
