proposal: PL/Pythonu - function ereport

2015-10-08 12:11 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

Hi

We cannot to raise PostgreSQL exception with setting all possible fields.
I propose new function

plpy.ereport(level, [ message [, detail [, hint [, sqlstate, ... ]]]])

The implementation will be based on keyword parameters, so only required
parameters should be used.

Examples:

plpy.ereport(plpy.NOTICE, 'some message', 'some detai')
plpy.ereport(plpy.ERROR, 'some message', sqlstate = 'zx243');

patch attached

regards

Pavel

On 10/8/15 6:11 AM, Pavel Stehule wrote:

We cannot to raise PostgreSQL exception with setting all possible
fields.

Such as? If there are fields missing, let's add them.

I propose new function

plpy.ereport(level, [ message [, detail [, hint [, sqlstate, ... ]]]])

That's not how Python works. If you want to cause an error, you should
raise an exception.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2015-10-09 15:22 GMT+02:00 Peter Eisentraut <peter_e@gmx.net>:

On 10/8/15 6:11 AM, Pavel Stehule wrote:

We cannot to raise PostgreSQL exception with setting all possible
fields.

Such as? If there are fields missing, let's add them.

I propose new function

plpy.ereport(level, [ message [, detail [, hint [, sqlstate, ... ]]]])

That's not how Python works. If you want to cause an error, you should
raise an exception.

ok,

the patch had two parts - enhancing spi exception and ereport function.
I'll drop implementation of ereport.

Regards

Pavel

Hi

2015-10-09 15:22 GMT+02:00 Peter Eisentraut <peter_e@gmx.net>:

On 10/8/15 6:11 AM, Pavel Stehule wrote:

We cannot to raise PostgreSQL exception with setting all possible
fields.

Such as? If there are fields missing, let's add them.

I propose new function

plpy.ereport(level, [ message [, detail [, hint [, sqlstate, ... ]]]])

That's not how Python works. If you want to cause an error, you should
raise an exception.

I wrote a example, how to do it

postgres=# do $$
x = plpy.SPIError('Nazdarek');
x.spidata = (100, "Some detail", "some hint", None, None);
raise x;
$$ language plpythonu;

ERROR: T1000: plpy.SPIError: Nazdarek
DETAIL: Some detail
HINT: some hint
CONTEXT: Traceback (most recent call last):
PL/Python anonymous code block, line 4, in <module>
raise x;
PL/Python anonymous code block
LOCATION: PLy_elog, plpy_elog.c:106
Time: 1.170 ms

Is it some better way?

I see a few disadvantages

1. sqlcode is entered via integer
2. it doesn't allow keyword parameters - so we can second constructor of
SPIError

some like

postgres=# do $$
def SPIError(message, detail = None, hint = None):
x = plpy.SPIError(message)
x.spidata = (0, detail, hint, None, None)
return x

raise SPIError('Nazdar Svete', hint = 'Hello world');
$$ language plpythonu;

The main problem is a name for this function.

Regards

Pavel

On 16 October 2015 at 02:47, Pavel Stehule <pavel.stehule@gmail.com> wrote:

postgres=# do $$
x = plpy.SPIError('Nazdarek');
x.spidata = (100, "Some detail", "some hint", None, None);
raise x;
$$ language plpythonu;

Shouldn't that look more like

raise plpy.SPIError(msg="Message", sqlstate="0P001", hint="Turn it on
and off again") ?

Keyword args are very much the norm for this sort of thing. I recall
them being pretty reasonable to deal with in the CPython API too, but
otherwise a trivial Python wrapper in the module can easily adapt the
interface.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2015-10-16 8:12 GMT+02:00 Craig Ringer <craig@2ndquadrant.com>:

On 16 October 2015 at 02:47, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

postgres=# do $$
x = plpy.SPIError('Nazdarek');
x.spidata = (100, "Some detail", "some hint", None, None);
raise x;
$$ language plpythonu;

Shouldn't that look more like

raise plpy.SPIError(msg="Message", sqlstate="0P001", hint="Turn it on
and off again") ?

postgres=# do $$
raise plpy.SPIError(msg="Message", sqlstate="0P001", hint="Turn it on and
off again");
$$ language plpythonu;
ERROR: TypeError: SPIError does not take keyword arguments
CONTEXT: Traceback (most recent call last):
PL/Python anonymous code block, line 2, in <module>
raise plpy.SPIError(msg="Message", sqlstate="0P001", hint="Turn it on
and off again");
PL/Python anonymous code block
Time: 1.193 ms

Keyword args are very much the norm for this sort of thing. I recall
them being pretty reasonable to deal with in the CPython API too, but
otherwise a trivial Python wrapper in the module can easily adapt the
interface.

postgres=# do $$
class cSPIError(plpy.SPIError):
def __init__( self, message, detail = None, hint = None):
self.spidata = (0, detail, hint, None, None,)
self.args = ( message, )

x = cSPIError('Nazdarek', hint = 'some hint')
raise x
$$ language plpythonu;
ERROR: cSPIError: Nazdarek
HINT: some hint
CONTEXT: Traceback (most recent call last):
PL/Python anonymous code block, line 8, in <module>
raise x
PL/Python anonymous code block

This code is working, so it needs explicit constructor for class SPIError

Regards

Pavel

2015-10-16 8:12 GMT+02:00 Craig Ringer <craig@2ndquadrant.com>:

On 16 October 2015 at 02:47, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

postgres=# do $$
x = plpy.SPIError('Nazdarek');
x.spidata = (100, "Some detail", "some hint", None, None);
raise x;
$$ language plpythonu;

Shouldn't that look more like

raise plpy.SPIError(msg="Message", sqlstate="0P001", hint="Turn it on
and off again") ?

Keyword args are very much the norm for this sort of thing. I recall
them being pretty reasonable to deal with in the CPython API too, but
otherwise a trivial Python wrapper in the module can easily adapt the
interface.

I wrote a constructor for SPIError with keyword parameters support - see
attached patch

The code is working

postgres=# do $$
raise plpy.SPIError("pokus",hint = "some info");
$$ language plpythonu;
ERROR: plpy.SPIError: pokus
HINT: some info
CONTEXT: Traceback (most recent call last):
PL/Python anonymous code block, line 2, in <module>
raise plpy.SPIError("pokus",hint = "some info");
PL/Python anonymous code block

but the implementation is pretty ugly :( - I didn't write C extensions for
Python before, and the extending exception class with some methods isn't
well supported and well documented.

Any help is welcome

Regards

Pavel

but the implementation is pretty ugly :( - I didn't write C extensions for

Python before, and the extending exception class with some methods isn't
well supported and well documented.

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.

Regards

Pavel

On Sat, Oct 17, 2015 at 8:18 PM, Pavel Stehule <pavel.stehule@gmail.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.

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.

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi

2015-10-23 7:34 GMT+02:00 Catalin Iacob <iacobcatalin@gmail.com>:

On Sat, Oct 17, 2015 at 8:18 PM, Pavel Stehule <pavel.stehule@gmail.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

On Tue, Oct 27, 2015 at 9:34 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

2015-10-23 7:34 GMT+02:00 Catalin Iacob <iacobcatalin@gmail.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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2015-10-28 7:25 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:

On Tue, Oct 27, 2015 at 9:34 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hi

2015-10-23 7:34 GMT+02:00 Catalin Iacob <iacobcatalin@gmail.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

Hello,

Here's a detailed review:

1. in PLy_spi_error__init__ you need to check kw for NULL before doing
PyDict_Size(kw) otherwise for plpy.SPIError() you get Bad internal
call because PyDict_Size expects a real dictionary not NULL

2. a test with just plpy.SPIError() is still missing, this would have caught 1.

3. the tests have "possibility to set all accessable fields in custom
exception" above a test that doesn't set all fields, it's confusing
(and accessible is spelled wrong)

4. in the docs, "The constructor of SPIError exception (class)
supports keyword parameters." sounds better as "An SPIError instance
can be constructed using keyword parameters."

5. there is conceptual code duplication between PLy_spi_exception_set
in plpy_spi.c, since that code also constructs an SPIError but from C
and with more info available (edata->internalquery and
edata->internalpos). But making a tuple and assigning it to spidata is
in both. Not sure how this can be improved.

6. __init__ can use METH_VARARGS | METH_KEYWORDS in both Python2 and
3, no need for the #if

7. "could not create dictionary for SPI exception" would be more
precise as "could not create dictionary for SPIError" right?

8. in PLy_add_methods_to_dictionary I would avoid import since it
makes one think of importing modules; maybe "could not create function
wrapper for \"%s\"", "could not create method wrapper for \"%s\""

9. also in PLy_add_methods_to_dictionary "could public method \"%s\"
in dictionary" is not proper English and is missing not, maybe "could
not add method \"%s\" to class dictionary"?

10. in PLy_add_methods_to_dictionary if PyCFunction_New succeeds but
PyMethod_New fails, func will leak

11. it would be nice to have a test for the invalid SQLSTATE code part

12. similar to 10, in PLy_spi_error__init__ taking the "invalid
SQLSTATE" branch leaks exc_args, in general early returns via PLy_elog
will leave the intermediate Python objects leaking

Will mark the patch as Waiting for author.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2015-11-02 17:01 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:

Hello,

Here's a detailed review:

1. in PLy_spi_error__init__ you need to check kw for NULL before doing
PyDict_Size(kw) otherwise for plpy.SPIError() you get Bad internal
call because PyDict_Size expects a real dictionary not NULL

PyDict_Size returns -1 when the dictionary is NULL

http://grokbase.com/t/python/python-dev/042ft6qaqq/pydict-size-error-return

done

2. a test with just plpy.SPIError() is still missing, this would have
caught 1.

please, can you write some example - I am not able raise described error

3. the tests have "possibility to set all accessable fields in custom
exception" above a test that doesn't set all fields, it's confusing
(and accessible is spelled wrong)

done

4. in the docs, "The constructor of SPIError exception (class)
supports keyword parameters." sounds better as "An SPIError instance
can be constructed using keyword parameters."

done

5. there is conceptual code duplication between PLy_spi_exception_set
in plpy_spi.c, since that code also constructs an SPIError but from C
and with more info available (edata->internalquery and
edata->internalpos). But making a tuple and assigning it to spidata is
in both. Not sure how this can be improved.

I see it, but I don't think, so current code should be changed.
PLy_spi_exception_set is controlled directly by fully filled ErrorData
structure, __init__ is based only on keyword parameters with possibility
use inherited data. If I'll build ErrorData in __init__ function and call
PLy_spi_exception_set, then the code will be longer and more complex.

6. __init__ can use METH_VARARGS | METH_KEYWORDS in both Python2 and
3, no need for the #if

done

7. "could not create dictionary for SPI exception" would be more
precise as "could not create dictionary for SPIError" right?

done

8. in PLy_add_methods_to_dictionary I would avoid import since it
makes one think of importing modules; maybe "could not create
functionwrapper for \"%s\"", "could not create method wrapper for \"%s\""

done

9. also in PLy_add_methods_to_dictionary "could public method \"%s\"
in dictionary" is not proper English and is missing not, maybe "could
not add method \"%s\" to class dictionary"?

done

10. in PLy_add_methods_to_dictionary if PyCFunction_New succeeds but
PyMethod_New fails, func will leak

done

11. it would be nice to have a test for the invalid SQLSTATE code part

done

12. similar to 10, in PLy_spi_error__init__ taking the "invalid
SQLSTATE" branch leaks exc_args, in general early returns via PLy_elog
will leave the intermediate Python objects leaking

dome

Will mark the patch as Waiting for author.

attached new update

Regards

Pavel

On Tue, Nov 3, 2015 at 12:49 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

1. in PLy_spi_error__init__ you need to check kw for NULL before doing
PyDict_Size(kw) otherwise for plpy.SPIError() you get Bad internal
call because PyDict_Size expects a real dictionary not NULL

PyDict_Size returns -1 when the dictionary is NULL

http://grokbase.com/t/python/python-dev/042ft6qaqq/pydict-size-error-return

Yes, but it also sets the error indicator to BadInternalCall. In 2.7
the code is:
Py_ssize_t
PyDict_Size(PyObject *mp)
{
if (mp == NULL || !PyDict_Check(mp)) {
PyErr_BadInternalCall();
return -1;
}
return ((PyDictObject *)mp)->ma_used;
}

I had a PLy_elog right after the PyDict_Size call for debugging and
that one was raising BadInternalCall since it checked the error
indicator. So the NULL check is needed.

2. a test with just plpy.SPIError() is still missing, this would have
caught 1.

please, can you write some example - I am not able raise described error

The example was plpy.SPIError() but I now realize that, in order to
see a failure, you need the extra PLy_elog which I had in there.
But this basic form of the constructor is still an important thing to
test so please add this as well to the regression test.

5. there is conceptual code duplication between PLy_spi_exception_set
in plpy_spi.c, since that code also constructs an SPIError but from C
and with more info available (edata->internalquery and
edata->internalpos). But making a tuple and assigning it to spidata is
in both. Not sure how this can be improved.

I see it, but I don't think, so current code should be changed.
PLy_spi_exception_set is controlled directly by fully filled ErrorData
structure, __init__ is based only on keyword parameters with possibility use
inherited data. If I'll build ErrorData in __init__ function and call
PLy_spi_exception_set, then the code will be longer and more complex.

Indeed, I don't really see how to improve this but it does bug me a bit.

One more thing,
+    The <literal>plpy</literal> module provides several possibilities to
+    to raise a exception:

This has "to" 2 times and is weird since it says it offers several
possibilities but then shows only one (the SPIError constructor).
And SPIError should be <literal>plpy.SPIError</literal> everywhere to
be consistent.

Maybe something like (needs markup):
A plpy.SPIError can be raised from PL/Python, the constructor accepts
keyword parameters:
plpy.SPIError([ message [, detail [, hint [, sqlstate [, schema [,
table [, column [, datatype [, constraint ]]]]]]]]])
then the example

If you fix the doc and add the plpy.SPIError() test I'm happy. I'll
give it one more test on Python2.7 and 3.5 and mark it Ready for
Committer.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2015-11-03 17:13 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:

On Tue, Nov 3, 2015 at 12:49 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

1. in PLy_spi_error__init__ you need to check kw for NULL before doing
PyDict_Size(kw) otherwise for plpy.SPIError() you get Bad internal
call because PyDict_Size expects a real dictionary not NULL

PyDict_Size returns -1 when the dictionary is NULL

http://grokbase.com/t/python/python-dev/042ft6qaqq/pydict-size-error-return

Yes, but it also sets the error indicator to BadInternalCall. In 2.7
the code is:
Py_ssize_t
PyDict_Size(PyObject *mp)
{
if (mp == NULL || !PyDict_Check(mp)) {
PyErr_BadInternalCall();
return -1;
}
return ((PyDictObject *)mp)->ma_used;
}

I had a PLy_elog right after the PyDict_Size call for debugging and
that one was raising BadInternalCall since it checked the error
indicator. So the NULL check is needed.

I did it in last patch - PyDict_Size is not called when kw is NULL

2. a test with just plpy.SPIError() is still missing, this would have
caught 1.

one test contains "x = plpy.SPIError()". Is it, what you want?

please, can you write some example - I am not able raise described error

The example was plpy.SPIError() but I now realize that, in order to
see a failure, you need the extra PLy_elog which I had in there.
But this basic form of the constructor is still an important thing to
test so please add this as well to the regression test.

5. there is conceptual code duplication between PLy_spi_exception_set
in plpy_spi.c, since that code also constructs an SPIError but from C
and with more info available (edata->internalquery and
edata->internalpos). But making a tuple and assigning it to spidata is
in both. Not sure how this can be improved.

I see it, but I don't think, so current code should be changed.
PLy_spi_exception_set is controlled directly by fully filled ErrorData
structure, __init__ is based only on keyword parameters with possibility

use

inherited data. If I'll build ErrorData in __init__ function and call
PLy_spi_exception_set, then the code will be longer and more complex.

Indeed, I don't really see how to improve this but it does bug me a bit.

One more thing,
+    The <literal>plpy</literal> module provides several possibilities to
+    to raise a exception:

This has "to" 2 times and is weird since it says it offers several
possibilities but then shows only one (the SPIError constructor).
And SPIError should be <literal>plpy.SPIError</literal> everywhere to
be consistent.

I'll do it tomorrow

Maybe something like (needs markup):
A plpy.SPIError can be raised from PL/Python, the constructor accepts
keyword parameters:
plpy.SPIError([ message [, detail [, hint [, sqlstate [, schema [,
table [, column [, datatype [, constraint ]]]]]]]]])
then the example

If you fix the doc and add the plpy.SPIError() test I'm happy. I'll
give it one more test on Python2.7 and 3.5 and mark it Ready for
Committer.

Regards

Pavel

Sorry, you're right, I didn't notice the x = plpy.SPIError() test.

I did notice that you included the kw != NULL, I was explaining why it
really is needed even though it *seems* the code also works without
it.

There's just the doc part left then.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi

2015-11-04 7:06 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:

Sorry, you're right, I didn't notice the x = plpy.SPIError() test.

I did notice that you included the kw != NULL, I was explaining why it
really is needed even though it *seems* the code also works without

it.

It helped me lot of, thank you

There's just the doc part left then.

done

Regards

Pavel

On Wed, Nov 4, 2015 at 10:12 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

It helped me lot of, thank you

Welcome, I learned quite some from the process as well.

There's just the doc part left then.

done

We're almost there but not quite.

There's still a typo in the docs: excation.

A plpy.SPIError can be raised should be A
<literal>plpy.SPIError</literal> can be raised right?

And most importantly, for Python 3.5 there is a plpython_error_5.out
which is needed because of an alternative exception message in that
version. You didn't update this file, this makes the tests fail on
Python3.5.

Since you might not have Python 3.5 easily available I've attached a
patch to plpython_error_5.out which makes the tests pass, you can fold
this into your patch.