Re: FETCH FIRST clause PERCENT option
От | Andres Freund |
---|---|
Тема | Re: FETCH FIRST clause PERCENT option |
Дата | |
Msg-id | 20190331213431.qrbh3tbj2bcketep@alap3.anarazel.de обсуждение исходный текст |
Ответ на | Re: FETCH FIRST clause PERCENT option (Surafel Temesgen <surafel3000@gmail.com>) |
Ответы |
Re: FETCH FIRST clause PERCENT option
(Tom Lane <tgl@sss.pgh.pa.us>)
Re: FETCH FIRST clause PERCENT option (Surafel Temesgen <surafel3000@gmail.com>) Re: FETCH FIRST clause PERCENT option (Surafel Temesgen <surafel3000@gmail.com>) |
Список | pgsql-hackers |
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 по дате отправления: