Re: Nicely exiting PG_TRY and PG_CATCH

[Mikhail Gribkov]

> Yeah, you can't return or goto out of the PG_TRY part.

So this is a problem if the check would ever work.
(Sorry for such a delayed answer.)

Then we need to fix it. Attached is a minimal patch, which changes nothing
except for correct PG_TRY exiting.
Isn't it better this way?

CCing to Peter Eisentraut, whose patch originally introduced these returns.
Peter, will such a patch work somehow against your initial idea?

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick



On Sat, Oct 8, 2022 at 12:19 AM Tom Lane  wrote:

> Mikhail Gribkov  writes:
> > Usually it's not a good idea to exit PG_TRY() block via return statement.
> > Otherwise it would leave PG_exception_stack global variable in a wrong
> > state and next ereport() will jump to some junk address.
>
> Yeah, you can't return or goto out of the PG_TRY part.
>
> > Another suspicious case is PG_CATCH block in jsonb_plpython.c:
>
> This should be OK.  The PG_CATCH and PG_FINALLY macros are set up so that
> we've fully restored that state *before* we execute any of the
> error-handling code.  It would be basically impossible to have a guarantee
> that CATCH blocks never throw errors; they'd be so restricted as to be
> near useless, like signal handlers.
>
> regards, tom lane
>
diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c
index 923703535a..0c8550e53c 100644
--- a/src/pl/plpython/plpy_exec.c
+++ b/src/pl/plpython/plpy_exec.c
@@ -418,7 +418,7 @@ PLy_function_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc)
 	{
 		args = PyList_New(proc->nargs);
 		if (!args)
-			return NULL;
+			goto end_of_try;
 
 		for (i = 0; i < proc->nargs; i++)
 		{
@@ -458,6 +458,7 @@ PLy_function_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc)
 			/* cache the output conversion functions */
 			PLy_output_setup_record(>result, desc, proc);
 		}
+end_of_try:
 	}
 	PG_CATCH();
 	{
@@ -694,7 +695,7 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
 	{
 		pltdata = PyDict_New();
 		if (!pltdata)
-			return NULL;
+			goto end_of_try;
 
 		pltname = PLyUnicode_FromString(tdata->tg_trigger->tgname);
 		PyDict_SetItemString(pltdata, "name", pltname);
@@ -839,7 +840,8 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
 			if (!pltargs)
 			{
 Py_DECREF(pltdata);
-return NULL;
+pltdata = NULL;
+goto end_of_try;
 			}
 			for (i = 0; i < tdata->tg_trigger->tgnargs; i++)
 			{
@@ -858,6 +860,7 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
 		}
 		PyDict_SetItemString(pltdata, "args", pltargs);
 		Py_DECREF(pltargs);
+end_of_try:
 	}
 	PG_CATCH();
 	{

Re: Nicely exiting PG_TRY and PG_CATCH

[Tom Lane]

Mikhail Gribkov  writes:
> Usually it's not a good idea to exit PG_TRY() block via return statement.
> Otherwise it would leave PG_exception_stack global variable in a wrong
> state and next ereport() will jump to some junk address.

Yeah, you can't return or goto out of the PG_TRY part.

> Another suspicious case is PG_CATCH block in jsonb_plpython.c:

This should be OK.  The PG_CATCH and PG_FINALLY macros are set up so that
we've fully restored that state *before* we execute any of the
error-handling code.  It would be basically impossible to have a guarantee
that CATCH blocks never throw errors; they'd be so restricted as to be
near useless, like signal handlers.

regards, tom lane

Nicely exiting PG_TRY and PG_CATCH

[Mikhail Gribkov]

Hi hackers,

I've found some odd lines in plpython-related code. These look to me like a
potential source of problems, but maybe I'm not fully aware of some nuances.

Usually it's not a good idea to exit PG_TRY() block via return statement.
Otherwise it would leave PG_exception_stack global variable in a wrong
state and next ereport() will jump to some junk address. But here it is a
straight return in plpy_exec.c:
PG_TRY();
{
args = PyList_New(proc->nargs);
if (!args)
return NULL;
...
(
https://github.com/postgres/postgres/blob/0fe954c28584169938e5c0738cfaa9930ce77577/src/pl/plpython/plpy_exec.c#L421
)

Two more cases could be found further in the same file:
https://github.com/postgres/postgres/blob/0fe954c28584169938e5c0738cfaa9930ce77577/src/pl/plpython/plpy_exec.c#L697
https://github.com/postgres/postgres/blob/0fe954c28584169938e5c0738cfaa9930ce77577/src/pl/plpython/plpy_exec.c#L842

Isn't it a problem?


Another suspicious case is PG_CATCH block in jsonb_plpython.c:
PG_CATCH();
{
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
 errmsg("could not convert value \"%s\" to jsonb", str)));
}
(
https://github.com/postgres/postgres/blob/0fe954c28584169938e5c0738cfaa9930ce77577/contrib/jsonb_plpython/jsonb_plpython.c#L372
)

The problem is that  leaving PG_CATCH() without PG_RE_THROW(),
ReThrowError() or FlushErrorState() will consume an errordata stack slot,
while the stack size is only 5. Do this five times and we'll have a PANIC
on the next ereport().
As it's stated in elog.c (comment about the error stack depth at the
beginning of FlushErrorState()), "The only case where it would be more than
one deep is if we serviced an error that interrupted construction of
another message."
Looks to me that  FlushErrorState() should be inserted before this ereport.
Shouldn't it?

--
 best regards,
Mikhail A. Gribkov