Re: plpgsq_plugin's stmt_end() is not called when an error is caught
On Fri, Dec 16, 2022 at 12:49 AM Tom Lane wrote: > > Masahiko Sawada 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
Re: plpgsq_plugin's stmt_end() is not called when an error is caught
On Thu, Dec 15, 2022 at 8:49 AM Tom Lane wrote: > Masahiko Sawada 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.
Re: plpgsq_plugin's stmt_end() is not called when an error is caught
Masahiko Sawada 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
Re: plpgsq_plugin's stmt_end() is not called when an error is caught
čt 15. 12. 2022 v 12:51 odesílatel Masahiko Sawada napsal: > On Thu, Dec 15, 2022 at 4:53 PM Kyotaro Horiguchi > wrote: > > > > At Thu, 15 Dec 2022 08:41:21 +0100, Pavel Stehule < > pavel.steh...@gmail.com> wrote in > > > čt 15. 12. 2022 v 8:25 odesílatel Masahiko Sawada < > sawada.m...@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 >
Re: plpgsq_plugin's stmt_end() is not called when an error is caught
On Thu, Dec 15, 2022 at 4:53 PM Kyotaro Horiguchi wrote: > > At Thu, 15 Dec 2022 08:41:21 +0100, Pavel Stehule > wrote in > > čt 15. 12. 2022 v 8:25 odesílatel Masahiko Sawada > > 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
Re: plpgsq_plugin's stmt_end() is not called when an error is caught
čt 15. 12. 2022 v 9:34 odesílatel Kyotaro Horiguchi napsal: > At Thu, 15 Dec 2022 09:03:12 +0100, Pavel Stehule > 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) { 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 >
Re: plpgsq_plugin's stmt_end() is not called when an error is caught
At Thu, 15 Dec 2022 09:03:12 +0100, Pavel Stehule 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
Re: plpgsq_plugin's stmt_end() is not called when an error is caught
čt 15. 12. 2022 v 8:53 odesílatel Kyotaro Horiguchi napsal: > At Thu, 15 Dec 2022 08:41:21 +0100, Pavel Stehule > wrote in > > čt 15. 12. 2022 v 8:25 odesílatel Masahiko Sawada > > > 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 https://github.com/okbob/plpgsql_check/commit/9a17e97354a48913de5219048ee3be6f8460bae9 regards Pavel regards. > > -- > Kyotaro Horiguchi > NTT Open Source Software Center >
Re: plpgsq_plugin's stmt_end() is not called when an error is caught
At Thu, 15 Dec 2022 08:41:21 +0100, Pavel Stehule wrote in > čt 15. 12. 2022 v 8:25 odesílatel Masahiko Sawada > 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
Re: plpgsq_plugin's stmt_end() is not called when an error is caught
čt 15. 12. 2022 v 8:25 odesílatel Masahiko Sawada 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 >
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 test_plpgsql.patch Description: Binary data