Обсуждение: 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

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