Re: PL/Python: Fix return in the middle of PG_TRY() block.

Hi,

On 2023-01-12 10:44:33 -0800, Nathan Bossart wrote:
> On Thu, Jan 12, 2023 at 11:19:29PM +0800, Xing Guo wrote:
> > @@ -690,12 +690,12 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
> >  	PyObject   *volatile pltdata = NULL;
> >  	char	   *stroid;
> >  
> > +	pltdata = PyDict_New();
> > +	if (!pltdata)
> > +		return NULL;
> > +
> >  	PG_TRY();
> >  	{
> > -		pltdata = PyDict_New();
> > -		if (!pltdata)
> > -			return NULL;
> > -
> >  		pltname = PLyUnicode_FromString(tdata->tg_trigger->tgname);
> >  		PyDict_SetItemString(pltdata, "name", pltname);
> >  		Py_DECREF(pltname);
> 
> There's another "return" later on in this PG_TRY block.  I wonder if it's
> possible to detect this sort of thing at compile time.

Clang provides some annotations that allow to detect this kind of thing. I
hacked up a test for this, and it finds quite a bit of prolematic
code. plpython is, uh, not being good? But also in plperl, pltcl.

Example complaints:

[776/1239 42  62%] Compiling C object src/pl/plpython/plpython3.so.p/plpy_exec.c.o
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c:472:1: warning: no_returns_in_pg_try 'no_returns_handle' is not held on every path through here [-Wthread-safety-analysis]
}
^
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c:417:2: note: no_returns_in_pg_try acquired here
        PG_TRY();
        ^
../../../../home/andres/src/postgresql/src/include/utils/elog.h:424:7: note: expanded from macro 'PG_TRY'
                    no_returns_start(no_returns_handle##__VA_ARGS__)
                    ^
...
[785/1239 42  63%] Compiling C object src/pl/tcl/pltcl.so.p/pltcl.c.o
../../../../home/andres/src/postgresql/src/pl/tcl/pltcl.c:1830:1: warning: no_returns_in_pg_try 'no_returns_handle' is not held on every path through here [-Wthread-safety-analysis]
}
^
../../../../home/andres/src/postgresql/src/pl/tcl/pltcl.c:1809:2: note: no_returns_in_pg_try acquired here
        PG_CATCH();
        ^
../../../../home/andres/src/postgresql/src/include/utils/elog.h:433:7: note: expanded from macro 'PG_CATCH'
                    no_returns_start(no_returns_handle##__VA_ARGS__)
                    ^

Not perfect digestible, but also not too bad. I pushed the
no_returns_start()/no_returns_stop() calls into all the PG_TRY related macros,
because that causes the warning to point to block that the problem is
in. E.g. above the first warning points to PG_TRY, the second to
PG_CATCH. It'd work to just put it into PG_TRY and PG_END_TRY.


Clearly this would need a bunch more work, but it seems promising? I think
there'd be other uses than this.


I briefly tried to use it for spinlocks. Mostly works and detects things like
returning with a spinlock held. But it does not like dynahash's habit of
conditionally acquiring and releasing spinlocks.

Greetings,

Andres Freund