Re: [HACKERS] Management of simple_eval_estate for plpgsql DO blocks
On 14 August 2015 at 17:42, Tom Lane t...@sss.pgh.pa.us wrote: The simplest fix for this would be to give up on the idea that DO blocks use private simple_eval_estates, and make them use the shared one. However, that would result in intra-transaction memory bloat for transactions executing large numbers of DO blocks; see commit c7b849a89, which installed that arrangement to begin with. Since that change was based on a user complaint, this answer doesn't seem appetizing. ... Or we could change things so that DO blocks use private cast_hash hashtables along with their private simple_eval_estates. This would give up some efficiency (since a DO block would then always need to do its own cast lookups) but it would be a simple and reliable fix. I'm kind of inclined to go with the last choice, but I wonder if anyone wants to argue differently, or sees another feasible solution. Not everyone uses large numbers of DO blocks, but casts apply everywhere, right? If the choice is efficiency or memory bloat, can't we choose a point where we switch from one to the other? i.e. use private structures after N uses of the shared structures. -- Simon Riggshttp://www.2ndQuadrant.com/ http://www.2ndquadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
[HACKERS] Management of simple_eval_estate for plpgsql DO blocks
In commit 0fc94a5ba I wrote: +* ... It's okay to update the [ session-wide ] +* hash table with the new tree because all plpgsql functions within a +* given transaction share the same simple_eval_estate. Um. Well, that's true for actual functions, but plpgsql DO blocks use their own private simple_eval_estate. That means that after a DO block runs, the cast_hash contains dangling pointers to expression eval state trees, which a subsequent plpgsql execution in the same transaction will think are still valid. Ooops. (See bug #13571.) The simplest fix for this would be to give up on the idea that DO blocks use private simple_eval_estates, and make them use the shared one. However, that would result in intra-transaction memory bloat for transactions executing large numbers of DO blocks; see commit c7b849a89, which installed that arrangement to begin with. Since that change was based on a user complaint, this answer doesn't seem appetizing. Or we could try to use the shared simple_eval_estate for CAST expressions even within DO blocks, but I'm afraid that would break things in subtle ways. We need to do actual execution in the block's own eval_estate, or we will have problems with leakage of pass-by-reference cast results because exec_eval_cleanup() won't know to clean them up. It's possible that we could get away with putting the expression state tree into the shared simple_eval_estate's per-query memory and then executing it with the block's private simple_eval_estate, but I'm afraid there are probably places in execQual and/or C functions that suppose that the expression state tree is in the estate's per-query memory. (That is, I doubt that we're totally consistent about whether we use fcinfo-flinfo-fn_mcxt or econtext-ecxt_per_query_memory for long-lived data, in which case an arrangement like this could lead to dangling pointers.) Or we could change things so that DO blocks use private cast_hash hashtables along with their private simple_eval_estates. This would give up some efficiency (since a DO block would then always need to do its own cast lookups) but it would be a simple and reliable fix. I'm kind of inclined to go with the last choice, but I wonder if anyone wants to argue differently, or sees another feasible solution. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Management of simple_eval_estate for plpgsql DO blocks
On 08/14/2015 12:42 PM, Tom Lane wrote: In commit 0fc94a5ba I wrote: +* ... It's okay to update the [ session-wide ] +* hash table with the new tree because all plpgsql functions within a +* given transaction share the same simple_eval_estate. Um. Well, that's true for actual functions, but plpgsql DO blocks use their own private simple_eval_estate. That means that after a DO block runs, the cast_hash contains dangling pointers to expression eval state trees, which a subsequent plpgsql execution in the same transaction will think are still valid. Ooops. (See bug #13571.) The simplest fix for this would be to give up on the idea that DO blocks use private simple_eval_estates, and make them use the shared one. However, that would result in intra-transaction memory bloat for transactions executing large numbers of DO blocks; see commit c7b849a89, which installed that arrangement to begin with. Since that change was based on a user complaint, this answer doesn't seem appetizing. Or we could try to use the shared simple_eval_estate for CAST expressions even within DO blocks, but I'm afraid that would break things in subtle ways. We need to do actual execution in the block's own eval_estate, or we will have problems with leakage of pass-by-reference cast results because exec_eval_cleanup() won't know to clean them up. It's possible that we could get away with putting the expression state tree into the shared simple_eval_estate's per-query memory and then executing it with the block's private simple_eval_estate, but I'm afraid there are probably places in execQual and/or C functions that suppose that the expression state tree is in the estate's per-query memory. (That is, I doubt that we're totally consistent about whether we use fcinfo-flinfo-fn_mcxt or econtext-ecxt_per_query_memory for long-lived data, in which case an arrangement like this could lead to dangling pointers.) Or we could change things so that DO blocks use private cast_hash hashtables along with their private simple_eval_estates. This would give up some efficiency (since a DO block would then always need to do its own cast lookups) but it would be a simple and reliable fix. I'm kind of inclined to go with the last choice, but I wonder if anyone wants to argue differently, or sees another feasible solution. +1 for the last unless someone comes up with something better. Is it going to involve a huge hit anyway? It seems unlikely to matter that much. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
