Confused coding in PLy_traceback()

My attention happened to be drawn to plpython's PLy_traceback()
function, and I noted a couple of things that sure seem like bugs.
First, there's this bit:

e_type_o = PyObject_GetAttrString(e, "__name__");
e_module_o = PyObject_GetAttrString(e, "__module__");
if (e_type_o)
e_type_s = PyString_AsString(e_type_o);
if (e_type_s)
e_module_s = PyString_AsString(e_module_o);

Surely that second "if" is meant to be "if (e_module_o)"? It doesn't
make any sense to be testing whether we could get a string from
e_type_o to decide if it's safe to touch e_module_o. This is probably
only a latent bug because not getting these strings is a can't-happen
case, but still.

Second, the whole function shows truly remarkable faith that none of
what it calls will ever throw an error. If that does happen, the code
will leak PyObject references --- and it *can* happen, if only because
of the possibility of OOM in string allocation. Since the code seems
to be trying not to leak those, the fact that its coverage is so
incomplete seems like a bug.

I then realized that there's another fundamental risk of leaking
PyObject references, which is that this function is charged with
dropping all the references in the passed-in traceback stack;
but if it errors out anywhere, it'll fail to do that. It would
be better for that responsibility to lie with PLy_elog_impl which
obtained the stack in the first place (and which also has unwarranted
faith in its callees not failing).

So I propose the attached. For ease of review, I've not re-indented
the code that needs to move inside PG_TRY blocks. Also, I dropped the
logic about pfree'ing the string buffers in PLy_elog_impl's PG_FINALLY
block: that doesn't seem necessary, and continuing to do it would
require making those things volatile which is notationally messy.

regards, tom lane