From: | Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Subject: | Re: Problem with tupdesc in jsonb_to_recordset |
Date: | 2018-07-11 10:38:46 |
Message-ID: | 87fu0qkrfr.fsf@news-spur.riddles.org.uk |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-bugs pgsql-hackers |
>>>>> "Michael" == Michael Paquier <michael(at)paquier(dot)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)
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2018-07-11 11:30:48 | Re: Problem with tupdesc in jsonb_to_recordset |
Previous Message | Dmitry Dolgov | 2018-07-11 09:33:14 | Re: Problem with tupdesc in jsonb_to_recordset |
From | Date | Subject | |
---|---|---|---|
Next Message | Dilip Kumar | 2018-07-11 10:50:37 | Re: partition pruning doesn't work with IS NULL clause in multikey range partition case |
Previous Message | Bruce Momjian | 2018-07-11 10:31:45 | Re: Negotiating the SCRAM channel binding type |