Re: proposal: PL/Pythonu - function ereport

Hi

2015-10-23 7:34 GMT+02:00 Catalin Iacob <iacobcatalin(at)gmail(dot)com>:

> On Sat, Oct 17, 2015 at 8:18 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> > here is new patch
> >
> > cleaned, all unwanted artefacts removed. I am not sure if used way for
> > method registration is 100% valid, but I didn't find any related
> > documentation.
>
> Pavel, some notes about the patch, not a full review (yet?).
>
> In PLy_add_exceptions PyDict_New is not checked for failure.
>
> In PLy_spi_error__init__, kw will be NULL if SPIError is called with
> no arguments. With the current code NULL will get passed to
> PyDict_Size which will generate something like SystemError Bad
> internal function call. This also means a test using just SPIError()
> is needed.
> It seems args should never be NULL by the way, if there are no
> arguments it's an empty tuple so the other side of the check is ok.
>
> 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.

>
> There is http://bugs.python.org/issue1587 which discusses how to
> replace the "third argument" functionality for PyMethod_New in
> Python3. One of the messages there links to
> http://bugs.python.org/issue1505 and
> https://hg.python.org/cpython/rev/429cadbc5b10/ which has an example
> very similar to what you're trying to do, rewritten to work in
> Python3. But this is still confusing: note that the replaced code
> *didn't really use PyMethod_New at all* as the removed line meth =
> PyMethod_New(func, NULL, ComError); was commented out and the replaced
> code used to simply assign the function to the class dictionary
> without even creating a method.
> 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.
>
> I tried to answer the "is it safe to use NULL for the third argument
> of PyMethod_New in Python2?" question and don't have a definite answer
> yet. If you look at the CPython code it's used to set im_class
> (Objects/classobject.c) of PyMethodObject which is accessible from
> Python.
> But this code:
> init = plpy.SPIError.__init__
> plpy.notice("repr {} str {} im_class {}".format(repr(init), str(init),
> init.im_class))
> produces:
> NOTICE: repr <unbound method SPIError.__init__> str <unbound method
> SPIError.__init__> im_class <class 'plpy.SPIError'>
> so the SPIError class name is set in im_class from somewhere. But this
> is all moot if you use the explicit SPIError type definition.
>

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?

>
> >> Any help is welcome
>
> I can work with you on this. I don't have a lot of experience with the
> C API but not zero either.
>

It is very helpful for me - C API doesn't look difficult, but when I have
to do some really low level work I am lost :(

Regards

Pavel