Обсуждение: some last patches breaks plan cache
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 failsCREATE 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=#
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
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
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
Вложения
2018-04-01 1:00 GMT+02:00 Tomas Vondra <tomas.vondra@2ndquadrant.com>:
The attached patch fixes this, but I'm not really sure it's the right
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.
>
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
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
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
Вложения
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
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
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
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
--
Peter Eisentraut http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services