On Tue, 27 Sept 2022 at 11:28, David Rowley <dgrowleyml@gmail.com> wrote:
> Maybe we could remove the datumCopy() from eval_windowfunction() and
> also document that a window function when returning a non-byval type,
> must allocate the Datum in either ps_ExprContext's
> ecxt_per_tuple_memory or ecxt_per_query_memory. We could ensure any
> extension which has its own window functions get the memo about the
> API change by adding an Assert to ensure that the return value (for
> byref types) is in the current context by calling the
> loop-over-the-blocks version of MemoryContextContains().
I did some work on this and it turned out that the value returned by
any of lead(), lag(), first_value(), last_value() and nth_value()
could also be in MessageContext or some child context to
CacheMemoryContext. The reason for the latter two is that cases like
LAG(col, 1, 'default value') will return the Const in the 3rd arg when
the offset value is outside of the window frame. That means
MessageContext for normal queries and it means it'll be cached in a
child context of CacheMemoryContext for PREPAREd queries.
This means the Assert that I wanted to add to eval_windowfunction
became quite complex. Namely:
Assert(perfuncstate->resulttypeByVal || fcinfo->isnull ||
MemoryContextContains(winstate->ss.ps.ps_ExprContext->ecxt_per_tuple_memory,
(void *) *result) ||
MemoryContextContains(winstate->ss.ps.ps_ExprContext->ecxt_per_query_memory,
(void *) *result) ||
MemoryContextContains(MessageContext, (void *) *result) ||
MemoryContextOrChildOfContains(CacheMemoryContext, (void *) *result));
Notice the invention of MemoryContextOrChildOfContains() to
recursively search the CacheMemoryContext children. It does not seem
so great as CacheMemoryContext tends to have many children and
searching through them all could make that Assert a bit slow.
I think I am fairly happy that all the 4 message contexts I mentioned
in the Assert will be around long enough for the result value to not
be freed. It's just that the whole thing feels a bit wrong and that
the context the return value is in should be a bit more predictable.
Does anyone have any opinions on this?
David
