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