Re: proposal: PL/Pythonu - function ereport

2015-10-28 7:25 GMT+01:00 Catalin Iacob <iacobcatalin(at)gmail(dot)com>:

> 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.
>

Thank you very much for your analyse. I am sending new version of proposed
patch with Python3 support. Fixed missing check of dictionary
initialization.

Regards

Pavel