>>>>> "Michael" == Michael Paquier <michael@paquier.xyz> writes:
>> I don't think that your solution is correct. From my read of
>> 37a795a6, the tuple descriptor is moved from the query-lifespan
>> memory context (ecxt_per_query_memory) to a function-level context,
>> which could cause the descriptor to become busted as your test is
>> pointing out. Tom?
Michael> Hacking my way out I am finishing with the attached, which
Michael> fixes the problem and passes all regression tests. I am not
Michael> completely sure that this the right approach though, I would
Michael> need to think a bit more about it.
What's happening in the original case is this: the SRF call protocol
says that it's the executor's responsibility to free rsi.setDesc if it's
not refcounted, under the assumption that in such a case it's in
per-query memory (and not in either a shorter-lived or longer-lived
context).
The change to update_cached_tupdesc broke the protocol by causing
populate_recordset_worker to set rsi.setDesc to a non-refcounted tupdesc
allocated in a context that's not the per-query context.
What's not clear here is why populate_recordset_record thinks it needs
to update the tupdesc for every record in a single result set. The
entire result set will ultimately be treated as having the same tupdesc
regardless, so any per-record changes will break things later anyway.
Your latest proposed fix isn't OK because it puts the wrong context in
cache->fn_mcxt - data that's dangled off flinfo->fn_extra must NOT be in
any context that has a different lifetime than flinfo->fn_mcxt (which
might not be query lifetime), unless you're taking specific steps to
free or invalidate it at the correct point.
My first approach - assuming that update_cached_tupdesc has good reason
to make a copy, which I'm not convinced is the case - would be to simply
make a per-query-context copy of the tupdesc to assign to rsi.setDesc in
order to conform to the call protocol.
--
Andrew (irc:RhodiumToad)
