plpgsq_plugin's stmt_end() is not called when an error is caught

č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

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

https://github.com/okbob/plpgsql_check/commit/9a17e97354a48913de5219048ee3be6f8460bae9

regards

Pavel

regards.

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

but it is based on resource owner, so timing can be different than you
expect

Regards

Pavel

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

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