On 04/01/2018 10:01 AM, Pavel Stehule wrote:
>
>
> 2018-04-01 1:00 GMT+02:00 Tomas Vondra <tomas.von...@2ndquadrant.com
> <mailto:tomas.von...@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
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 67123f8..4337b78 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2167,9 +2167,15 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
Param *param;
if (!IsA(n, Param))
+ {
+ /* cleanup the plan pointer */
+ if (!estate->atomic)
+ expr->plan = NULL;
+
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("argument %d is an output argument but is not writable", i + 1)));
+ }
param = castNode(Param, n);
/* paramid is offset by 1 (see make_datum_param()) */
@@ -2193,6 +2199,10 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
exec_eval_cleanup(estate);
SPI_freetuptable(SPI_tuptable);
+ /* cleanup the plan pointer */
+ if (!estate->atomic)
+ expr->plan = NULL;
+
return PLPGSQL_RC_OK;
}
