Re: FETCH FIRST clause PERCENT option

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?