>>>>> "Michael" == Michael Paquier <mich...@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)
