Re: some last patches breaks plan cache

On 04/01/2018 10:01 AM, Pavel Stehule wrote:
> 
> 
> 2018-04-01 1:00 GMT+02:00 Tomas Vondra <tomas.vondra@2ndquadrant.com
> <mailto:tomas.vondra@2ndquadrant.com>>:
> 
> 
> 
>     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'.
> 
> 
> it fixes some issue, but not all
> 
> I see changes in plpgsql_check regress tests
> 
> CREATE OR REPLACE PROCEDURE public.testproc()
>  LANGUAGE plpgsql
> AS $procedure$
> declare r int;
> begin
>   call proc(10, r + 10, 20);
> end;
> $procedure$
> 
> postgres=# call testproc();
> ERROR:  argument 2 is an output argument but is not writable
> CONTEXT:  PL/pgSQL function testproc() line 4 at CALL
> postgres=# call testproc();
> ERROR:  SPI_execute_plan_with_paramlist failed executing query "CALL
> proc(10, r + 10, 20)": SPI_ERROR_ARGUMENT
> CONTEXT:  PL/pgSQL function testproc() line 4 at CALL
> 

This should do the trick - I've failed to realize exec_stmt_call may
exit by calling elog(ERROR) too, in which case the plan pointer was not
reset.

This does fix the failures presented here, but I don't think it's the
right solution - for example, if any other function call ends with
elog(ERROR), the dangling pointer will be there. There must be a better
place to cleanup this automatically ...

regards

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