Re: some last patches breaks plan cache

On 03/31/2018 08:28 PM, Tomas Vondra wrote:
>
>
> On 03/31/2018 07:56 PM, Tomas Vondra wrote:
>> On 03/31/2018 07:38 PM, Pavel Stehule wrote:
>>> Hi
>>>
>>> CREATE OR REPLACE PROCEDURE public.proc(a integer, INOUT b integer, c
>>> integer)
>>>  LANGUAGE plpgsql
>>> AS $procedure$
>>> begin
>>>   b := a + c;
>>> end;
>>> $procedure$
>>>
>>> CREATE OR REPLACE PROCEDURE public.testproc()
>>>  LANGUAGE plpgsql
>>> AS $procedure$
>>> declare r int;
>>> begin
>>>   call proc(10, r, 20);
>>> end;
>>> $procedure$
>>>
>>> postgres=# call testproc();
>>> CALL
>>> postgres=# call testproc();
>>> ERROR:  SPI_execute_plan_with_paramlist failed executing query "CALL
>>> proc(10, r, 20)": SPI_ERROR_ARGUMENT
>>> CONTEXT:  PL/pgSQL function testproc() line 4 at CALL
>>> postgres=#
>>>
>>> second call fails
>>
>> Yeah.
>>
>> d92bc83c48bdea9888e64cf1e2edbac9693099c9 seems to have broken this :-/
>>
>
> FWIW it seems the issue is somewhere in exec_stmt_call, which does this:
>
>     /*
>      * Don't save the plan if not in atomic context.  Otherwise,
>      * transaction ends would cause warnings about plan leaks.
>      */
>     exec_prepare_plan(estate, expr, 0, estate->atomic);
>
> When executed outside transaction, CALL has estate->atomic=false, and so
> calls exec_prepare_plan() with keepplan=false. And on the second call it
> gets bogus Plan, of course (with the usual 0x7f7f7f7f7f7f7f7f patterns).
>
> When in a transaction, it sets keepplan=true, and everything works fine.
>
> So either estate->atomic is not sufficient on it's own, or we need to
> reset the expr->plan somewhere.
>

The attached patch fixes this, but I'm not really sure it's the right
fix - I'd expect there to be a more principled way, doing resetting the
plan pointer when 'plan->saved == false'.

regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services