Re: some last patches breaks plan cache

[Pavel Stehule]

2018-04-05 21:01 GMT+02:00 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com>:

> On 4/4/18 14:03, Tomas Vondra wrote:
> >> If there's really no other way, you could use a PG_TRY block to
> >> ensure that the pointer gets reset on the way out. But I question
> >> why we've got a design that requires that in the first place. It's
> >> likely to have more problems than this.
> >
> > I agree it needs a solution that does not require us to track and
> > manually reset pointers on random places. No argument here.
>
> I've committed a fix with PG_TRY.
>
> A more complete solution would be to able to keep the plan independent
> of a resowner.  That would require a bit more deep surgery in SPI, it
> seems.  I'll take a look if it's doable.
>

The issues that I detected in plpgsql_check are fixed

Thank you

Pavel

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

Re: some last patches breaks plan cache

[Peter Eisentraut]

On 4/4/18 14:03, Tomas Vondra wrote:
>> If there's really no other way, you could use a PG_TRY block to 
>> ensure that the pointer gets reset on the way out. But I question
>> why we've got a design that requires that in the first place. It's
>> likely to have more problems than this.
> 
> I agree it needs a solution that does not require us to track and
> manually reset pointers on random places. No argument here.

I've committed a fix with PG_TRY.

A more complete solution would be to able to keep the plan independent
of a resowner.  That would require a bit more deep surgery in SPI, it
seems.  I'll take a look if it's doable.

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

Re: some last patches breaks plan cache

[Tomas Vondra]

On 04/04/2018 07:54 PM, Tom Lane wrote:
> Tomas Vondra  writes:
>> 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
> 
> No, it's completely unacceptable.
> 

Yes, I realize that and I was not really suggesting this as a proper
fix. It was meant more to demonstrate that it's still the same issue
with the same dangling pointer.

> If there's really no other way, you could use a PG_TRY block to 
> ensure that the pointer gets reset on the way out. But I question
> why we've got a design that requires that in the first place. It's
> likely to have more problems than this.
> 

I agree it needs a solution that does not require us to track and
manually reset pointers on random places. No argument here.

regards

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

Re: some last patches breaks plan cache

[Tom Lane]

Tomas Vondra  writes:
> 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

No, it's completely unacceptable.

If there's really no other way, you could use a PG_TRY block to ensure
that the pointer gets reset on the way out.  But I question why we've
got a design that requires that in the first place.  It's likely to
have more problems than this.

regards, tom lane

Re: some last patches breaks plan cache

[Tomas Vondra]

On 04/01/2018 10:01 AM, Pavel Stehule wrote:
> 
> 
> 2018-04-01 1:00 GMT+02:00 Tomas Vondra  >:
> 
> 
> 
> 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;
 }
 

Re: some last patches breaks plan cache

[Pavel Stehule]

2018-04-01 1:00 GMT+02:00 Tomas Vondra :

>
>
> 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




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

Re: some last patches breaks plan cache

[Tomas Vondra]

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
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index fc0f0f0..ae8bed0 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2193,6 +2193,9 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
 	exec_eval_cleanup(estate);
 	SPI_freetuptable(SPI_tuptable);
 
+	if (!expr->plan->saved)
+		expr->plan = NULL;
+
 	return PLPGSQL_RC_OK;
 }
 

Re: some last patches breaks plan cache

[Tomas Vondra]

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.

regards

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

Re: some last patches breaks plan cache

[Tomas Vondra]

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 :-/

regards

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

some last patches breaks plan cache

[Pavel Stehule]

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

Regards

Pavel