Re: proposal: PL/Pythonu - function ereport

On Tue, Oct 27, 2015 at 9:34 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Hi
>
> 2015-10-23 7:34 GMT+02:00 Catalin Iacob <iacobcatalin(at)gmail(dot)com>:
>> The current code doesn't build on Python3 because the 3rd argument of
>> PyMethod_New, the troubled one you need set to NULL has been removed.
>> This has to do with the distinction between bound and unbound methods
>> which is gone in Python3.
>
>
> this part of is well isolated, and we can do switch for Python2 and Python3
> without significant problems.

I had a quick look at this and at least 2 things are needed for Python3:

* using PyInstanceMethod_New instead of PyMethod_New; as
http://bugs.python.org/issue1587 and
https://docs.python.org/3/c-api/method.html explain that's the new way
of binding a PyCFunction to a class, PyMethod_New(func, NULL) fails

* in the PLy_spi_error_methods definition, __init__ has to take
METH_VARARGS | METH_KEYWORDS not just METH_KEYWORDS; in Python2
METH_KEYWORDS implied METH_VARARGS so it's not needed (it also doesn't
hurt) but if I don't add it, in Python3 I get:
ERROR: SystemError: Bad call flags in PyCFunction_Call. METH_OLDARGS
is no longer supported!

>> Still, the above link shows a (more verbose but maybe better)
>> alternative: don't use PyErr_NewException and instead define an
>> SPIError type with each slot spelled out explicitly. This will remove
>> the "is it safe to set the third argument to NULL" question.
>
> Should be there some problems, if we lost dependency on basic exception
> class? Some compatibility issues? Or is possible to create full type with
> inheritance support?

It's possible to give it a base type, see at
https://hg.python.org/cpython/rev/429cadbc5b10/ this line before
calling PyType_Ready:
PyComError_Type.tp_base = (PyTypeObject*)PyExc_Exception;

PyErr_NewException is a shortcut for defining simple Exception
deriving types, usually one defines a type with the full PyTypeObject
definition.

In the meantime, I had a deeper look at the 2.7.10 code and I trust
that PyMethod_New with the last argument set to NULL is ok. Setting
that to NULL will lead to the PyMethod representing __init__ im_class
being NULL. But that PyMethod object is not held onto by C code, it's
added to the SPIError class' dict. From there, it is always retrieved
from Python via an instance or via the class (so SPIError().__init__
or SPIError.__init__) which will lead to instancemethod_descr_get
being called because it's the tp_descr_get slot of PyMethod_Type and
that code knows the class you're retrieving the attribute from
(SPIError in this case), checks if the PyMethod already has a not NULL
im_class (which SPIError.__init__ doesn't) and, if not, calls
PyMethod_New again and passes the class (SPIError) as the 3rd
argument.

Given this, I think it's ok to keep using PyErr_NewException rather
than spelling out the type explicitly.