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