Re: FETCH FIRST clause PERCENT option
От | Surafel Temesgen |
---|---|
Тема | Re: FETCH FIRST clause PERCENT option |
Дата | |
Msg-id | CALAY4q-eqSwkv3riFM_HF7THWBr9DuTJqZDkCYBmhF7vNdpqvw@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: FETCH FIRST clause PERCENT option (Andres Freund <andres@anarazel.de>) |
Ответы |
Re: FETCH FIRST clause PERCENT option
|
Список | pgsql-hackers |
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@anarazel.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
Вложения
В списке pgsql-hackers по дате отправления: