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?

Greetings,

Andres Freund