Re: FETCH FIRST clause PERCENT option

Hello,
The attached patch include the fix for all the comment given
regards
Surafel

On Mon, Apr 1, 2019 at 12:34 AM Andres Freund <andres(at)anarazel(dot)de> wrote:

> 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
>