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(dot)vondra(at)2ndquadrant(dot)com
> <mailto:tomas(dot)vondra(at)2ndquadrant(dot)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