On Thu, 1 Dec 2022 at 21:18, Richard Guo <guofengli...@gmail.com> wrote:
>> + if (!func_strict(opexpr->opfuncid))
>> + return false;
>>
>> Should return true instead?
>
>
> Yeah, you're right. This should be a thinko.
Yeah, oops. That's wrong.
I've adjusted that in the attached.
I'm keen to move along and push the fix for this bug. If there are no
objections to the method in the attached and also adding the
restriction to limit the optimization to only working with strict
OpExprs, then I'm going to push this, likely about 24 hours from now.
David
diff --git a/src/backend/executor/nodeWindowAgg.c
b/src/backend/executor/nodeWindowAgg.c
index 81ba024bba..110c594edd 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -2300,7 +2300,27 @@ ExecWindowAgg(PlanState *pstate)
continue;
}
else
+ {
winstate->status =
WINDOWAGG_PASSTHROUGH;
+
+ /*
+ * Otherwise, if we're not the
top-window, we'd better
+ * NULLify the aggregate
results, else, by-ref result
+ * types will point to freed
memory. We can get away
+ * without storing the final
result of the WindowFunc
+ * because in the planner we
insisted that the
+ * runcondition is strict.
Setting these to NULL will
+ * ensure the top-level
WindowAgg filter will always
+ * filter out the rows produced
in this WindowAgg when
+ * not in WINDOWAGG_RUN state.
+ */
+ numfuncs = winstate->numfuncs;
+ for (i = 0; i < numfuncs; i++)
+ {
+
econtext->ecxt_aggvalues[i] = (Datum) 0;
+
econtext->ecxt_aggnulls[i] = true;
+ }
+ }
}
else
{
diff --git a/src/backend/optimizer/path/allpaths.c
b/src/backend/optimizer/path/allpaths.c
index 03ee6dc832..595870ea30 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -2399,6 +2399,24 @@ check_and_push_window_quals(Query *subquery,
RangeTblEntry *rte, Index rti,
if (list_length(opexpr->args) != 2)
return true;
+ /*
+ * Currently, we restrict this optimization to strict OpExprs. The
reason
+ * for this is that during execution, once the runcondition becomes
false,
+ * we stop evaluating WindowFuncs in WindowAgg nodes and since we're no
+ * longer updating them, we NULLify the aggregate and window aggregate
+ * results to prevent by-ref result typed window aggregates from
pointing
+ * to free'd memory for byref WindowFuncs and storing stale values for
+ * byval WindowFuncs (remember that window functions such as
row_number()
+ * return a byref type on non-FLOAT8PASSBYVAL builds). Upper-level
+ * WindowAgg nodes may make reference to these results in their filter
+ * clause and we can ensure the filter remain false by making sure the
+ * operator is strict and by performing the NULLification in the
executor
+ * when the runcondition first becomes false.
+ */
+ set_opfuncid(opexpr);
+ if (!func_strict(opexpr->opfuncid))
+ return true;
+
/*
* Check for plain Vars that reference window functions in the subquery.
* If we find any, we'll ask find_window_run_conditions() if 'opexpr'
can
