some last patches breaks plan cache

Started by Pavel Stehulealmost 8 years ago10 messages

pavel.stehule@gmail.com

almost 8 years ago

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

Tomas Vondra

tomas.vondra@2ndquadrant.com

almost 8 years ago

In reply to: Pavel Stehule (#1)

Re: some last patches breaks plan cache

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

Tomas Vondra

tomas.vondra@2ndquadrant.com

almost 8 years ago

In reply to: Tomas Vondra (#2)

Re: some last patches breaks plan cache

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

Tomas Vondra

tomas.vondra@2ndquadrant.com

almost 8 years ago

In reply to: Tomas Vondra (#3)

1 attachment(s)

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

Attachments:

plpgsql-call-plan-fix.difftext/x-patch; name=plpgsql-call-plan-fix.diffDownload

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

Pavel Stehule

pavel.stehule@gmail.com

almost 8 years ago

In reply to: Tomas Vondra (#4)

Re: some last patches breaks plan cache

2018-04-01 1:00 GMT+02:00 Tomas Vondra <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

Show quoted text

regards

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

Tomas Vondra

tomas.vondra@2ndquadrant.com

almost 8 years ago

In reply to: Pavel Stehule (#5)

1 attachment(s)

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

Attachments:

plpgsql-call-plan-fix-v2.difftext/x-patch; name=plpgsql-call-plan-fix-v2.diffDownload

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

Tom Lane

tgl@sss.pgh.pa.us

almost 8 years ago

In reply to: Tomas Vondra (#6)

Re: some last patches breaks plan cache

Tomas Vondra <tomas.vondra@2ndquadrant.com> 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

Tomas Vondra

tomas.vondra@2ndquadrant.com

almost 8 years ago

In reply to: Tom Lane (#7)

Re: some last patches breaks plan cache

On 04/04/2018 07:54 PM, Tom Lane wrote:

Tomas Vondra <tomas.vondra@2ndquadrant.com> 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

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

almost 8 years ago

In reply to: Tomas Vondra (#8)

Re: some last patches breaks plan cache

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

#10

Pavel Stehule

pavel.stehule@gmail.com

almost 8 years ago

In reply to: Peter Eisentraut (#9)

Re: some last patches breaks plan cache

2018-04-05 21:01 GMT+02:00 Peter Eisentraut <
peter.eisentraut@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

Show quoted text

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