Обсуждение: plpgsq_plugin's stmt_end() is not called when an error is caught
Hi,
While investigating the issue reported on pg_hint_plan[1], I realized
that stmt_end() callback is not called if an error raised during the
statement execution is caught. I've attached the patch to check when
stmt_beg() and stmt_end() are called. Here is an example:
postgres(1:3220232)=# create or replace function testfn(a text) returns int as
$$
declare
x int;
begin
select a::int into x;
return x;
exception when others then return 99;
end;
$$
language plpgsql;
CREATE FUNCTION
postgres(1:3220232)=# select testfn('1');
NOTICE: stmt_beg toplevel_block
NOTICE: stmt_beg stmt SQL statement
NOTICE: stmt_end stmt SQL statement
NOTICE: stmt_beg stmt RETURN
NOTICE: stmt_end stmt RETURN
NOTICE: stmt_end toplevel_block
testfn
--------
1
(1 row)
postgres(1:3220232)=# select testfn('x');
NOTICE: stmt_beg toplevel_block
NOTICE: stmt_beg stmt SQL statement
NOTICE: stmt_beg stmt RETURN
NOTICE: stmt_end stmt RETURN
NOTICE: stmt_end toplevel_block
testfn
--------
99
(1 row)
In exec_stmt_block(), we call exec_stmts() in a PG_TRY() block and
call stmt_beg() and stmt_end() callbacks for each statement executed
there. However, if an error is caught during executing a statement, we
jump to PG_CATCH() block in exec_stmt_block() so we don't call
stmt_end() callback that is supposed to be called in exec_stmts(). To
fix it, I think we can call stmt_end() callback in PG_CATCH() block.
pg_hint_plan increments and decrements a count in stmt_beg() and
stmt_end() callbacks, respectively[2]. It resets the counter when
raising an ERROR (not caught). But if an ERROR is caught, the counter
could be left as an invalid value.
Is this a bug in plpgsql?
Regards,
[1] https://github.com/ossc-db/pg_hint_plan/issues/93
[2] https://github.com/ossc-db/pg_hint_plan/blob/master/pg_hint_plan.c#L4870
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Вложения
čt 15. 12. 2022 v 8:25 odesílatel Masahiko Sawada <sawada.mshk@gmail.com> napsal:
Hi,
While investigating the issue reported on pg_hint_plan[1], I realized
that stmt_end() callback is not called if an error raised during the
statement execution is caught. I've attached the patch to check when
stmt_beg() and stmt_end() are called. Here is an example:
postgres(1:3220232)=# create or replace function testfn(a text) returns int as
$$
declare
x int;
begin
select a::int into x;
return x;
exception when others then return 99;
end;
$$
language plpgsql;
CREATE FUNCTION
postgres(1:3220232)=# select testfn('1');
NOTICE: stmt_beg toplevel_block
NOTICE: stmt_beg stmt SQL statement
NOTICE: stmt_end stmt SQL statement
NOTICE: stmt_beg stmt RETURN
NOTICE: stmt_end stmt RETURN
NOTICE: stmt_end toplevel_block
testfn
--------
1
(1 row)
postgres(1:3220232)=# select testfn('x');
NOTICE: stmt_beg toplevel_block
NOTICE: stmt_beg stmt SQL statement
NOTICE: stmt_beg stmt RETURN
NOTICE: stmt_end stmt RETURN
NOTICE: stmt_end toplevel_block
testfn
--------
99
(1 row)
In exec_stmt_block(), we call exec_stmts() in a PG_TRY() block and
call stmt_beg() and stmt_end() callbacks for each statement executed
there. However, if an error is caught during executing a statement, we
jump to PG_CATCH() block in exec_stmt_block() so we don't call
stmt_end() callback that is supposed to be called in exec_stmts(). To
fix it, I think we can call stmt_end() callback in PG_CATCH() block.
pg_hint_plan increments and decrements a count in stmt_beg() and
stmt_end() callbacks, respectively[2]. It resets the counter when
raising an ERROR (not caught). But if an ERROR is caught, the counter
could be left as an invalid value.
Is this a bug in plpgsql?
I think it is by design. There is not any callback that is called after an exception.
It is true, so some callbacks on statement error and function's error can be nice. It can help me to implement profilers, or tracers more simply and more robustly.
But I am not sure about performance impacts. This is on a critical path.
Regards
Pavel
Regards,
[1] https://github.com/ossc-db/pg_hint_plan/issues/93
[2] https://github.com/ossc-db/pg_hint_plan/blob/master/pg_hint_plan.c#L4870
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
At Thu, 15 Dec 2022 08:41:21 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in > čt 15. 12. 2022 v 8:25 odesílatel Masahiko Sawada <sawada.mshk@gmail.com> > napsal: > > Is this a bug in plpgsql? > > > > I think it is by design. There is not any callback that is called after an > exception. > > It is true, so some callbacks on statement error and function's error can > be nice. It can help me to implement profilers, or tracers more simply and > more robustly. > > But I am not sure about performance impacts. This is on a critical path. I didn't searched for, but I guess all of the end-side callback of all begin-end type callbacks are not called on exception. Additional PG_TRY level wouldn't be acceptable for performance reasons. What we (pg_hint_plan people) want is any means to know that the top-level function is exited, to reset function nest level. It would be simpler than calling end callback at every nest level. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
čt 15. 12. 2022 v 8:53 odesílatel Kyotaro Horiguchi <horikyota.ntt@gmail.com> napsal:
At Thu, 15 Dec 2022 08:41:21 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in
> čt 15. 12. 2022 v 8:25 odesílatel Masahiko Sawada <sawada.mshk@gmail.com>
> napsal:
> > Is this a bug in plpgsql?
> >
>
> I think it is by design. There is not any callback that is called after an
> exception.
>
> It is true, so some callbacks on statement error and function's error can
> be nice. It can help me to implement profilers, or tracers more simply and
> more robustly.
>
> But I am not sure about performance impacts. This is on a critical path.
I didn't searched for, but I guess all of the end-side callback of all
begin-end type callbacks are not called on exception. Additional
PG_TRY level wouldn't be acceptable for performance reasons.
What we (pg_hint_plan people) want is any means to know that the
top-level function is exited, to reset function nest level. It would
be simpler than calling end callback at every nest level.
I found some solution based by using fmgr hook
regards
Pavel
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
At Thu, 15 Dec 2022 09:03:12 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in > I found some solution based by using fmgr hook > > https://github.com/okbob/plpgsql_check/commit/9a17e97354a48913de5219048ee3be6f8460bae9 Oh! Thanks for the pointer, will look into that. By the way, It seems to me that the tool is using RegisterResourceReleaseCallback to reset the function nest level. But since there's a case where the mechanism doesn't work, I suspect that the callback can be missed in some cases of error return, which seems to be a bug if it is true.. # I haven't confirmed that behavior by myself, though. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
čt 15. 12. 2022 v 9:34 odesílatel Kyotaro Horiguchi <horikyota.ntt@gmail.com> napsal:
At Thu, 15 Dec 2022 09:03:12 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in
> I found some solution based by using fmgr hook
>
> https://github.com/okbob/plpgsql_check/commit/9a17e97354a48913de5219048ee3be6f8460bae9
Oh! Thanks for the pointer, will look into that.
By the way, It seems to me that the tool is using
RegisterResourceReleaseCallback to reset the function nest level. But
since there's a case where the mechanism doesn't work, I suspect that
the callback can be missed in some cases of error return, which seems
to be a bug if it is true..
# I haven't confirmed that behavior by myself, though.
it should be executed
/*
* Register or deregister callback functions for resource cleanup
*
* These functions are intended for use by dynamically loaded modules.
* For built-in modules we generally just hardwire the appropriate calls.
*
* Note that the callback occurs post-commit or post-abort, so the callback
* functions can only do noncritical cleanup.
*/
void
RegisterResourceReleaseCallback(ResourceReleaseCallback callback, void *arg)
{
* Register or deregister callback functions for resource cleanup
*
* These functions are intended for use by dynamically loaded modules.
* For built-in modules we generally just hardwire the appropriate calls.
*
* Note that the callback occurs post-commit or post-abort, so the callback
* functions can only do noncritical cleanup.
*/
void
RegisterResourceReleaseCallback(ResourceReleaseCallback callback, void *arg)
{
but it is based on resource owner, so timing can be different than you expect
Regards
Pavel
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Thu, Dec 15, 2022 at 4:53 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Thu, 15 Dec 2022 08:41:21 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in > > čt 15. 12. 2022 v 8:25 odesílatel Masahiko Sawada <sawada.mshk@gmail.com> > > napsal: > > > Is this a bug in plpgsql? > > > > > > > I think it is by design. There is not any callback that is called after an > > exception. > > > > It is true, so some callbacks on statement error and function's error can > > be nice. It can help me to implement profilers, or tracers more simply and > > more robustly. > > > > But I am not sure about performance impacts. This is on a critical path. > > I didn't searched for, but I guess all of the end-side callback of all > begin-end type callbacks are not called on exception. Additional > PG_TRY level wouldn't be acceptable for performance reasons. I don't think we need additional PG_TRY() for that since exec_stmts() is already called in PG_TRY() if there is an exception block. I meant to call stmt_end() in PG_CATCH() in exec_stmt_block() (i.e. only when an error is caught by the exception block). Currently, if an error is caught, we call stmt_begin() and stmt_end() for statements executed inside the exception block but call only stmt_begin() for the statement that raised an error. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
čt 15. 12. 2022 v 12:51 odesílatel Masahiko Sawada <sawada.mshk@gmail.com> napsal:
On Thu, Dec 15, 2022 at 4:53 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Thu, 15 Dec 2022 08:41:21 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in
> > čt 15. 12. 2022 v 8:25 odesílatel Masahiko Sawada <sawada.mshk@gmail.com>
> > napsal:
> > > Is this a bug in plpgsql?
> > >
> >
> > I think it is by design. There is not any callback that is called after an
> > exception.
> >
> > It is true, so some callbacks on statement error and function's error can
> > be nice. It can help me to implement profilers, or tracers more simply and
> > more robustly.
> >
> > But I am not sure about performance impacts. This is on a critical path.
>
> I didn't searched for, but I guess all of the end-side callback of all
> begin-end type callbacks are not called on exception. Additional
> PG_TRY level wouldn't be acceptable for performance reasons.
I don't think we need additional PG_TRY() for that since exec_stmts()
is already called in PG_TRY() if there is an exception block. I meant
to call stmt_end() in PG_CATCH() in exec_stmt_block() (i.e. only when
an error is caught by the exception block). Currently, if an error is
caught, we call stmt_begin() and stmt_end() for statements executed
inside the exception block but call only stmt_begin() for the
statement that raised an error.
PG_TRY is used only for STMT_BLOCK, other statements don't use PG_TRY.
I have no idea about possible performance impacts, I never tested it. Personally, I like the possibility of having some error callback function. Maybe PG_TRY can be used, only when this callback is used. So there will not be any impact on performance without some extensions that use it. Unfortunately, there are two functions necessary. Some exceptions can be raised after the last statement before the function ends. Changing behaviour of stmt_end or func_end can be problematic, because after an exception a lot of internal API is not available, and you should know, so this is that situation. Now anybody knows so at stmt_end function, the code is not after an exception.
But it can be not too easy, because there can be more chained extensions that use dbg API - like PL profiler, PL debugger and plpgsql_check - and maybe others.
Regards
Pavel
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Masahiko Sawada <sawada.mshk@gmail.com> writes:
> I don't think we need additional PG_TRY() for that since exec_stmts()
> is already called in PG_TRY() if there is an exception block. I meant
> to call stmt_end() in PG_CATCH() in exec_stmt_block() (i.e. only when
> an error is caught by the exception block). Currently, if an error is
> caught, we call stmt_begin() and stmt_end() for statements executed
> inside the exception block but call only stmt_begin() for the
> statement that raised an error.
I fail to see anything wrong with that. We never completed execution
of the statement that raised an error, but calling stmt_end for it
would imply that we did. I think changing this will break more things
than it fixes, completely independently of whatever cost it would add.
Or in other words: the initial complaint describes a bug in pg_hint_plan,
not one in plpgsql.
regards, tom lane
On Thu, Dec 15, 2022 at 8:49 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Masahiko Sawada <sawada.mshk@gmail.com> writes:
> I don't think we need additional PG_TRY() for that since exec_stmts()
> is already called in PG_TRY() if there is an exception block. I meant
> to call stmt_end() in PG_CATCH() in exec_stmt_block() (i.e. only when
> an error is caught by the exception block). Currently, if an error is
> caught, we call stmt_begin() and stmt_end() for statements executed
> inside the exception block but call only stmt_begin() for the
> statement that raised an error.
I fail to see anything wrong with that. We never completed execution
of the statement that raised an error, but calling stmt_end for it
would imply that we did. I think changing this will break more things
than it fixes, completely independently of whatever cost it would add.
Or in other words: the initial complaint describes a bug in pg_hint_plan,
not one in plpgsql.
The OP suggests needing something akin to a "finally" callback for statement. While a fine feature request for plpgsql its absence doesn't constitute a bug.
David J.
On Fri, Dec 16, 2022 at 12:49 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Masahiko Sawada <sawada.mshk@gmail.com> writes: > > I don't think we need additional PG_TRY() for that since exec_stmts() > > is already called in PG_TRY() if there is an exception block. I meant > > to call stmt_end() in PG_CATCH() in exec_stmt_block() (i.e. only when > > an error is caught by the exception block). Currently, if an error is > > caught, we call stmt_begin() and stmt_end() for statements executed > > inside the exception block but call only stmt_begin() for the > > statement that raised an error. > > I fail to see anything wrong with that. We never completed execution > of the statement that raised an error, but calling stmt_end for it > would imply that we did. Thank you for the comment. Agreed. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com