Обсуждение: proposal: PL/Pythonu - function ereport
Hi
We cannot to raise PostgreSQL exception with setting all possible fields. I propose new function 2015-10-08 12:11 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:
plpy.ereport(plpy.ERROR, 'some message', sqlstate = 'zx243');plpy.ereport(plpy.NOTICE, 'some message', 'some detai')Examples:The implementation will be based on keyword parameters, so only required parameters should be used.plpy.ereport(level, [ message [, detail [, hint [, sqlstate, ... ]]]])HiWe cannot to raise PostgreSQL exception with setting all possible fields. I propose new function
patch attached
regards
Pavel
PavelRegardsComments, notices, objections?
Вложения
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.
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
Regards
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
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.
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.
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
2015-10-16 8:12 GMT+02:00 Craig Ringer <craig@2ndquadrant.com>:
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
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
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
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
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
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
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Вложения
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
Any help is welcomeRegardsPavel
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Вложения
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.
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.
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.
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
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.
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.
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.
Вложения
2015-11-05 7:24 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
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?
fixed
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.
this fix will be pretty hard - if I'll fix for 3.5, then any other will be broken
I can move these tests to separate file and run some tests for 3.5 and other for older. But it is pretty ugly - and we have not any similar workaround elsewhere.
I checked the diff and looks so only language identifiers are different
7c7
< +$$ LANGUAGE plpython3u;
---
> +$$ LANGUAGE plpythonu;
26c26
< +$$ LANGUAGE plpython3u;
---
> +$$ LANGUAGE plpythonu;
43c43
< +$$ LANGUAGE plpython3u;
---
> +$$ LANGUAGE plpythonu;
52c52
< +$$ LANGUAGE plpython3u;
---
> +$$ LANGUAGE plpythonu;
60c60
< +$$ LANGUAGE plpython3u;
---
> +$$ LANGUAGE plpythonu;
7c7
< +$$ LANGUAGE plpython3u;
---
> +$$ LANGUAGE plpythonu;
26c26
< +$$ LANGUAGE plpython3u;
---
> +$$ LANGUAGE plpythonu;
43c43
< +$$ LANGUAGE plpython3u;
---
> +$$ LANGUAGE plpythonu;
52c52
< +$$ LANGUAGE plpython3u;
---
> +$$ LANGUAGE plpythonu;
60c60
< +$$ LANGUAGE plpython3u;
---
> +$$ LANGUAGE plpythonu;
It is strange - I cannot to understand how is possible so other Python's tests are working in your comp. I don't know where is the core of this issue, but I am inclined to think so some wrong is in your environment. The identifier plpython3u shouldn't be used in tests.
Regards
Pavel
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.
Вложения
2015-11-05 7:24 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
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.
I needed to understand the support for Python 3.5.
The patch with the fix is attached regress tests 3.5 Python
Regards
Pavel
Вложения
On Mon, Nov 9, 2015 at 2:58 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > I needed to understand the support for Python 3.5. > > The patch with the fix is attached regress tests 3.5 Python I wanted to type a reply this morning to explain and then I noticed there's another file (_0.out) for Python2.4 and older (as explained by src/pl/plpython/expected/README) so tests there fail as well. I built Python 2.4.6 and then Postgres against it and updated the expected output for 2.4 as well. Find the changes for 2.4 attached. You can add those to your patch. While I was at it I tested 2.5 and 2.6 as well, they also pass all tests. So with the 2.4 changes attached I think we're done. BTW, the alternative output files for pg_regress are explained at http://www.postgresql.org/docs/devel/static/regress-variant.html
Вложения
2015-11-09 16:46 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
On Mon, Nov 9, 2015 at 2:58 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> I needed to understand the support for Python 3.5.
>
> The patch with the fix is attached regress tests 3.5 Python
I wanted to type a reply this morning to explain and then I noticed
there's another file (_0.out) for Python2.4 and older (as explained by
src/pl/plpython/expected/README) so tests there fail as well. I built
Python 2.4.6 and then Postgres against it and updated the expected
output for 2.4 as well. Find the changes for 2.4 attached. You can add
those to your patch.
I forgot it
While I was at it I tested 2.5 and 2.6 as well, they also pass all
tests. So with the 2.4 changes attached I think we're done.
merged your patch
BTW, the alternative output files for pg_regress are explained at
http://www.postgresql.org/docs/devel/static/regress-variant.html
I didn't know it - nice, maybe I use it in orafce.
Thank you
Regards
Pavel
Вложения
On Mon, Nov 9, 2015 at 6:31 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > merged your patch So, I just that tested version 08 is the same as the previous patch + my change and I already tested that on Python 2.4, 2.5, 2.6, 2.7 and 3.5. All previous comments addressed so I'll mark this Ready for Committer. Thanks Pavel
2015-11-09 19:21 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
On Mon, Nov 9, 2015 at 6:31 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> merged your patch
So, I just that tested version 08 is the same as the previous patch +
my change and I already tested that on Python 2.4, 2.5, 2.6, 2.7 and
3.5.
All previous comments addressed so I'll mark this Ready for Committer.
Thank you very much for help
Regards
Pavel
Thanks Pavel
I don't think it's right to reuse SPIError for this. SPIError is clearly meant to signal an error in the SPI calls. Of course, we can't stop users from raising whatever exception they want, but if we're going to advertise that users can raise exceptions, then we should create separate exception classes. I suppose the proper way to set this up would be to create a base class like plpy.Error and derive SPIError from that.
2015-11-16 5:20 GMT+01:00 Peter Eisentraut <peter_e@gmx.net>:
I don't think it's right to reuse SPIError for this. SPIError is
clearly meant to signal an error in the SPI calls. Of course, we can't
stop users from raising whatever exception they want, but if we're going
to advertise that users can raise exceptions, then we should create
separate exception classes.
I suppose the proper way to set this up would be to create a base class
like plpy.Error and derive SPIError from that.
Do you have some ideas about the name of this class?
Regards
Pavel
On 11/15/15 11:29 PM, Pavel Stehule wrote: > > > 2015-11-16 5:20 GMT+01:00 Peter Eisentraut <peter_e@gmx.net > <mailto:peter_e@gmx.net>>: > > I don't think it's right to reuse SPIError for this. SPIError is > clearly meant to signal an error in the SPI calls. Of course, we can't > stop users from raising whatever exception they want, but if we're going > to advertise that users can raise exceptions, then we should create > separate exception classes. > > I suppose the proper way to set this up would be to create a base class > like plpy.Error and derive SPIError from that. > > > Do you have some ideas about the name of this class? I think plpy.Error is fine.
Is this patch in 'Waiting on Author' state actually? >> I don't think it's right to reuse SPIError for this. SPIError is >> clearly meant to signal an error in the SPI calls. Of course, we can't >> stop users from raising whatever exception they want, but if we're going >> to advertise that users can raise exceptions, then we should create >> separate exception classes. >> >> I suppose the proper way to set this up would be to create a base class >> like plpy.Error and derive SPIError from that. >> >> >> Do you have some ideas about the name of this class? > > I think plpy.Error is fine. > > > -- Teodor Sigaev E-mail: teodor@sigaev.ru WWW: http://www.sigaev.ru/
2015-11-27 17:54 GMT+01:00 Teodor Sigaev <teodor@sigaev.ru>:
Is this patch in 'Waiting on Author' state actually?
yes
I'll try redesign patch by Peter's proposal
Pavel
--I don't think it's right to reuse SPIError for this. SPIError is
clearly meant to signal an error in the SPI calls. Of course, we can't
stop users from raising whatever exception they want, but if we're going
to advertise that users can raise exceptions, then we should create
separate exception classes.
I suppose the proper way to set this up would be to create a base class
like plpy.Error and derive SPIError from that.
Do you have some ideas about the name of this class?
I think plpy.Error is fine.
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/
On Sat, Nov 28, 2015 at 1:57 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > > > 2015-11-27 17:54 GMT+01:00 Teodor Sigaev <teodor@sigaev.ru>: >> >> Is this patch in 'Waiting on Author' state actually? I am marking it as returned with feedback for this CF then. -- Michael
2015-11-28 13:11 GMT+01:00 Michael Paquier <michael.paquier@gmail.com>:
On Sat, Nov 28, 2015 at 1:57 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> 2015-11-27 17:54 GMT+01:00 Teodor Sigaev <teodor@sigaev.ru>:
>>
>> Is this patch in 'Waiting on Author' state actually?
I am marking it as returned with feedback for this CF then.
please, can revert it?
I though so Peter request will require some massive change, but I had to change few lines.
So this patch is still ready for review - minimally for Peter's review.
Regards
Pavel
--
Michael
Hi
2015-11-16 23:40 GMT+01:00 Peter Eisentraut <peter_e@gmx.net>:
On 11/15/15 11:29 PM, Pavel Stehule wrote:
>
>
> 2015-11-16 5:20 GMT+01:00 Peter Eisentraut <peter_e@gmx.net
> <mailto:peter_e@gmx.net>>:
>
> I don't think it's right to reuse SPIError for this. SPIError is
> clearly meant to signal an error in the SPI calls. Of course, we can't
> stop users from raising whatever exception they want, but if we're going
> to advertise that users can raise exceptions, then we should create
> separate exception classes.
>
> I suppose the proper way to set this up would be to create a base class
> like plpy.Error and derive SPIError from that.
>
>
> Do you have some ideas about the name of this class?
I think plpy.Error is fine.
here is updated patch - work with 2.x Python.
I have 3.x Python broken on my fedora, so I should not do tests on 3.x.
Regards
Pavel
Вложения
On Sun, Nov 29, 2015 at 4:00 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
2015-11-28 13:11 GMT+01:00 Michael Paquier <michael.paquier@gmail.com>:On Sat, Nov 28, 2015 at 1:57 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> 2015-11-27 17:54 GMT+01:00 Teodor Sigaev <teodor@sigaev.ru>:
>>
>> Is this patch in 'Waiting on Author' state actually?
I am marking it as returned with feedback for this CF then.please, can revert it?
I have moved it to the next CF instead as you are still working on it. It is going to be time to finish the current CF soon.
--
--
Michael
2015-11-29 13:42 GMT+01:00 Michael Paquier <michael.paquier@gmail.com>:
On Sun, Nov 29, 2015 at 4:00 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:2015-11-28 13:11 GMT+01:00 Michael Paquier <michael.paquier@gmail.com>:On Sat, Nov 28, 2015 at 1:57 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> 2015-11-27 17:54 GMT+01:00 Teodor Sigaev <teodor@sigaev.ru>:
>>
>> Is this patch in 'Waiting on Author' state actually?
I am marking it as returned with feedback for this CF then.please, can revert it?I have moved it to the next CF instead as you are still working on it. It is going to be time to finish the current CF soon.
ook
Regards
Pavel
--Michael
Hi
>
> Do you have some ideas about the name of this class?
I think plpy.Error is fine.here is updated patch - work with 2.x Python.I have 3.x Python broken on my fedora, so I should not do tests on 3.x.
here is complete patch - regress tests for all supported Python branches
RegardsPavel
Вложения
On Tue, Dec 1, 2015 at 2:17 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > here is complete patch - regress tests for all supported Python branches I had a look at what changed in v10 since my last reviewed version and indeed most of it is straightforward: renames from SPIError to Error. The patch also changes plpy.Fatal and plpy.SPIError to inherit from the existing plpy.Error which is a backward incompatibility: catching a plpy.Error will now also catch SPIError and Fatal. But by doing this I noticed plpy.Error already existed without the patch and raising plpy.Error(msg) is documented as equivalent to calling plpy.error(msg), similar for plpy.Fatal and plpy.fatal(). This patch makes it possible to raise plpy.Error with more arguments including keyword ones but doesn't change plpy.error(msg). And Fatal is not touched so it becomes inconsistent with Error. So I now think the approach this patch took is wrong. We should instead enhance the existing error and fatal functions and Error and Fatal exceptions to accept other arguments that ereport accepts (hint, sqlstate) and be able to pass all those as keyword parameters. SPIError should be left unchanged as that's for errors raised by query execution not by the PL/Python user. This is also close to Pavel's original ereport proposal but by extending existing functionality instead of adding a new function and it covers Peter's observation that in Python the ereport function should be "raise an exception" as that's already an alternative way of doing it.
<div dir="ltr"><br /><div class="gmail_extra"><br /><div class="gmail_quote">2015-12-03 7:04 GMT+01:00 Catalin Iacob <spandir="ltr"><<a href="mailto:iacobcatalin@gmail.com" target="_blank">iacobcatalin@gmail.com</a>></span>:<br /><blockquoteclass="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><spanclass="">On Tue, Dec 1, 2015 at 2:17 AM, Pavel Stehule <<a href="mailto:pavel.stehule@gmail.com">pavel.stehule@gmail.com</a>>wrote:<br /> > here is complete patch - regress testsfor all supported Python branches<br /><br /></span>I had a look at what changed in v10 since my last reviewed versionand<br /> indeed most of it is straightforward: renames from SPIError to Error.<br /> The patch also changes plpy.Fataland plpy.SPIError to inherit from<br /> the existing plpy.Error which is a backward incompatibility: catching<br/> a plpy.Error will now also catch SPIError and Fatal.<br /><br /> But by doing this I noticed plpy.Error alreadyexisted without the<br /> patch and raising plpy.Error(msg) is documented as equivalent to<br /> calling plpy.error(msg),similar for plpy.Fatal and plpy.fatal(). This<br /> patch makes it possible to raise plpy.Error with morearguments<br /> including keyword ones but doesn't change plpy.error(msg). And Fatal<br /> is not touched so it becomesinconsistent with Error.<br /><br /> So I now think the approach this patch took is wrong. We should<br /> insteadenhance the existing error and fatal functions and Error and<br /> Fatal exceptions to accept other arguments thatereport accepts (hint,<br /> sqlstate) and be able to pass all those as keyword parameters.<br /> SPIError should beleft unchanged as that's for errors raised by query<br /> execution not by the PL/Python user. This is also close to Pavel's<br/> original ereport proposal but by extending existing functionality<br /> instead of adding a new function andit covers Peter's observation<br /> that in Python the ereport function should be "raise an exception" as<br /> that'salready an alternative way of doing it.<br /></blockquote></div><br /><br /></div><div class="gmail_extra">I am sorry,I don't understand. Now due inheritance plpy.Fatal and plpy.SPIError has availability to use keyword parameters.<br/><br />postgres=# do $$ raise plpy.Fatal('AHOJ','NAZDAR');<br />$$ language plpythonu;<br />FATAL: 38000:plpy.Fatal: AHOJ<br />DETAIL: NAZDAR<br />CONTEXT: Traceback (most recent call last):<br /> PL/Python anonymouscode block, line 1, in <module><br /> raise plpy.Fatal('AHOJ','NAZDAR');<br />PL/Python anonymous codeblock<br /><br /></div><div class="gmail_extra">Regards<br /><br /></div><div class="gmail_extra">Pavel<br /></div></div>
On Thu, Dec 3, 2015 at 7:58 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > I am sorry, I don't understand. Now due inheritance plpy.Fatal and > plpy.SPIError has availability to use keyword parameters. Indeed, I didn't realize this, but I don't think it changes the main argument. What I think should happen: 1. Error should take keyword arguments 2. same for Fatal 3. Fatal should not be changed to inherit from Error - it should stay like it is now, just another exception class You can argue a Fatal error is an Error but these classes already exist and are independent, changing their relationship is backward incompatible. 4. SPIError should not be changed at all It's for errors raised by query execution not user PL/Python code so doing raise SPIError in PL/Python doesn't make sense. And again, changing the inheritance relationship of these existing classes changes meaning of existing code that catches the exceptions. 5. all the reporting functions: plpy.debug...plpy.fatal functions should also be changed to allow more arguments than the message and allow them as keyword arguments They are Python wrappers for ereport and this would make them similar in capabilities to the PL/pgSQL RAISE This will make plpy.error(...) stay equivalent in capabilities with raise plpy.Error(...), same for fatal and Fatal 6. the docs should move to the "Utility Functions" section There, it's already described how to raise errors either viathe exceptions or the utility functions. I think the above doesn't break anything, doesn't confuse user exceptions with backend SPIError exceptions, enhances error reporting features for the PL/Python user to bring them up to par with ereport and PL/pgSQL RAISE and solve your initial use case at the top of the thread (but with slightly different spelling to match what already exists in PL/Python): "We cannot to raise PostgreSQL exception with setting all possible fields. I propose new function plpy.ereport(level, [ message [, detail [, hint [, sqlstate, ... ]]]])" Is what I mean more clear now? Do you (and others) agree?
2015-12-03 16:57 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
On Thu, Dec 3, 2015 at 7:58 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> I am sorry, I don't understand. Now due inheritance plpy.Fatal and
> plpy.SPIError has availability to use keyword parameters.
Indeed, I didn't realize this, but I don't think it changes the main argument.
What I think should happen:
1. Error should take keyword arguments
2. same for Fatal
understand
3. Fatal should not be changed to inherit from Error - it should stay
like it is now, just another exception class
You can argue a Fatal error is an Error but these classes already
exist and are independent, changing their relationship is backward
incompatible.
Don't understand - if Fatal has same behave as Error, then why it cannot be inherited from Error?
What can be broken?
4. SPIError should not be changed at all
It's for errors raised by query execution not user PL/Python code
so doing raise SPIError in PL/Python doesn't make sense.
And again, changing the inheritance relationship of these
existing classes changes meaning of existing code that catches the
exceptions.
can be
5. all the reporting functions: plpy.debug...plpy.fatal functions
should also be changed to allow more arguments than the message and
allow them as keyword arguments
this is maybe bigger source of broken compatibility
lot of people uses plpy.debug(var1, var2, var3)
rich constructor use $1 for message, $2 for detail, $3 for hint. So it was a reason, why didn't touch these functions
They are Python wrappers for ereport and this would make them
similar in capabilities to the PL/pgSQL RAISE
This will make plpy.error(...) stay equivalent in capabilities
with raise plpy.Error(...), same for fatal and Fatal
6. the docs should move to the "Utility Functions" section
There, it's already described how to raise errors either via the
exceptions or the utility functions.
I think the above doesn't break anything, doesn't confuse user
exceptions with backend SPIError exceptions, enhances error reporting
features for the PL/Python user to bring them up to par with ereport
and PL/pgSQL RAISE and solve your initial use case at the top of the
thread (but with slightly different spelling to match what already
exists in PL/Python):
"We cannot to raise PostgreSQL exception with setting all possible
fields. I propose new function
plpy.ereport(level, [ message [, detail [, hint [, sqlstate, ... ]]]])"
I am not against to plpy.ereport function - it can live together with rich exceptions class, and users can use what they prefer.
It is few lines more
Is what I mean more clear now? Do you (and others) agree?
not too much :)
Regards
Pavel
On Thu, Dec 3, 2015 at 6:54 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > Don't understand - if Fatal has same behave as Error, then why it cannot be > inherited from Error? > > What can be broken? Existing code that did "except plpy.Error as exc" will now also be called if plpy.Fatal is raised. That wasn't the case so this changes meaning of existing code, therefore it introduces an incompatibility. >> >> 5. all the reporting functions: plpy.debug...plpy.fatal functions >> should also be changed to allow more arguments than the message and >> allow them as keyword arguments > > > this is maybe bigger source of broken compatibility > > lot of people uses plpy.debug(var1, var2, var3) > > rich constructor use $1 for message, $2 for detail, $3 for hint. So it was a > reason, why didn't touch these functions No, if you use PyArg_ParseTupleAndKeywords you'll have Python accept all of these so previous ways are still accepted: plpy.error('a_msg', 'a_detail', 'a_hint') plpy.error'a_msg', 'a_detail', hint='a_hint') plpy.error('a_msg', detail='a_detail', hint='a_hint') plpy.error(msg='a_msg', detail='a_detail', hint='a_hint') plpy.error('a_msg') plpy.error(msg='a_msg') etc. But I now see that even though the docs say plpy.error and friends take a single msg argument, they actually allow any number of arguments. If multiple arguments are passed they are converted to a tuple and the string representation of that tuple goes into ereport/plpy.Error: CREATE FUNCTION test() RETURNS int AS $$ try: plpy.error('an error message', 'detail for error', 'hint for error') except plpy.Error as exc: plpy.info('haveexc {}'.format(exc)) plpy.info('have exc.args |{}| of type {}'.format(exc.args, type(exc.args))) $$ LANGUAGE plpython3u; SELECT test(); [catalin@archie pg]$ psql -f plpy test CREATE FUNCTION psql:plpy:13: INFO: have exc ('an error message', 'detail for error', 'hint for error') psql:plpy:13: INFO: have exc.args |("('an error message', 'detail for error', 'hint for error')",)| of type <class 'tuple'> In the last line note that exc.args (the args tuple passed in the constructor of plpy.Error) is a tuple with a single element which is a string which is a representation of the tuple passed into plpy.error. Don't know why this was thought useful, it was certainly surprising to me. Anyway, the separate $2, $3 etc are currently not detail and hint, they're just more stuff that gets appended to this tuple. They don't get passed to clients as hints. And you can pass lots of them not just detail and hint. My proposal would change the meaning of this to actually interpret the second argument as detail, third as hint and to only allow a limited number (the ones with meaning to ereport). The hint would go to ereport so it would be a real hint: it would go to clients as HINT and so on. I think that's way more useful that what is done now. But I now see my proposal is also backward incompatible. > I am not against to plpy.ereport function - it can live together with rich > exceptions class, and users can use what they prefer. I wasn't suggesting introducing ereport, I was saying the existing functions and exceptions are very similar to your proposed ereport. Enhancing them to take keyword arguments would make them a bit more powerful. Adding ereport would be another way of doing the same thing so that's more confusing than useful. All in all it's hard to improve this cleanly. I'm still not sure the latest patch is a good idea but now I'm also not sure what I proposed is better than leaving it as it is.
Hi
2015-12-08 7:06 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
On Thu, Dec 3, 2015 at 6:54 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Don't understand - if Fatal has same behave as Error, then why it cannot be
> inherited from Error?
>
> What can be broken?
Existing code that did "except plpy.Error as exc" will now also be
called if plpy.Fatal is raised. That wasn't the case so this changes
meaning of existing code, therefore it introduces an incompatibility.
yes, there should be some common ancestor for plpy.Error and plpy.Fatal
Have you any idea, how this ancestor should be named?
>>
>> 5. all the reporting functions: plpy.debug...plpy.fatal functions
>> should also be changed to allow more arguments than the message and
>> allow them as keyword arguments
>
>
> this is maybe bigger source of broken compatibility
>
> lot of people uses plpy.debug(var1, var2, var3)
>
> rich constructor use $1 for message, $2 for detail, $3 for hint. So it was a
> reason, why didn't touch these functions
No, if you use PyArg_ParseTupleAndKeywords you'll have Python accept
all of these so previous ways are still accepted:
plpy.error('a_msg', 'a_detail', 'a_hint')
plpy.error'a_msg', 'a_detail', hint='a_hint')
plpy.error('a_msg', detail='a_detail', hint='a_hint')
plpy.error(msg='a_msg', detail='a_detail', hint='a_hint')
plpy.error('a_msg')
plpy.error(msg='a_msg')
etc.
But I now see that even though the docs say plpy.error and friends
take a single msg argument, they actually allow any number of
arguments. If multiple arguments are passed they are converted to a
tuple and the string representation of that tuple goes into
ereport/plpy.Error:
CREATE FUNCTION test()
RETURNS int
AS $$
try:
plpy.error('an error message', 'detail for error', 'hint for error')
except plpy.Error as exc:
plpy.info('have exc {}'.format(exc))
plpy.info('have exc.args |{}| of type {}'.format(exc.args, type(exc.args)))
$$ LANGUAGE plpython3u;
SELECT test();
[catalin@archie pg]$ psql -f plpy test
CREATE FUNCTION
psql:plpy:13: INFO: have exc ('an error message', 'detail for error',
'hint for error')
psql:plpy:13: INFO: have exc.args |("('an error message', 'detail for
error', 'hint for error')",)| of type <class 'tuple'>
In the last line note that exc.args (the args tuple passed in the
constructor of plpy.Error) is a tuple with a single element which is a
string which is a representation of the tuple passed into plpy.error.
Don't know why this was thought useful, it was certainly surprising to
me. Anyway, the separate $2, $3 etc are currently not detail and hint,
they're just more stuff that gets appended to this tuple. They don't
get passed to clients as hints. And you can pass lots of them not just
detail and hint.
using tuple is less work for you, you don't need to concat more values into one. I don't know, how often this technique is used - probably it has sense only for NOTICE and lower levels - for adhoc debug messages. Probably not used elsewhere massively.
My proposal would change the meaning of this to actually interpret the
second argument as detail, third as hint and to only allow a limited
number (the ones with meaning to ereport). The hint would go to
ereport so it would be a real hint: it would go to clients as HINT and
so on. I think that's way more useful that what is done now. But I now
see my proposal is also backward incompatible.
It was my point
> I am not against to plpy.ereport function - it can live together with rich
> exceptions class, and users can use what they prefer.
I wasn't suggesting introducing ereport, I was saying the existing
functions and exceptions are very similar to your proposed ereport.
Enhancing them to take keyword arguments would make them a bit more
powerful. Adding ereport would be another way of doing the same thing
so that's more confusing than useful.
ok
All in all it's hard to improve this cleanly. I'm still not sure the
latest patch is a good idea but now I'm also not sure what I proposed
is better than leaving it as it is.
We can use different names, we should not to implement all changes at once.
My main target is possibility to raise rich exception instead dirty workaround. Changing current functions isn't necessary - although if we are changing API, is better to do it once.
Regards
Pavel
Hi
2015-12-08 7:06 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
On Thu, Dec 3, 2015 at 6:54 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Don't understand - if Fatal has same behave as Error, then why it cannot be
> inherited from Error?
>
> What can be broken?
Existing code that did "except plpy.Error as exc" will now also be
called if plpy.Fatal is raised. That wasn't the case so this changes
meaning of existing code, therefore it introduces an incompatibility.
I read some notes about Python naming convention
and we can use different names for new exception classes
* common ancestor - plpy.BaseError
* descendants - plpy.ExecError, plpy.SPIError, plpy.FatalError
It should to decrease compatibility issues to minimum. SPIError was used mostly for error trapping (see doc).
Is it ok for you?
Regards
Pavel
Hi
here is new version.Вложения
shellOn Thu, Dec 31, 2015 at 11:37 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > here is new version. And here's a new review, sorry for the delay. > Now I use a common ancestor "plpy.BaseError" for plpy builtin classes. So > plpy.SPIError isn't descendant of plpy.Error and then there are not possible > incompatibility issues. That's good. > Instead modification builtin function plpy.debug, plpy.info, ... and > introduction incompatibility I wrote new set of functions with keyword > parameters (used mainly for elevel < ERROR): > > plpy.raise_debug, plpy.raise_info, plpy.raise_notice, plpy.raise_warning, > plpy.raise_error and plpy.raise_fatal. I'm on the fence whether these are good ideas. On one hand having them is nice and they avoid changing the existing plpy.debug etc. in backward incompatible ways, on the other hand they're similar to those so it can be confusing to know which ones to pick. Any opinions from others on whether it's better to add these or not? The docs need more work, especially if raise_* are kept, as the docs should then clearly explain the differences between the 2 sets and nudge users toward the new raise_* functions. I can help with that though if there are objections to these functions I wouldn't mind hearing it before I document them. As for the code: 1. there are quite some out of date comments referring to SPIError in places that now handle more types (like /* prepare dictionary with __init__ method for SPIError class */ in plpy_plpymodule.c) 2. you moved PLy_spi_exception_set from plpy_spi.c to plpy_elog.c and renamed it to PLy_base_exception_set but it's still spi specific: it still sets an attribute called spidata. You needed this because you call it in PLy_output_kw but can't you instead make PLy_output_kw similar to PLy_output and just call PLy_exception_set in the PG_CATCH block like PLy_output does? With the patch plpy.error(msg) will raise an error object without an spidata attribute but plpy.raise_error(msg) will raise an error with an spidata attribute. 3. PLy_error__init__ is used for BaseError but always sets an attribute called spidata, I would expect that to only happen for SPIError not for BaseError. You'll need to pick some other way of attaching the error details to BaseError instances. Similarly, PLy_get_spi_error_data became PLy_get_error_data and it's invoked on other classes than SPIError but it still always looks at the spidata attribute. 4. it wouldn't hurt to expand the tests a bit to also raise plpy.Fatal with keyword arguments and maybe catch BaseError and inspect it a bit to see it contains reasonable values (per 4. have spidata when raising an SPIError but not when raising an Error or BaseError or Fatal etc.) As seen from 1, 2, and 3 the patch is still too much about SPIError. As I see it, it should only touch SPIError to make it inherit from the new BaseError but for the rest, the patch shouldn't change it and shouldn't propagate spidata attributes and SPIError comments. As it stands, the patch has historical artifacts that show it was initially a patch for SPIError but it's now a patch for error reporting so those should go away. I'll set the patch back to waiting for author.
2016-01-08 7:31 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
shellOn Thu, Dec 31, 2015 at 11:37 AM, Pavel Stehule
<pavel.stehule@gmail.com> wrote:
> here is new version.
And here's a new review, sorry for the delay.
> Now I use a common ancestor "plpy.BaseError" for plpy builtin classes. So
> plpy.SPIError isn't descendant of plpy.Error and then there are not possible
> incompatibility issues.
That's good.
> Instead modification builtin function plpy.debug, plpy.info, ... and
> introduction incompatibility I wrote new set of functions with keyword
> parameters (used mainly for elevel < ERROR):
>
> plpy.raise_debug, plpy.raise_info, plpy.raise_notice, plpy.raise_warning,
> plpy.raise_error and plpy.raise_fatal.
I'm on the fence whether these are good ideas. On one hand having them
is nice and they avoid changing the existing plpy.debug etc. in
backward incompatible ways, on the other hand they're similar to those
so it can be confusing to know which ones to pick. Any opinions from
others on whether it's better to add these or not?
The docs need more work, especially if raise_* are kept, as the docs
should then clearly explain the differences between the 2 sets and
nudge users toward the new raise_* functions. I can help with that
though if there are objections to these functions I wouldn't mind
hearing it before I document them.
ok, we will wait.
As for the code:
1. there are quite some out of date comments referring to SPIError in
places that now handle more types (like /* prepare dictionary with
__init__ method for SPIError class */ in plpy_plpymodule.c)
2. you moved PLy_spi_exception_set from plpy_spi.c to plpy_elog.c and
renamed it to PLy_base_exception_set but it's still spi specific: it
still sets an attribute called spidata. You needed this because you
call it in PLy_output_kw but can't you instead make PLy_output_kw
similar to PLy_output and just call PLy_exception_set in the PG_CATCH
block like PLy_output does? With the patch plpy.error(msg) will raise
an error object without an spidata attribute but plpy.raise_error(msg)
will raise an error with an spidata attribute.
3. PLy_error__init__ is used for BaseError but always sets an
attribute called spidata, I would expect that to only happen for
SPIError not for BaseError. You'll need to pick some other way of
attaching the error details to BaseError instances. Similarly,
PLy_get_spi_error_data became PLy_get_error_data and it's invoked on
other classes than SPIError but it still always looks at the spidata
attribute.
I afraid of compatibility issues - so I am not changing internal format simply. Some legacy application can be based on detection of spidata attribute. So I cannot to change this structure for SPIError.
I can change it for BaseError, but this is question. Internally BaseError and SPIError share same data. So it can be strange if BaseError and SPIError uses different internal formats.
I am interesting your opinion??
4. it wouldn't hurt to expand the tests a bit to also raise plpy.Fatal
with keyword arguments and maybe catch BaseError and inspect it a bit
to see it contains reasonable values (per 4. have spidata when raising
an SPIError but not when raising an Error or BaseError or Fatal etc.)
a Fatal cannnot be raised - it is a session end. Personally - support of Fatal level is wrong, and it should not be propagated to user level, but it is now. And then due consistency I wrote support for fatal level. But hard to test it.
As seen from 1, 2, and 3 the patch is still too much about SPIError.
As I see it, it should only touch SPIError to make it inherit from the
new BaseError but for the rest, the patch shouldn't change it and
shouldn't propagate spidata attributes and SPIError comments. As it
stands, the patch has historical artifacts that show it was initially
a patch for SPIError but it's now a patch for error reporting so those
should go away.
I'll set the patch back to waiting for author.
On Fri, Jan 8, 2016 at 7:56 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: >> 3. PLy_error__init__ is used for BaseError but always sets an >> attribute called spidata, I would expect that to only happen for >> SPIError not for BaseError. You'll need to pick some other way of >> attaching the error details to BaseError instances. Similarly, >> PLy_get_spi_error_data became PLy_get_error_data and it's invoked on >> other classes than SPIError but it still always looks at the spidata >> attribute. > > > I afraid of compatibility issues - so I am not changing internal format > simply. Some legacy application can be based on detection of spidata > attribute. So I cannot to change this structure for SPIError. Indeed, I wasn't suggesting changing it for SPIError, that needs to stay spidata. > I can change it for BaseError, but this is question. Internally BaseError > and SPIError share same data. So it can be strange if BaseError and SPIError > uses different internal formats. > > I am interesting your opinion?? Yes, it's not great if BaseError and SPIError have different ways but I think having BaseError have a member (spidata) named after one of its subclasses is even worse. I would say detail, hint etc belong to BaseError and I would make them different attributes of BaseError instances instead of one big BaseError.data tuple similar to what SPIError.spidata is now. SPIError would keep its spidata for compatibility but would get the same information into its detail, hint etc. attributes since it's derived from BaseError. So an equivalent to this Python (pseudo)code: # base class for all exceptions raised by PL/Python class BaseError: def __init__(self, msg, detail=None, hint=None, and so on for every accepted argument): self.msg = msg self.detail = detail # and so on class Error(BaseError): pass class Fatal(BaseError): pass class SPIError(BaseError): def __init__(self, msg): # call BaseError.__init__ so that SPIError also gets .msg, .detail and so on like all other BaseError instances # do what's done today to fill spidata to keep backward compatibility If you don't like that spidata duplicates the other fields, it's probably also ok to not make inherit from BaseError at all. I actually like this better. Then I would call the base class something like UserRaisedError (ugly name but can't think of something better to emphasize that it's about errors that the PL/Python developer is supposed to raise rather SPIError which is Postgres raising them) and it would be: # base class for exceptions raised by users in their PL/Python code class UserRaisedError: def __init__(self, msg, detail=None, hint=None, and so on for every accepted argument): self.msg = msg self.detail = detail # and so on class Error(UserRaisedError): pass class Fatal(UserRaisedError): pass # base class for exceptions raised by Postgres when executing sql code class SPIError: # no change to this class > > a Fatal cannnot be raised - it is a session end. Personally - support of > Fatal level is wrong, and it should not be propagated to user level, but it > is now. And then due consistency I wrote support for fatal level. But hard > to test it. > I see, then never mind.
2016-01-11 17:05 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
On Fri, Jan 8, 2016 at 7:56 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> 3. PLy_error__init__ is used for BaseError but always sets an
>> attribute called spidata, I would expect that to only happen for
>> SPIError not for BaseError. You'll need to pick some other way of
>> attaching the error details to BaseError instances. Similarly,
>> PLy_get_spi_error_data became PLy_get_error_data and it's invoked on
>> other classes than SPIError but it still always looks at the spidata
>> attribute.
>
>
> I afraid of compatibility issues - so I am not changing internal format
> simply. Some legacy application can be based on detection of spidata
> attribute. So I cannot to change this structure for SPIError.
Indeed, I wasn't suggesting changing it for SPIError, that needs to
stay spidata.
> I can change it for BaseError, but this is question. Internally BaseError
> and SPIError share same data. So it can be strange if BaseError and SPIError
> uses different internal formats.
>
> I am interesting your opinion??
Yes, it's not great if BaseError and SPIError have different ways but
I think having BaseError have a member (spidata) named after one of
its subclasses is even worse.
I would say detail, hint etc belong to BaseError and I would make them
different attributes of BaseError instances instead of one big
BaseError.data tuple similar to what SPIError.spidata is now. SPIError
would keep its spidata for compatibility but would get the same
information into its detail, hint etc. attributes since it's derived
from BaseError.
So an equivalent to this Python (pseudo)code:
# base class for all exceptions raised by PL/Python
class BaseError:
def __init__(self, msg, detail=None, hint=None, and so on for
every accepted argument):
self.msg = msg
self.detail = detail
# and so on
class Error(BaseError):
pass
class Fatal(BaseError):
pass
class SPIError(BaseError):
def __init__(self, msg):
# call BaseError.__init__ so that SPIError also gets .msg,
.detail and so on like all other BaseError instances
# do what's done today to fill spidata to keep backward compatibility
If you don't like that spidata duplicates the other fields, it's
probably also ok to not make inherit from BaseError at all. I actually
like this better. Then I would call the base class something like
UserRaisedError (ugly name but can't think of something better to
emphasize that it's about errors that the PL/Python developer is
supposed to raise rather SPIError which is Postgres raising them) and
it would be:
# base class for exceptions raised by users in their PL/Python code
class UserRaisedError:
def __init__(self, msg, detail=None, hint=None, and so on for
every accepted argument):
self.msg = msg
self.detail = detail
# and so on
class Error(UserRaisedError):
pass
class Fatal(UserRaisedError):
pass
# base class for exceptions raised by Postgres when executing sql code
class SPIError:
# no change to this class
I see it.
it looks like distinguish between Error and SPIError is wrong way. And I have any other ugly use case.
If I raise a Error from one PLPythonu function, then in other PLPython function I'll trap a SPIError exception, because the information about class was lost inside Postgres. And it should be pretty messy.
I have not information if any exception was User based or SPI based.
The differentiation between Error and SPIError is wrong, because there isn't any difference in reality. There are two ways.
1. break compatibility and SPIError replace by Error
2. don't introduce Error class
-- @1 is better - the SPIError isn't the best name, but breaking compatibility is pretty unhappy - so only one solution is @2 :(
Comments, notes ??
Regards
Pavel
>
> a Fatal cannnot be raised - it is a session end. Personally - support of
> Fatal level is wrong, and it should not be propagated to user level, but it
> is now. And then due consistency I wrote support for fatal level. But hard
> to test it.
>
I see, then never mind.
On 1/11/16 12:33 PM, Pavel Stehule wrote: > 1. break compatibility and SPIError replace by Error At this point I've lost track... what's the incompatibility between the two? -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
2016-01-11 19:41 GMT+01:00 Jim Nasby <Jim.Nasby@bluetreble.com>:
On 1/11/16 12:33 PM, Pavel Stehule wrote:1. break compatibility and SPIError replace by Error
At this point I've lost track... what's the incompatibility between the two?
the name and internal format (but this structure can be visible to user space)
Pavel
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
On 1/11/16 12:46 PM, Pavel Stehule wrote: > 2016-01-11 19:41 GMT+01:00 Jim Nasby <Jim.Nasby@bluetreble.com > <mailto:Jim.Nasby@bluetreble.com>>: > > On 1/11/16 12:33 PM, Pavel Stehule wrote: > > 1. break compatibility and SPIError replace by Error > > > At this point I've lost track... what's the incompatibility between > the two? > > > the name and internal format (but this structure can be visible to user > space) Were Error and Fatal ever documented as classes? All I see is "raise plpy.Error(msg) and raise plpy.Fatal(msg) are equivalent to calling plpy.error and plpy.fatal, respectively." which doesn't lead me to believe I should be trapping on those. It's not clear to me why you'd want to handle error and fatal differently anyway; an error is an error. Unless fatal isn't supposed to be trappable? [1] leads me to believe that you shouldn't be able to trap a FATAL because it's supposed to cause the entire session to abort. Since spiexceptions and SPIError are the only documented exceptions classes, I'd say we should stick with those and get rid of the others. Worst-case, we can have a compatability GUC, but I think plpy.Error and plpy.Fatal were just poorly thought out. [1] http://www.postgresql.org/docs/9.5/static/runtime-config-logging.html#RUNTIME-CONFIG-SEVERITY-LEVELS -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
2016-01-11 20:11 GMT+01:00 Jim Nasby <Jim.Nasby@bluetreble.com>:
On 1/11/16 12:46 PM, Pavel Stehule wrote:2016-01-11 19:41 GMT+01:00 Jim Nasby <Jim.Nasby@bluetreble.com
<mailto:Jim.Nasby@bluetreble.com>>:
On 1/11/16 12:33 PM, Pavel Stehule wrote:
1. break compatibility and SPIError replace by Error
At this point I've lost track... what's the incompatibility between
the two?
the name and internal format (but this structure can be visible to user
space)
Were Error and Fatal ever documented as classes? All I see is "raise plpy.Error(msg) and raise plpy.Fatal(msg) are equivalent to calling plpy.error and plpy.fatal, respectively." which doesn't lead me to believe I should be trapping on those.
Error and Fatal exception classes are introduced in my patch - it was Peter' request (if I remember it well), and now I am thinking so it is not good idea.
It's not clear to me why you'd want to handle error and fatal differently anyway; an error is an error. Unless fatal isn't supposed to be trappable? [1] leads me to believe that you shouldn't be able to trap a FATAL because it's supposed to cause the entire session to abort.
Since spiexceptions and SPIError are the only documented exceptions classes, I'd say we should stick with those and get rid of the others. Worst-case, we can have a compatability GUC, but I think plpy.Error and plpy.Fatal were just poorly thought out.
I have same opinion now. I remove it from my patch.
Pavel
[1] http://www.postgresql.org/docs/9.5/static/runtime-config-logging.html#RUNTIME-CONFIG-SEVERITY-LEVELS
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
On Tue, Jan 12, 2016 at 5:50 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > Error and Fatal exception classes are introduced in my patch - it was Peter' > request (if I remember it well), and now I am thinking so it is not good > idea. Now everybody is confused :). Your patch does: - PLy_exc_error = PyErr_NewException("plpy.Error", NULL, NULL); - PLy_exc_fatal = PyErr_NewException("plpy.Fatal", NULL, NULL); - PLy_exc_spi_error = PyErr_NewException("plpy.SPIError", NULL, NULL); [snip] + PLy_exc_error = PyErr_NewException("plpy.Error", PLy_exc_base, NULL); + PLy_exc_fatal = PyErr_NewException("plpy.Fatal", PLy_exc_base, NULL); + PLy_exc_spi_error = PyErr_NewException("plpy.SPIError", PLy_exc_base, NULL); So they are there without the patch, you now make them inherit from the new BaseError previously they just inherited from Exception. More soon in another reply I was just typing when this came in.
2016-01-12 17:59 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
On Tue, Jan 12, 2016 at 5:50 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Error and Fatal exception classes are introduced in my patch - it was Peter'
> request (if I remember it well), and now I am thinking so it is not good
> idea.
Now everybody is confused :). Your patch does:
- PLy_exc_error = PyErr_NewException("plpy.Error", NULL, NULL);
- PLy_exc_fatal = PyErr_NewException("plpy.Fatal", NULL, NULL);
- PLy_exc_spi_error = PyErr_NewException("plpy.SPIError", NULL, NULL);
[snip]
+ PLy_exc_error = PyErr_NewException("plpy.Error", PLy_exc_base, NULL);
+ PLy_exc_fatal = PyErr_NewException("plpy.Fatal", PLy_exc_base, NULL);
+ PLy_exc_spi_error = PyErr_NewException("plpy.SPIError",
PLy_exc_base, NULL);
So they are there without the patch, you now make them inherit from
the new BaseError previously they just inherited from Exception.
I was wrong, I am sorry.
More soon in another reply I was just typing when this came in.
On Mon, Jan 11, 2016 at 7:33 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > I see it. > > it looks like distinguish between Error and SPIError is wrong way. And I > have any other ugly use case. > > If I raise a Error from one PLPythonu function, then in other PLPython > function I'll trap a SPIError exception, because the information about class > was lost inside Postgres. And it should be pretty messy. > I have not information if any exception was User based or SPI based. This isn't that bad. The real Python exception is only lost if the first function calls into Postgres which then ends into another PL/Python function which raises. That's just the way it is. If you use Psycopg2 on the client and a PL/Python function in the server you also don't get in the client the real exception that PL/Python raised even though it's Python at both ends. The analogy isn't perfect since one is in process and the other cross process but you get the point. If a PL/Python function calls another one directly and that one raises something, the caller can catch that just like in regular Python. > The differentiation between Error and SPIError is wrong, because there isn't > any difference in reality. They're similar but not really the same thing. raise Error and plpy.error are both ways to call ereport(ERROR, ...) while SPIError is raised when coming back after calling into Postgres to execute SQL that itself raises an error. Now indeed, that executed SQL raised an error itself via another ereport(ERROR, ...) somewhere but that's a different thing. I'm getting more and more convinced that SPIError should be left unchanged and should not even inherit from BaseError as it's a different thing. And as I said this means BaseError isn't the base of all exceptions that can be raised by the plpy module so then it should probably not be called BaseError. Maybe something like ReportableError (the base class of ereport frontends). Or don't have a base class at all and just allow constructing both Error and Fatal instances with message, detail, hint etc. What you loose is you can't catch both Error and Fatal with a single except ReportableError block but Fatal is maybe not meant to be caught and Error is also not really meant to be caught either, it's meant to be raised in order to call ereport(ERROR, ...). I now like this option (no base class but same attributes in both instances) most. As I see it, what this patch tries to solve is that raise Error and plpy.error (and all the others) are not exposing all features of ereport, they can only pass a message to ereport but can't pass all the other things ereport accepts: detail, hint etc. The enhancement is being able to pass all those arguments (as positional or keyword arguments) when constructing and raising an Error or Fatal instance or when using the plpy.error helpers. Since the existing helpers accept multiple arguments already (which they unfortunately and weirdly concatenate in a tuple whose string representation is pushed into ereport as message) we can't repurpose the multiple arguments as ereport detail, hint etc. so Pavel invented paralel plpy.raise_error and friends which do that. If a bold committer says the string representation of the tuple is too weird to be relied on we could even just change meaning of plpy.error and accept (and document) the compatibility break. So the patch is almost there I think, it just needs to stop messing around with SPIError and the spidata attribute.
On 1/12/16 11:25 AM, Catalin Iacob wrote: >> >The differentiation between Error and SPIError is wrong, because there isn't >> >any difference in reality. > They're similar but not really the same thing. raise Error and > plpy.error are both ways to call ereport(ERROR, ...) while SPIError is > raised when coming back after calling into Postgres to execute SQL > that itself raises an error. Now indeed, that executed SQL raised an > error itself via another ereport(ERROR, ...) somewhere but that's a > different thing. Why should they be different? An error is an error. You either want to trap a specific type of error or you don't. Having two completely different ways to do the same thing is just confusing. IMHO the Error and Fatal classes should never have been committed, especially since they're undocumented. It's not the responsibility of this patch to remove them, but it certainly shouldn't dig the hole deeper. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
On Wed, Jan 13, 2016 at 7:40 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: > On 1/12/16 11:25 AM, Catalin Iacob wrote: >> They're similar but not really the same thing. raise Error and >> plpy.error are both ways to call ereport(ERROR, ...) while SPIError is >> raised when coming back after calling into Postgres to execute SQL >> that itself raises an error. Now indeed, that executed SQL raised an >> error itself via another ereport(ERROR, ...) somewhere but that's a >> different thing. > > > Why should they be different? An error is an error. You either want to trap > a specific type of error or you don't. Having two completely different ways > to do the same thing is just confusing. With my (indeed limited) understanding, I don't agree they're the same thing and I tried to explain above why I think they're quite different. You may not agree. If they are different things, using different exception types is natural. Consider this call chain: SQL code 1 -> PL/Python code 1 -> SPI (via plpy.execute) -> SQL code 2 -> PL/Python code 2 If I'm writing PL/Python code 1 and I want to raise an error toward SQL code 1 I use raise plpy.Error. plpy.SPIError is what I get if I call into SQL code 2 and that has an error. That error could indeed come from a plpy.Error thrown by PL/Python code 2 or it could come from a SQL syntax error or whatever. But the symmetry holds: plpy.Error is what you raise to stop the query and return errors to your SQL caller and plpy.SPIError is what you get back if you use SPI to execute some other piece of SQL which has an error. I *think* (I'm an internals newbie though so I could be wrong) that SQL code 1 doesn't call into PL/Python code 1 via SPI so why would the latter use something called SPIError to inform the former about an error?
2016-01-14 17:16 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
On Wed, Jan 13, 2016 at 7:40 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
> On 1/12/16 11:25 AM, Catalin Iacob wrote:
>> They're similar but not really the same thing. raise Error and
>> plpy.error are both ways to call ereport(ERROR, ...) while SPIError is
>> raised when coming back after calling into Postgres to execute SQL
>> that itself raises an error. Now indeed, that executed SQL raised an
>> error itself via another ereport(ERROR, ...) somewhere but that's a
>> different thing.
>
>
> Why should they be different? An error is an error. You either want to trap
> a specific type of error or you don't. Having two completely different ways
> to do the same thing is just confusing.
With my (indeed limited) understanding, I don't agree they're the same
thing and I tried to explain above why I think they're quite
different. You may not agree. If they are different things, using
different exception types is natural.
Consider this call chain: SQL code 1 -> PL/Python code 1 -> SPI (via
plpy.execute) -> SQL code 2 -> PL/Python code 2
If I'm writing PL/Python code 1 and I want to raise an error toward
SQL code 1 I use raise plpy.Error. plpy.SPIError is what I get if I
call into SQL code 2 and that has an error. That error could indeed
come from a plpy.Error thrown by PL/Python code 2 or it could come
from a SQL syntax error or whatever. But the symmetry holds:
plpy.Error is what you raise to stop the query and return errors to
your SQL caller and plpy.SPIError is what you get back if you use SPI
to execute some other piece of SQL which has an error. I *think* (I'm
an internals newbie though so I could be wrong) that SQL code 1
doesn't call into PL/Python code 1 via SPI so why would the latter use
something called SPIError to inform the former about an error?
It is not correct - outside PLPython you got a Error (PostgreSQL error has not any classes), and isn't important the raising class (Error or SPIError). Inside PL/Python you will got SPIError or successors (based on SQLcode).
Currently If I raise plpy.Error, then it is immediately trasformed to PostgreSQL, and and then to SPIError and successors.
On 1/21/16 4:57 PM, Pavel Stehule wrote: > It is not correct - outside PLPython you got a Error (PostgreSQL error > has not any classes), and isn't important the raising class (Error or > SPIError). Inside PL/Python you will got SPIError or successors (based > on SQLcode). Right. The closest thing we have to error classes is SQLSTATE. If someone found a clever way to setup an exception inheritance tree[1] on that then maybe different exceptions would make sense. Short of that, I don't see it. [1] There's a hierarchy to the SQL state codes, based on the first 2 characters. So if there was... class connection_exception(spi_exception) __init__ str = 'Connection Exception' class connection_does_not_exist(connection_exception) __init__ str = 'Connection Does Not Exist" ... to map to the small set of errors below, maybe that would make sense. Obviously that would need to be auto-generated. It seems more trouble than it's worth though. Section: Class 08 - Connection Exception 08000 E ERRCODE_CONNECTION_EXCEPTION connection_exception 08003 E ERRCODE_CONNECTION_DOES_NOT_EXIST connection_does_not_exist 08006 E ERRCODE_CONNECTION_FAILURE connection_failure 08001 E ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION sqlclient_unable_to_establish_sqlconnection 08004 E ERRCODE_SQLSERVER_REJECTED_ESTABLISHMENT_OF_SQLCONNECTION sqlserver_rejected_establishment_of_sqlconnection 08007 E ERRCODE_TRANSACTION_RESOLUTION_UNKNOWN transaction_resolution_unknown 08P01 E ERRCODE_PROTOCOL_VIOLATION protocol_violation -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
On 1/21/16, Pavel Stehule <pavel.stehule@gmail.com> wrote: > 2016-01-14 17:16 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>: >> Consider this call chain: SQL code 1 -> PL/Python code 1 -> SPI (via >> plpy.execute) -> SQL code 2 -> PL/Python code 2 >> >> If I'm writing PL/Python code 1 and I want to raise an error toward >> SQL code 1 I use raise plpy.Error. plpy.SPIError is what I get if I >> call into SQL code 2 and that has an error. That error could indeed >> come from a plpy.Error thrown by PL/Python code 2 or it could come >> from a SQL syntax error or whatever. But the symmetry holds: >> plpy.Error is what you raise to stop the query and return errors to >> your SQL caller and plpy.SPIError is what you get back if you use SPI >> to execute some other piece of SQL which has an error. I *think* (I'm >> an internals newbie though so I could be wrong) that SQL code 1 >> doesn't call into PL/Python code 1 via SPI so why would the latter use >> something called SPIError to inform the former about an error? >> > > It is not correct - outside PLPython you got a Error (PostgreSQL error has > not any classes), and isn't important the raising class (Error or > SPIError). Inside PL/Python you will got SPIError or successors (based on > SQLcode). What exactly is not correct? Indeed, *outside* PLPython you don't get a Python exception because Postgres isn't Python, you get Postgres' way of representing an exception (error code, message, hint and so on that might go to the client or not etc.). But that's normal, it's just the impedance mismatch between the fact that you embed Python with its rules into Postgres with its other rules. It's normal that the "host" (Postgres) wins in the end and uses its mechanism to report errors. The "guest" (PLPython) is there only to give mechanisms to activate the "host". But *inside* PLPython what I wrote is true, see this example for what I mean: CREATE FUNCTION test() RETURNS int AS $$ def a_func(): raise plpy.Error('an error') try: a_func() except plpy.Error as exc: plpy.info('have exc {} of type {}'.format(exc, type(exc))) plpy.info('havespidata {}'.format(hasattr(exc, 'spidata'))) $$ LANGUAGE plpython3u; Running this produces: DROP FUNCTION CREATE FUNCTION psql:plpy_demo:16: INFO: have exc an error of type <class 'plpy.Error'> psql:plpy_demo:16: INFO: have spidata False > Currently If I raise plpy.Error, then it is immediately trasformed to > PostgreSQL, and and then to SPIError and successors. Depends how you define immediately. I would say it's really not immediately, it's only at the Postgres boundary. Imagine in the example above that a_func could call lots of other Python code and somewhere deep down raise Error would be used. Within that whole execution the error stays Error and can be caught as such, it will have nothing to do with SPIError. But in my opinion this discussion shouldn't really even be about catching these things, most of the times you won't catch them and instead you'll let them go to Postgres. The discussion should be whether raise plpy.Error(...), plpy.raise_error, plpy.raise_info(,,,) etc. all with keyword argument support are a good PLPython interface to Postgres' ereport. I think they are. On a different note, I've explained what I think but I might be wrong and don't want to stall the patch just because I don't agree with the approach it takes. The waiting for author status is ok since, as far as I can see, Pavel also agrees that at least some of the issues raised in http://www.postgresql.org/message-id/CAHg_5grU=LVRbZDhfCiiYc9PhS=1X5f=Gd44-3JcuL-QAoRC-A@mail.gmail.com need to be fixed. Would it help maybe to remove myself as reviewer in the CF? Maybe that makes somebody else pick it up and move it further. I could also code the version I'm describing but that design would be contrary to what Jim and Pavel lean towards now. It could help to compare approaches side by side but I don't like the idea of dueling patches.
2016-01-26 7:31 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
On 1/21/16, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> 2016-01-14 17:16 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
>> Consider this call chain: SQL code 1 -> PL/Python code 1 -> SPI (via
>> plpy.execute) -> SQL code 2 -> PL/Python code 2
>>
>> If I'm writing PL/Python code 1 and I want to raise an error toward
>> SQL code 1 I use raise plpy.Error. plpy.SPIError is what I get if I
>> call into SQL code 2 and that has an error. That error could indeed
>> come from a plpy.Error thrown by PL/Python code 2 or it could come
>> from a SQL syntax error or whatever. But the symmetry holds:
>> plpy.Error is what you raise to stop the query and return errors to
>> your SQL caller and plpy.SPIError is what you get back if you use SPI
>> to execute some other piece of SQL which has an error. I *think* (I'm
>> an internals newbie though so I could be wrong) that SQL code 1
>> doesn't call into PL/Python code 1 via SPI so why would the latter use
>> something called SPIError to inform the former about an error?
>>
>
> It is not correct - outside PLPython you got a Error (PostgreSQL error has
> not any classes), and isn't important the raising class (Error or
> SPIError). Inside PL/Python you will got SPIError or successors (based on
> SQLcode).
What exactly is not correct? Indeed, *outside* PLPython you don't get
a Python exception because Postgres isn't Python, you get Postgres'
way of representing an exception (error code, message, hint and so on
that might go to the client or not etc.). But that's normal, it's just
the impedance mismatch between the fact that you embed Python with its
rules into Postgres with its other rules. It's normal that the "host"
(Postgres) wins in the end and uses its mechanism to report errors.
The "guest" (PLPython) is there only to give mechanisms to activate
the "host".
But *inside* PLPython what I wrote is true, see this example for what I mean:
CREATE FUNCTION test()
RETURNS int
AS $$
def a_func():
raise plpy.Error('an error')
try:
a_func()
except plpy.Error as exc:
plpy.info('have exc {} of type {}'.format(exc, type(exc)))
plpy.info('have spidata {}'.format(hasattr(exc, 'spidata')))
$$ LANGUAGE plpython3u;
Running this produces:
DROP FUNCTION
CREATE FUNCTION
psql:plpy_demo:16: INFO: have exc an error of type <class 'plpy.Error'>
psql:plpy_demo:16: INFO: have spidata False
> Currently If I raise plpy.Error, then it is immediately trasformed to
> PostgreSQL, and and then to SPIError and successors.
Depends how you define immediately. I would say it's really not
immediately, it's only at the Postgres boundary. Imagine in the
example above that a_func could call lots of other Python code and
somewhere deep down raise Error would be used. Within that whole
execution the error stays Error and can be caught as such, it will
have nothing to do with SPIError.
But in my opinion this discussion shouldn't really even be about
catching these things, most of the times you won't catch them and
instead you'll let them go to Postgres. The discussion should be
whether raise plpy.Error(...), plpy.raise_error, plpy.raise_info(,,,)
etc. all with keyword argument support are a good PLPython interface
to Postgres' ereport. I think they are.
On a different note, I've explained what I think but I might be wrong
and don't want to stall the patch just because I don't agree with the
approach it takes.
The waiting for author status is ok since, as far as I can see, Pavel
also agrees that at least some of the issues raised in
http://www.postgresql.org/message-id/CAHg_5grU=LVRbZDhfCiiYc9PhS=1X5f=Gd44-3JcuL-QAoRC-A@mail.gmail.com
need to be fixed.
Would it help maybe to remove myself as reviewer in the CF? Maybe that
makes somebody else pick it up and move it further.
I could also code the version I'm describing but that design would be
contrary to what Jim and Pavel lean towards now. It could help to
compare approaches side by side but I don't like the idea of dueling
patches.
I would to reduce this patch and don't try to touch on buildin exceptions. I hope, so there isn't any disagreement?
I though about it lot of, and I see a the core of problems in orthogonal constructed exceptions in Python and Postgres. We working with statements elog, ereport, RAISE EXCEPTION - and these statements are much more like templates - can generate any possible exception. Python is based on working with instances of predefined exceptions. And it is core of problem. Template like class cannot be ancestor of instance oriented classes. This is task for somebody who knows well OOP and meta OOP in Python - total different chapter, different patch
Regards
Pavel
2016-01-26 8:22 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com>:
2016-01-26 7:31 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:On 1/21/16, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> 2016-01-14 17:16 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
>> Consider this call chain: SQL code 1 -> PL/Python code 1 -> SPI (via
>> plpy.execute) -> SQL code 2 -> PL/Python code 2
>>
>> If I'm writing PL/Python code 1 and I want to raise an error toward
>> SQL code 1 I use raise plpy.Error. plpy.SPIError is what I get if I
>> call into SQL code 2 and that has an error. That error could indeed
>> come from a plpy.Error thrown by PL/Python code 2 or it could come
>> from a SQL syntax error or whatever. But the symmetry holds:
>> plpy.Error is what you raise to stop the query and return errors to
>> your SQL caller and plpy.SPIError is what you get back if you use SPI
>> to execute some other piece of SQL which has an error. I *think* (I'm
>> an internals newbie though so I could be wrong) that SQL code 1
>> doesn't call into PL/Python code 1 via SPI so why would the latter use
>> something called SPIError to inform the former about an error?
>>
>
> It is not correct - outside PLPython you got a Error (PostgreSQL error has
> not any classes), and isn't important the raising class (Error or
> SPIError). Inside PL/Python you will got SPIError or successors (based on
> SQLcode).
What exactly is not correct? Indeed, *outside* PLPython you don't get
a Python exception because Postgres isn't Python, you get Postgres'
way of representing an exception (error code, message, hint and so on
that might go to the client or not etc.). But that's normal, it's just
the impedance mismatch between the fact that you embed Python with its
rules into Postgres with its other rules. It's normal that the "host"
(Postgres) wins in the end and uses its mechanism to report errors.
The "guest" (PLPython) is there only to give mechanisms to activate
the "host".
But *inside* PLPython what I wrote is true, see this example for what I mean:
CREATE FUNCTION test()
RETURNS int
AS $$
def a_func():
raise plpy.Error('an error')
try:
a_func()
except plpy.Error as exc:
plpy.info('have exc {} of type {}'.format(exc, type(exc)))
plpy.info('have spidata {}'.format(hasattr(exc, 'spidata')))
$$ LANGUAGE plpython3u;
Running this produces:
DROP FUNCTION
CREATE FUNCTION
psql:plpy_demo:16: INFO: have exc an error of type <class 'plpy.Error'>
psql:plpy_demo:16: INFO: have spidata False
> Currently If I raise plpy.Error, then it is immediately trasformed to
> PostgreSQL, and and then to SPIError and successors.
Depends how you define immediately. I would say it's really not
immediately, it's only at the Postgres boundary. Imagine in the
example above that a_func could call lots of other Python code and
somewhere deep down raise Error would be used. Within that whole
execution the error stays Error and can be caught as such, it will
have nothing to do with SPIError.
But in my opinion this discussion shouldn't really even be about
catching these things, most of the times you won't catch them and
instead you'll let them go to Postgres. The discussion should be
whether raise plpy.Error(...), plpy.raise_error, plpy.raise_info(,,,)
etc. all with keyword argument support are a good PLPython interface
to Postgres' ereport. I think they are.
On a different note, I've explained what I think but I might be wrong
and don't want to stall the patch just because I don't agree with the
approach it takes.
The waiting for author status is ok since, as far as I can see, Pavel
also agrees that at least some of the issues raised in
http://www.postgresql.org/message-id/CAHg_5grU=LVRbZDhfCiiYc9PhS=1X5f=Gd44-3JcuL-QAoRC-A@mail.gmail.com
need to be fixed.
Would it help maybe to remove myself as reviewer in the CF? Maybe that
makes somebody else pick it up and move it further.
please, wait. We can finish the part, where is a agreement. And although it is not final solution from Python perspective - I hope, so it can be really useful for PLPython user - there will be one way how to raise rich exception without ugly workaround
Regards
Pavel
I could also code the version I'm describing but that design would be
contrary to what Jim and Pavel lean towards now. It could help to
compare approaches side by side but I don't like the idea of dueling
patches.I would to reduce this patch and don't try to touch on buildin exceptions. I hope, so there isn't any disagreement?I though about it lot of, and I see a the core of problems in orthogonal constructed exceptions in Python and Postgres. We working with statements elog, ereport, RAISE EXCEPTION - and these statements are much more like templates - can generate any possible exception. Python is based on working with instances of predefined exceptions. And it is core of problem. Template like class cannot be ancestor of instance oriented classes. This is task for somebody who knows well OOP and meta OOP in Python - total different chapter, different patch.
RegardsPavel
Hi
But in my opinion this discussion shouldn't really even be about
catching these things, most of the times you won't catch them and
instead you'll let them go to Postgres. The discussion should be
whether raise plpy.Error(...), plpy.raise_error, plpy.raise_info(,,,)
etc. all with keyword argument support are a good PLPython interface
to Postgres' ereport. I think they are.
I removed from previous patch all OOP related changes. New patch contains raise_xxxx functions only. This interface is new generation of previous functions: info, notice, warning, error with keyword parameters interface. I didn't changed older functions due keeping compatibility.
I spent lot of time with experiments how to merge PostgreSQL exception system with Python exception system. But I didn't find a solution without some design issues. These concepts(s) are too different (note: there are two concepts: PostgreSQL levels and ANSI SQL SQLSTATE (class, subclass codes).
I hope so raise_xxxx has benefit for PLPythonu users too. Currently users has to use ugly workaround to raise rich PostgreSQL exception from Python. With new function all available functionality related to PostgreSQL exception can be used simply.
plpy.raise_warning("some doesn't work", detail = "user registration ... ")
This patch is reduced previous patch (no new features, no different solution, no different code), so I use same entry in commitfest.
Regards
Pavel
Вложения
On recent occasions, Pavel Stehule and Catalin Iacob have written: ... >> But *inside* PLPython what I wrote is true, see this example for what I >> mean: >> >> CREATE FUNCTION test() >> RETURNS int >> AS $$ >> def a_func(): >> raise plpy.Error('an error') >> >> try: >> a_func() >> except plpy.Error as exc: >> plpy.info('have exc {} of type {}'.format(exc, type(exc))) >> plpy.info('have spidata {}'.format(hasattr(exc, 'spidata'))) >> $$ LANGUAGE plpython3u; >> >> Running this produces: >> >> DROP FUNCTION >> CREATE FUNCTION >> psql:plpy_demo:16: INFO: have exc an error of type <class 'plpy.Error'> >> psql:plpy_demo:16: INFO: have spidata False >> >>> Currently If I raise plpy.Error, then it is immediately trasformed to >>> PostgreSQL, and and then to SPIError and successors. >> >> Depends how you define immediately. I would say it's really not >> immediately, it's only at the Postgres boundary. Imagine in the >> example above that a_func could call lots of other Python code and >> somewhere deep down raise Error would be used. Within that whole >> execution the error stays Error and can be caught as such, it will >> have nothing to do with SPIError. ... > I would to reduce this patch and don't try to touch on buildin exceptions. > I hope, so there isn't any disagreement? > > I though about it lot of, and I see a the core of problems in orthogonal > constructed exceptions in Python and Postgres. We working with statements > elog, ereport, RAISE EXCEPTION - and these statements are much more like > templates - can generate any possible exception. Python is based on working > with instances of predefined exceptions. And it is core of problem. > Template like class cannot be ancestor of instance oriented classes. This > is task for somebody who knows well OOP and meta OOP in Python - total I've been following this discussion with great interest, because PL/Java also is rather overdue for tackling the same issues: it has some partial ability to catch things from PostgreSQL and even examine them in proper detail, but very limited ability to throw things as information-rich as is possible from C with ereport. And that work is not as far along as you are with PL/Python, there is just a preliminary design/discussion wiki document at https://github.com/tada/pljava/wiki/Thoughts-on-logging I was unaware of this project in PL/Pythonu when I began it, then added the reference when I saw this discussion. In some ways designing for PL/Java might be easier because it is not complete freedom to invent the whole design; for example in Java there is already an SQLException hierarchy specified in JDBC 4, with SQLNonTransientException, SQLTransientException, SQLRecoverable exception and their several subclasses, with a defined mapping from the leading two-character SQLSTATE category codes. So for Java at least one part of the bikeshed is already painted. :) But a lot of the rest of the design clearly involves asking the same questions, such as what happens to an event as it bubbles up from a PL function called by a PG query called by a PL function ... maybe all the way out to an exception handler in front-end code. (That could be a thinkable thought for Java because the standards specify JDBC as its common database API both client-side and for server PL code, so it's natural to think about the exception handling being similar both places. It would be analogous to having the plpy database access functions designed to present an interface consistent with a client-side API like psycopg. Forgive me, I'm not familiar enough with PL/Pythonu to know if it *is* like that or not.) From following this thread, I have the impression that what is currently under discussion is low-hanging fruit that may be committed soon, and other questions remaining for further design and discussion. I'll continue to be interested in how the deeper design issues continue to be shaped. Maybe ideas can sort of cross-pollinate .... -Chap
Hi
>
> I though about it lot of, and I see a the core of problems in orthogonal
> constructed exceptions in Python and Postgres. We working with statements
> elog, ereport, RAISE EXCEPTION - and these statements are much more like
> templates - can generate any possible exception. Python is based on working
> with instances of predefined exceptions. And it is core of problem.
> Template like class cannot be ancestor of instance oriented classes. This
> is task for somebody who knows well OOP and meta OOP in Python - total
I've been following this discussion with great interest, because PL/Java
also is rather overdue for tackling the same issues: it has some partial
ability to catch things from PostgreSQL and even examine them in proper
detail, but very limited ability to throw things as information-rich as
is possible from C with ereport. And that work is not as far along as
you are with PL/Python, there is just a preliminary design/discussion
wiki document at
https://github.com/tada/pljava/wiki/Thoughts-on-logging
I was unaware of this project in PL/Pythonu when I began it, then added
the reference when I saw this discussion.
I read your article and it is exactly same situation.
It is conflict between procedural (PostgreSQL) and OOP (Python, Java) API. I see possible solution in design independent class hierarchies - static (buildin exceptions) and dynamic (custom exceptions). It cannot be mixed, but there can be some abstract ancestor. Second solution is defensive - using procedural API for custom exceptions - what i doing in PLPythonu.
Regards
Pavel
On Tue, Jan 26, 2016 at 5:42 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > I removed from previous patch all OOP related changes. New patch contains > raise_xxxx functions only. This interface is new generation of previous > functions: info, notice, warning, error with keyword parameters interface. I > didn't changed older functions due keeping compatibility. Hi, Even without the OOP changes, the exception classes are still there as the underlying mechanism that error and raise_error use. I looked at the patch and what I don't like about it is that raise_error uses SPIError to transport detail, hint etc while error uses Error. This is inconsistent and confusing. Take this example: CREATE OR REPLACE FUNCTION err() RETURNS int AS $$ plpy.error('only msg from plpy.error', 'arg2 for error is part of tuple', 'arg3 also in tuple') $$ LANGUAGE plpython3u; SELECT err(); CREATE OR REPLACE FUNCTION raise_err() RETURNS int AS $$ plpy.raise_error('only msg from plpy.raise_error', 'arg2 for raise_error is detail', 'arg3 is hint') $$ LANGUAGE plpython3u; SELECT raise_err(); With your patch, this results in: CREATE FUNCTION psql:plpy_error_raise_error_difference:9: ERROR: plpy.Error: ('only msg from plpy.error', 'arg2 for error is part of tuple', 'arg3 also in tuple') CONTEXT: Traceback (most recent call last): PL/Python function "err", line 2, in <module> plpy.error('only msg from plpy.error', 'arg2 for error is part of tuple', 'arg3 also in tuple') PL/Python function "err" CREATE FUNCTION psql:plpy_error_raise_error_difference:17: ERROR: plpy.SPIError: only msg from plpy.raise_error DETAIL: arg2 for raise_error is detail HINT: arg3 is hint CONTEXT: Traceback (most recent call last): PL/Python function "raise_err", line 2, in <module> plpy.raise_error('only msg from plpy.raise_error', 'arg2 for raise_error is detail', 'arg3 is hint') PL/Python function "raise_err" >From the output you can already see that plpy.error and plpy.raise_error (even with a single argument) don't use the same exception: plpy.error uses Error while raise_error uses SPIError. I think with a single argument they should be identical and with multiple arguments raise_error should still use Error but use the arguments as detail, hint etc. In code you had to export a function to plpy_spi.h to get to the details in SPIError while plpy.error worked just fine without that. I've attached two patches on top of yours: first is a comment fix, the next one is a rough proof of concept for using plpy.Error to carry the details. This allows undoing the change to export PLy_spi_exception_set to plpy_spi.h. And then I realized, the changes you did to SPIError are actually a separate enhancement (report more details like schema name, table name and so on for the query executed by SPI). They should be split into a separate patch. With these patches the output of the above test is: CREATE FUNCTION psql:plpy_error_raise_error_difference:9: ERROR: plpy.Error: ('only msg from plpy.error', 'arg2 for error is part of tuple', 'arg3 also in tuple') CONTEXT: Traceback (most recent call last): PL/Python function "err", line 2, in <module> plpy.error('only msg from plpy.error', 'arg2 for error is part of tuple', 'arg3 also in tuple') PL/Python function "err" CREATE FUNCTION psql:plpy_error_raise_error_difference:17: ERROR: plpy.Error: only msg from plpy.raise_error DETAIL: arg2 for raise_error is detail HINT: arg3 is hint CONTEXT: Traceback (most recent call last): PL/Python function "raise_err", line 2, in <module> plpy.raise_error('only msg from plpy.raise_error', 'arg2 for raise_error is detail', 'arg3 is hint') PL/Python function "raise_err" The two approaches are consistent now, both create a plpy.Error. The patch is not complete, it only handles detail and hint not the others but it should illustrate the idea. Looking at the output above, I don't see who would rely on calling plpy.error with multiple arguments and getting the tuple so I'm actually in favor of just breaking backward compatibility. Note that passing multiple arguments isn't even documented. So I would just change debug, info, error and friends to do what raise_debug, raise_info, raise_error do. With a single argument behavior stays the same, with multiple arguments one gets more useful behavior (detail, hint) instead of the useless tuple. That's my preference but we can leave the patch with raise and leave the decision to the committer. What do you think? Jim doesn't like the separate Error being raised. I don't agree, but even if I would, it's not this patch's job to change that and Error is already raised today. This patch should be consistent with the status quo and therefore be similar to plpy.error. If Error is bad, it should be replaced by SPIError everywhere separately. Next week I'll send a patch to improve the docs.
Вложения
<p dir="ltr">hi<p dir="ltr">I am sorry, I am writing from mobile phone and I cannot to cutting text well.<p dir="ltr">Dne29. 1. 2016 18:09 napsal uživatel "Catalin Iacob" <<a href="mailto:iacobcatalin@gmail.com">iacobcatalin@gmail.com</a>>:<br/> This interface is new generation of previous<br/> > > functions: info, notice, warning, error with keyword parameters interface. I<br /> > > didn'tchanged older functions due keeping compatibility.<br /> ><br /> > Hi,<br /> ><br /> > Even without theOOP changes, the exception classes are still there as<br /> > the underlying mechanism that error and raise_error use.<br/> ><br /> > I looked at the patch and what I don't like about it is that<br /> > raise_error uses SPIErrorto transport detail, hint etc while error<br /> > uses Error. This is inconsistent and confusing.<br /> ><br/> > Take this example:<br /> ><br /> > CREATE OR REPLACE FUNCTION err()<br /> > RETURNS int<br /> >AS $$<br /> > plpy.error('only msg from plpy.error', 'arg2 for error is part of<br /> > tuple', 'arg3 also intuple')<br /> > $$ LANGUAGE plpython3u;<br /> ><br /> > SELECT err();<br /> ><br /> > CREATE OR REPLACEFUNCTION raise_err()<br /> > RETURNS int<br /> > AS $$<br /> > plpy.raise_error('only msg from plpy.raise_error','arg2 for<br /> > raise_error is detail', 'arg3 is hint')<br /> > $$ LANGUAGE plpython3u;<br /> ><br/> > SELECT raise_err();<br /> ><br /> > With your patch, this results in:<br /> ><br /> > CREATE FUNCTION<br/> > psql:plpy_error_raise_error_difference:9: ERROR: plpy.Error: ('only<br /> > msg from plpy.error','arg2 for error is part of tuple', 'arg3 also in<br /> > tuple')<br /> > CONTEXT: Traceback (most recentcall last):<br /> > PL/Python function "err", line 2, in <module><br /> > plpy.error('only msg fromplpy.error', 'arg2 for error is part of<br /> > tuple', 'arg3 also in tuple')<br /> > PL/Python function "err"<br/> > CREATE FUNCTION<br /> > psql:plpy_error_raise_error_difference:17: ERROR: plpy.SPIError: only<br /> >msg from plpy.raise_error<br /> > DETAIL: arg2 for raise_error is detail<br /> > HINT: arg3 is hint<br /> >CONTEXT: Traceback (most recent call last):<br /> > PL/Python function "raise_err", line 2, in <module><br/> > plpy.raise_error('only msg from plpy.raise_error', 'arg2 for<br /> > raise_error is detail','arg3 is hint')<br /> > PL/Python function "raise_err"<br /> ><br /> > From the output you can already seethat plpy.error and<br /> > plpy.raise_error (even with a single argument) don't use the same<br /> > exception:plpy.error uses Error while raise_error uses SPIError. I<br /> > think with a single argument they should beidentical and with<br /> > multiple arguments raise_error should still use Error but use the<br /> > arguments asdetail, hint etc. In code you had to export a function to<br /> > plpy_spi.h to get to the details in SPIError whileplpy.error worked<br /> > just fine without that.<br /> ><p dir="ltr">yes, using SPIError is unhappy, I used it,because this exception supported structured exceptions already, but In python code it has not sense and raising Erroris better.<p dir="ltr">> I've attached two patches on top of yours: first is a comment fix, the<br /> > next oneis a rough proof of concept for using plpy.Error to carry the<br /> > details. This allows undoing the change to export<br/> > PLy_spi_exception_set to plpy_spi.h. And then I realized, the changes<br /> > you did to SPIError areactually a separate enhancement (report more<br /> > details like schema name, table name and so on for the query executed<br/> > by SPI). They should be split into a separate patch. <p dir="ltr">This patch is simple, and maintainingmore patches has not sense.<br /><p dir="ltr">With these<br /> > patches the output of the above test is:<br/> ><br /> > CREATE FUNCTION<br /> > psql:plpy_error_raise_error_difference:9: ERROR: plpy.Error: ('only<br/> > msg from plpy.error', 'arg2 for error is part of tuple', 'arg3 also in<br /> > tuple')<br /> > CONTEXT: Traceback (most recent call last):<br /> > PL/Python function "err", line 2, in <module><br /> > plpy.error('only msg from plpy.error', 'arg2 for error is part of<br /> > tuple', 'arg3 also in tuple')<br /> >PL/Python function "err"<br /> > CREATE FUNCTION<br /> > psql:plpy_error_raise_error_difference:17: ERROR: plpy.Error:only<br /> > msg from plpy.raise_error<br /> > DETAIL: arg2 for raise_error is detail<br /> > HINT: arg3 is hint<br /> > CONTEXT: Traceback (most recent call last):<br /> > PL/Python function "raise_err", line2, in <module><br /> > plpy.raise_error('only msg from plpy.raise_error', 'arg2 for<br /> > raise_erroris detail', 'arg3 is hint')<br /> > PL/Python function "raise_err"<br /> ><br /> > The two approachesare consistent now, both create a plpy.Error. The<br /> > patch is not complete, it only handles detail andhint not the others<br /> > but it should illustrate the idea.<br /> ><br /> > Looking at the output above, Idon't see who would rely on calling<br /> > plpy.error with multiple arguments and getting the tuple so I'm<br /> >actually in favor of just breaking backward compatibility. Note that<br /> > passing multiple arguments isn't evendocumented. So I would just<br /> > change debug, info, error and friends to do what raise_debug,<br /> > raise_info,raise_error do. With a single argument behavior stays the<br /> > same, with multiple arguments one gets moreuseful behavior (detail,<br /> > hint) instead of the useless tuple. That's my preference but we can<br /> > leavethe patch with raise and leave the decision to the committer.<br /> ><p dir="ltr">if breaking compatibility, thenraise* functions are useless, and should be removed.<p dir="ltr">> What do you think? Jim doesn't like the separateError being raised. I<br /> > don't agree, but even if I would, it's not this patch's job to change<br /> >that and Error is already raised today. This patch should be<br /> > consistent with the status quo and thereforebe similar to plpy.error.<br /> > If Error is bad, it should be replaced by SPIError everywhere<br /> > separately.<pdir="ltr">ok<p dir="ltr">The wrong is duality of Error and SPIError, because it is only one real exception.<pdir="ltr">The correct naming for current situation are names: PoorError (message only) and RichError (full setof Edata fields). This is strange from Pg perspective and still is strange from Python view.<p dir="ltr">I agree, we shouldto unify a exception from (raise, elog) functions and Error has little bit better sense for me. The disadvantage isredundant code for filling and reading exception properties (I have to support SPIError), but in this case it is less wrongthan strange behave.<p dir="ltr">Regards<p dir="ltr">Pavel<br /><p dir="ltr">><br /> > Next week I'll send a patchto improve the docs.<br />
On Mon, Feb 1, 2016 at 5:37 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > Dne 29. 1. 2016 18:09 napsal uživatel "Catalin Iacob" > <iacobcatalin@gmail.com>: >> Looking at the output above, I don't see who would rely on calling >> plpy.error with multiple arguments and getting the tuple so I'm >> actually in favor of just breaking backward compatibility. Note that >> passing multiple arguments isn't even documented. So I would just >> change debug, info, error and friends to do what raise_debug, >> raise_info, raise_error do. With a single argument behavior stays the >> same, with multiple arguments one gets more useful behavior (detail, >> hint) instead of the useless tuple. That's my preference but we can >> leave the patch with raise and leave the decision to the committer. >> > > if breaking compatibility, then raise* functions are useless, and should be > removed. Indeed. I think it's better to change the existing functions and break compatibility instead of adding the raise_ functions. But the committer will decide if that's what should be done. Since you wrote the patch with raise_* I propose you keep it that way for now and let the committer decide. I wrote the doc patch based on raise_* as well. Attached is the doc patch (made on top of your patch). I'll wait for you to combine them and switch to raising Error and then hopefully this is ready for a committer to look at.
Вложения
<p dir="ltr"><br /> Dne 2. 2. 2016 7:30 napsal uživatel "Catalin Iacob" <<a href="mailto:iacobcatalin@gmail.com">iacobcatalin@gmail.com</a>>:<br/> ><br /> > On Mon, Feb 1, 2016 at 5:37 PM,Pavel Stehule <<a href="mailto:pavel.stehule@gmail.com">pavel.stehule@gmail.com</a>> wrote:<br /> > > Dne29. 1. 2016 18:09 napsal uživatel "Catalin Iacob"<br /> > > <<a href="mailto:iacobcatalin@gmail.com">iacobcatalin@gmail.com</a>>:<br/> > >> Looking at the output above, I don'tsee who would rely on calling<br /> > >> plpy.error with multiple arguments and getting the tuple so I'm<br/> > >> actually in favor of just breaking backward compatibility. Note that<br /> > >> passing multiplearguments isn't even documented. So I would just<br /> > >> change debug, info, error and friends to dowhat raise_debug,<br /> > >> raise_info, raise_error do. With a single argument behavior stays the<br /> >>> same, with multiple arguments one gets more useful behavior (detail,<br /> > >> hint) instead of theuseless tuple. That's my preference but we can<br /> > >> leave the patch with raise and leave the decision tothe committer.<br /> > >><br /> > ><br /> > > if breaking compatibility, then raise* functions areuseless, and should be<br /> > > removed.<br /> ><br /> > Indeed. I think it's better to change the existingfunctions and break<br /> > compatibility instead of adding the raise_ functions. But the<br /> > committerwill decide if that's what should be done. Since you wrote<br /> > the patch with raise_* I propose you keepit that way for now and let<br /> > the committer decide. I wrote the doc patch based on raise_* as well.<p dir="ltr">Ifwe decided to break compatibility, then we have to do explicitly. Thid discussion can continue with commiter,but send path with duplicitly defined functions has not sense for me. Currently I out of office, so I cannot toclean it. 4.2 I should to work usually<p dir="ltr">><br /> > Attached is the doc patch (made on top of your patch).I'll wait for<br /> > you to combine them and switch to raising Error and then hopefully<br /> > this is readyfor a committer to look at.<br />
On Tue, Feb 2, 2016 at 3:48 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > If we decided to break compatibility, then we have to do explicitly. Thid > discussion can continue with commiter, but send path with duplicitly defined > functions has not sense for me. Currently I out of office, so I cannot to > clean it. 4.2 I should to work usually I think I didn't make myself clear so I'll try again. There are 2 options: 1. keep info etc. unchanged and add raise_info etc. 2. change behaviour of info etc. and of course don't add raise_* You already implemented 1. I said I think 2. is better and worth the compatibility break in my opinion. But the committer decides not me. Since 1. is already done, what I propose is: let's finish the other aspects of the patch (incorporate my docs updates and details in Error instead of SPIError) then I'll mark this ready for committer and summarize the discussion. I will say there that option 1. was implemented since it's backward compatible but that if the committer thinks option 2. is better we can change the patch to implement option 2. If you do the work for 2 now, the committer might still say "I want 1" and then you need to do more work again to go back to 1. Easier to just stay with 1 for now until we have committer input.
On Tue, Feb 2, 2016 at 10:39 AM, Catalin Iacob <iacobcatalin@gmail.com> wrote: > On Tue, Feb 2, 2016 at 3:48 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: >> If we decided to break compatibility, then we have to do explicitly. Thid >> discussion can continue with commiter, but send path with duplicitly defined >> functions has not sense for me. Currently I out of office, so I cannot to >> clean it. 4.2 I should to work usually > > I think I didn't make myself clear so I'll try again. > > There are 2 options: > > 1. keep info etc. unchanged and add raise_info etc. > 2. change behaviour of info etc. and of course don't add raise_* > > You already implemented 1. I said I think 2. is better and worth the > compatibility break in my opinion. But the committer decides not me. > > Since 1. is already done, what I propose is: let's finish the other > aspects of the patch (incorporate my docs updates and details in Error > instead of SPIError) then I'll mark this ready for committer and > summarize the discussion. I will say there that option 1. was > implemented since it's backward compatible but that if the committer > thinks option 2. is better we can change the patch to implement option > 2. If you do the work for 2 now, the committer might still say "I want > 1" and then you need to do more work again to go back to 1. Easier to > just stay with 1 for now until we have committer input. The eventual committer is likely to be much happier with this patch if you guys have achieved consensus among yourselves on the best approach. (Disclaimer: The eventual committer won't be me. I'm not a Python guy. But we try to proceed by consensus rather than committer-dictat around here, when we can. Obviously the committer has the final say at some level, but it's better if that power doesn't need to be exercised too often.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas wrote: > The eventual committer is likely to be much happier with this patch if > you guys have achieved consensus among yourselves on the best > approach. > > (Disclaimer: The eventual committer won't be me. I'm not a Python > guy. But we try to proceed by consensus rather than committer-dictat > around here, when we can. Obviously the committer has the final say > at some level, but it's better if that power doesn't need to be > exercised too often.) Actually I imagine that if there's no agreement between author and first reviewer, there might not *be* a committer in the first place. Perhaps try to get someone else to think about it and make a decision. It is possible that some other committer is able to decide by themselves but I wouldn't count on it. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2/2/16 4:52 PM, Alvaro Herrera wrote: > Robert Haas wrote: > >> The eventual committer is likely to be much happier with this patch if >> you guys have achieved consensus among yourselves on the best >> approach. >> >> (Disclaimer: The eventual committer won't be me. I'm not a Python >> guy. But we try to proceed by consensus rather than committer-dictat >> around here, when we can. Obviously the committer has the final say >> at some level, but it's better if that power doesn't need to be >> exercised too often.) > > Actually I imagine that if there's no agreement between author and first > reviewer, there might not *be* a committer in the first place. Perhaps > try to get someone else to think about it and make a decision. It is > possible that some other committer is able to decide by themselves but I > wouldn't count on it. +1. FWIW, I'd think it's better to not break backwards compatibility, but I'm also far from a python expert. It might well be worth adding a plpython GUC to control the behavior so that there's a migration path forward, or maybe do something like the 'import __future__' that python is doing to ease migration to python 3. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
On Tue, Feb 2, 2016 at 11:52 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Robert Haas wrote: >> The eventual committer is likely to be much happier with this patch if >> you guys have achieved consensus among yourselves on the best >> approach. > > Actually I imagine that if there's no agreement between author and first > reviewer, there might not *be* a committer in the first place. Perhaps > try to get someone else to think about it and make a decision. It is > possible that some other committer is able to decide by themselves but I > wouldn't count on it. Pavel and I agree that the backward incompatible behavior is cleaner, but it's backward incompatible. Whether that extra cleanness is worth the incompatibility is never obvious. In this case I think it does. But since my opinion is just my opinion, I was planning to make the committer be that someone else that weighs the issues and makes a decision. I'm new around here and picked this patch to get started due to having Python knowledge and the patch seeming self contained enough. Being new makes me wonder all the time "am I just making life difficult for the patch author or is this idea genuinely good and therefore I should push it forward?". I think more beginners that try to do reviews struggle with this. But, let's try to reach a decision. The existing behaviour dates back to 0bef7ba Add plpython code by Bruce. I've added him to the mail, maybe he can help us with a decision. I'll summarize the patch and explain the incompatibility decision with some illustrative code. The patch is trying to make it possible to call ereport from PL/Python and provide the rich ereport information (detail, hint, sqlcode etc.). There are already functions like plpy.info() but they only expose message and they call elog instead of ereport. See the attached incompat.py. Running it produces: existing behaviour PG elog/ereport with message: 1: hi detail: None hint: None PG elog/ereport with message: ('2: hi', 'another argument') detail: None hint: None PG elog/ereport with message: ('3: hi', 'another argument', 2) detail: None hint: None PG elog/ereport with message: ('4: hi', 'another argument', 2, 'lots', 'of', 'arguments') detail: None hint: None new behaviour PG elog/ereport with message: 1: hi detail: None hint: None PG elog/ereport with message: 2: hi detail: another argument hint: None PG elog/ereport with message: 3: hi detail: another argument hint: 2 Traceback (most recent call last): File "incompat.py", line 43, in <module> info_new('4: hi', 'another argument', 2, 'lots', 'of', 'arguments') TypeError: info_new() takes at most 3 arguments (6 given) I find existing behaviour for 2, 3 and 4 unlike other Python APIs I've seen, surprising and not very useful. If I want to log a tuple I can construct and pass a single tuple, I don't see why plpy.info() needs to construct it for me. And for the documented, single argument call nothing changes. The question to Bruce (and others) is: is it ok to change to the new behaviour illustrated and change meaning for usages like 2, 3 and 4? If we don't want that, the solution Pavel and I see is introducing a parallel API named plpy.raise_info or plpy.rich_info or something like that with the new behaviour and leave the existing functions unchanged. Another option is some compatibility GUC but I don't think it's worth the trouble and confusion.
Вложения
On 2/4/16 3:13 AM, Catalin Iacob wrote: Thanks for the overview. Very helpful. > I find existing behaviour for 2, 3 and 4 unlike other Python APIs I've > seen, surprising and not very useful. If I want to log a tuple I can > construct and pass a single tuple, I don't see why plpy.info() needs > to construct it for me. And for the documented, single argument call > nothing changes. Agreed, that usage is wonky. > The question to Bruce (and others) is: is it ok to change to the new > behaviour illustrated and change meaning for usages like 2, 3 and 4? If any users have a bunch of code that depends on the old behavior, they're going to be rather irritated if we break it. If we want to depricate it then I think we need a GUC that allows you to get the old behavior back. > If we don't want that, the solution Pavel and I see is introducing a > parallel API named plpy.raise_info or plpy.rich_info or something like > that with the new behaviour and leave the existing functions > unchanged. Another option is some compatibility GUC but I don't think > it's worth the trouble and confusion. If we're going to provide an alternative API, I'd just do plpy.raise(LEVEL, ...). At this point, my vote would be: Add a plpython.ereport_mode GUC that has 3 settings: current (deprecated) behavior, allow ONLY 1 argument, new behavior. The reason for the 1 argument option is it makes it much easier to find code that's still using the old behavior. I think it's also worth having plpy.raise(LEVEL, ...) as an alternative. If folks feel that's overkill then I'd vote to leave the existing behavior alone and just add plpy.raise(LEVEL, ...). -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
I don't think we need to preserve absolutely all the existing behavior to the letter. We do need to preserve the behavior for sensible cases that people might be reasonably be using currently; and if there's anything somewhat obscure but really useful that you currently get by using some clever trick, them we should provide some reasonable way to get that functionality with the new stuff too. But this doesn't mean all existing code must continue to behave in exactly the same way. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi
Actually I imagine that if there's no agreement between author and first
reviewer, there might not *be* a committer in the first place. Perhaps
try to get someone else to think about it and make a decision. It is
possible that some other committer is able to decide by themselves but I
wouldn't count on it.
+1.
FWIW, I'd think it's better to not break backwards compatibility, but I'm also far from a python expert. It might well be worth adding a plpython GUC to control the behavior so that there's a migration path forward, or maybe do something like the 'import __future__' that python is doing to ease migration to python 3.
Iacob is maybe little bit too defensive - but why not. The implementation of GUC should not be hard. I see it as best way now. Tomorrow I'll try to find last versions of this patch in mailbox and try to design compatibility mode.
I don't like too much a introduction of new general function raise (proposed by Jim). It is not consistent with current design and it needs a introduction of enums visible from user level. The work with it isn't too much comfortable. If it will be required, then we have it. The original implementation of this proposal was designed in exactly same style.
Regards
Pavel
On 2/10/16 12:44 PM, Pavel Stehule wrote: > > FWIW, I'd think it's better to not break backwards compatibility, > but I'm also far from a python expert. It might well be worth adding > a plpython GUC to control the behavior so that there's a migration > path forward, or maybe do something like the 'import __future__' > that python is doing to ease migration to python 3. > > > > Iacob is maybe little bit too defensive - but why not. The > implementation of GUC should not be hard. I see it as best way now. > Tomorrow I'll try to find last versions of this patch in mailbox and try > to design compatibility mode. BTW, there's other cases where we're going to face this compatibility issue. The one I know of right now is that current handling of composite types containing arrays in plpython sucks, but there's no way to change that without breaking compatibility. I don't see any good way to handle these compatibility things other than giving each one its own pl-specific GUC, but I figured I'd at least mention it. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
Hi
I am sending new version. Current version does:5. fields: message, detail and hint are implicitly translated to string - it decrease a necessity of legacy mode
Regards
Pavel
Вложения
On Sat, Feb 13, 2016 at 4:26 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > I am sending new version. Current version does: Hello, I had a look and I like the big picture. Tests fail when built against Python 3.5 with stuff like this in regression.diffs: -ERROR: plpy.Error: stop on error -DETAIL: some detail -HINT: some hint -CONTEXT: Traceback (most recent call last): - PL/Python function "elog_test", line 18, in <module> - plpy.error('stop on error', 'some detail','some hint') -PL/Python function "elog_test" +ERROR: could not convert Python Unicode object to bytes +DETAIL: TypeError: bad argument type for built-in operation +CONTEXT: PL/Python function "elog_test" This is related to the use of PyString_AsString and the changed semantics of that in Python 3 (due to the fact that strings are now Unicode objects and so on). Didn't have time to dig more deeply into the exact cause. Similarly, there are alternative expected test outputs that you didn't update, for example src/pl/plpython/expected/plpython_types_3.out so tests fail on some Python versions due to those as well. > 1. the plpy utility functions can use all ErrorData fields, > 2. there are no new functions, > 3. via GUC plpythonu.legacy_custom_exception we can return previous behave, > 4. only exception Error is raised with natural structure - no composite > value spidata. > 5. fields: message, detail and hint are implicitly translated to string - it > decrease a necessity of legacy mode I disagree that 5. is a good idea. I think we should just treat message, detail and hint like the other ones (schema, table etc.). Use s in PyArg_ParseTupleAndKeywords and let the user explicitly cast to a string. Explicit is better than implicit. The way you did it you keep part of the weird old interface which used to cast to string for you. We shouldn't keep warts of the old behaviour, especially with the GUC to ask for the old behaviour. By the way, getting rid of object_to_string and its usage in PLy_output_kw removed some "ERROR: could not convert Python Unicode object to bytes" failures in the tests so I'm quite sure that the usage of PyString_AsString is responsible for those. I don't like that you set legacy_custom_exception=true in some existing tests, probably to avoid changing them to the new behaviour. We should trust that the new behaviour is what we want and the tests should reflect that. If it's too much work, remember we're asking users to do the same work to convert their code. We have elog_test_legacy to test elog_test_legacy=true, the rest of the tests should use legacy_custom_exception=false.
On Tue, Feb 16, 2016 at 5:18 PM, Catalin Iacob <iacobcatalin@gmail.com> wrote: > We have > elog_test_legacy to test elog_test_legacy=true, the rest of the tests > should use legacy_custom_exception=false. This should have been: We have elog_test_legacy to test legacy_custom_exception=true, the rest of the tests should use legacy_custom_exception=false. I noticed the patch isn't registered in the March CF, please do that. It would be a pity to miss 9.6 because of not registering it in time.
2016-02-16 21:06 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
On Tue, Feb 16, 2016 at 5:18 PM, Catalin Iacob <iacobcatalin@gmail.com> wrote:
> We have
> elog_test_legacy to test elog_test_legacy=true, the rest of the tests
> should use legacy_custom_exception=false.
This should have been:
We have elog_test_legacy to test legacy_custom_exception=true, the
rest of the tests should use legacy_custom_exception=false.
I understand - I fixed regress tests for new behave
I noticed the patch isn't registered in the March CF, please do that.
It would be a pity to miss 9.6 because of not registering it in time.
I'll do it. Now, I fixing 3.4 Python code. There are more issues with "could not convert Python Unicode object to bytes"
Regards
Pavel
Hi
2016-02-16 17:18 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
On Sat, Feb 13, 2016 at 4:26 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> I am sending new version. Current version does:
Hello,
I had a look and I like the big picture.
Tests fail when built against Python 3.5 with stuff like this in
regression.diffs:
-ERROR: plpy.Error: stop on error
-DETAIL: some detail
-HINT: some hint
-CONTEXT: Traceback (most recent call last):
- PL/Python function "elog_test", line 18, in <module>
- plpy.error('stop on error', 'some detail','some hint')
-PL/Python function "elog_test"
+ERROR: could not convert Python Unicode object to bytes
+DETAIL: TypeError: bad argument type for built-in operation
+CONTEXT: PL/Python function "elog_test"
fixed - the object serialization fails on Py_None
This is related to the use of PyString_AsString and the changed
semantics of that in Python 3 (due to the fact that strings are now
Unicode objects and so on). Didn't have time to dig more deeply into
the exact cause.
Similarly, there are alternative expected test outputs that you didn't
update, for example src/pl/plpython/expected/plpython_types_3.out so
tests fail on some Python versions due to those as well.
> 1. the plpy utility functions can use all ErrorData fields,
> 2. there are no new functions,
> 3. via GUC plpythonu.legacy_custom_exception we can return previous behave,
> 4. only exception Error is raised with natural structure - no composite
> value spidata.
> 5. fields: message, detail and hint are implicitly translated to string - it
> decrease a necessity of legacy mode
I disagree that 5. is a good idea. I think we should just treat
message, detail and hint like the other ones (schema, table etc.). Use
s in PyArg_ParseTupleAndKeywords and let the user explicitly cast to a
string. Explicit is better than implicit. The way you did it you keep
part of the weird old interface which used to cast to string for you.
We shouldn't keep warts of the old behaviour, especially with the GUC
to ask for the old behaviour.
removed @5
By the way, getting rid of object_to_string and its usage in
PLy_output_kw removed some "ERROR: could not convert Python Unicode
object to bytes" failures in the tests so I'm quite sure that the
usage of PyString_AsString is responsible for those.
I don't like that you set legacy_custom_exception=true in some
existing tests, probably to avoid changing them to the new behaviour.
We should trust that the new behaviour is what we want and the tests
should reflect that. If it's too much work, remember we're asking
users to do the same work to convert their code. We have
elog_test_legacy to test elog_test_legacy=true, the rest of the tests
should use legacy_custom_exception=false.
all regress tests now works in new mode
Regards
Pavel
Вложения
<div dir="ltr"><br /><div class="gmail_extra"><span class=""></span><br /><span class=""></span><div class="gmail_quote"><blockquoteclass="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><spanclass=""> </span>I noticed the patch isn't registered in the March CF, please dothat.<br /> It would be a pity to miss 9.6 because of not registering it in time.<br /></blockquote></div><br /><a href="https://commitfest.postgresql.org/9/525/">https://commitfest.postgresql.org/9/525/</a><br/><br /></div><div class="gmail_extra">Regards<br/><br /></div><div class="gmail_extra">Pavel<br /></div></div>
After I unzip the patch it doesn't apply. patch says: patch: **** Only garbage was found in the patch input. It's a combined diff, the git-diff manual says this about it: Chunk header format is modified to prevent people from accidentally feeding it to patch -p1. Combined diff format was created for review of merge commit changes, and was not meant for apply. Thanks for doing the test changes. It definitely shows the change is big. The tests at least were heavily relying on both the str conversion and on passing more than one argument. Sad face. You're going to hate me but seeing this I changed my mind about 5, requiring all those extra str calls is too much change in behaviour. I was going to propose passing everything through str (so message, detail, hint but also schema, table) but thinking about it some more, I think I have something better. Python 3 has keyword only arguments. It occurs to me they're exactly for "optional extra stuff" like detail, hint etc. Python 2 doesn't have that notion but you can kind of fake it since you get an args tuple and a kwargs dictionary. What about keeping the same behaviour for multiple positional arguments (single one -> make it str, multiple -> use str of the args tuple) and requiring users to pass detail, hint only as keyword arguments? Code wise it would only mean adding PyObject* kw to PLy_output and adding some code to extract detail, hint etc. from kw. This would also allow getting rid of the GUC since backward compatibility is fully preserved. Again, sorry for all the back and forth but it still feels like we haven't nailed the design to something satisfactory. All the tests you needed to change are a hint towards that.
Hi
2016-02-17 7:34 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
After I unzip the patch it doesn't apply.
patch says:
patch: **** Only garbage was found in the patch input.
It's a combined diff, the git-diff manual says this about it:
Chunk header format is modified to prevent people from accidentally
feeding it to patch -p1. Combined diff format was created for review
of merge commit changes, and was not meant for apply.
Thanks for doing the test changes. It definitely shows the change is
big. The tests at least were heavily relying on both the str
conversion and on passing more than one argument. Sad face.
You're going to hate me but seeing this I changed my mind about 5,
requiring all those extra str calls is too much change in behaviour. I
was going to propose passing everything through str (so message,
detail, hint but also schema, table) but thinking about it some more,
I think I have something better.
Ok, it is no problem - this work is +/- research, and there are not direct way often :)
Python 3 has keyword only arguments. It occurs to me they're exactly
for "optional extra stuff" like detail, hint etc.
Python 2 doesn't have that notion but you can kind of fake it since
you get an args tuple and a kwargs dictionary.
I prefer a possibility to use both ways - positional form is shorter, keywords can help with some parameters.
But I cannot to imagine your idea, can you show it in detail?
What about keeping the same behaviour for multiple positional
arguments (single one -> make it str, multiple -> use str of the args
tuple) and requiring users to pass detail, hint only as keyword
arguments? Code wise it would only mean adding PyObject* kw to
PLy_output and adding some code to extract detail, hint etc. from kw.
This would also allow getting rid of the GUC since backward
compatibility is fully preserved.
Again, sorry for all the back and forth but it still feels like we
haven't nailed the design to something satisfactory. All the tests you
needed to change are a hint towards that.
It not any problem. I am thankful for cooperation.
Regards
Pavel
Вложения
On Wed, Feb 17, 2016 at 3:32 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: >> Python 3 has keyword only arguments. It occurs to me they're exactly >> for "optional extra stuff" like detail, hint etc. >> Python 2 doesn't have that notion but you can kind of fake it since >> you get an args tuple and a kwargs dictionary. > > > I prefer a possibility to use both ways - positional form is shorter, > keywords can help with some parameters. > > But I cannot to imagine your idea, can you show it in detail? Sure, what I mean is: plpy.error('msg') # as before produces message 'msg' plpy.error(42) # as before produces message '42', including the conversion of the int to str plpy.error('msg', 'arg 2 is still part of msg') # as before, produces message '('msg', 'arg2 is still part of msg')' # and so on for as many positional arguments, nothing changes # I still think allowing more than one positional argument is unfortunate but for compatibility we keep allowing more # to pass detail you MUST use keyword args to disambiguate "I really want detail" vs. "I have argument 2 which is part of the messsage tuple for compatibility" plpy.error('msg', 42, detail='a detail') # produces message '('msg', 42)' and detail 'a detail' plpy.error('msg', detail=77) # produces message 'msg' and detail '77' so detail is also converted to str just like message for consistency # and so on for the others plpy.error('msg', 42, detail='a detail', hint='a hint') plpy.error('msg', 42, schema='sch') Only keyword arguments are treated specially and we know no existing code has keyword arguments since they didn't work before. Implementation wise, it's something like this but in C: def error(*args, **kwargs): if len(args) == 1: message = str(args[0]) else: message = str(args) # fetch value from dictionary or None if the key is missing detail = kwargs.pop('detail', None) hint = kwargs.pop('hint',None) # use message, detail, hint etc. to raise exception for error and fatal/call ereport for the other levels Is it clear now? What do you think?
2016-02-17 16:54 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
On Wed, Feb 17, 2016 at 3:32 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> Python 3 has keyword only arguments. It occurs to me they're exactly
>> for "optional extra stuff" like detail, hint etc.
>> Python 2 doesn't have that notion but you can kind of fake it since
>> you get an args tuple and a kwargs dictionary.
>
>
> I prefer a possibility to use both ways - positional form is shorter,
> keywords can help with some parameters.
>
> But I cannot to imagine your idea, can you show it in detail?
Sure, what I mean is:
plpy.error('msg') # as before produces message 'msg'
plpy.error(42) # as before produces message '42', including the
conversion of the int to str
plpy.error('msg', 'arg 2 is still part of msg') # as before, produces
message '('msg', 'arg2 is still part of msg')'
# and so on for as many positional arguments, nothing changes
# I still think allowing more than one positional argument is
unfortunate but for compatibility we keep allowing more
# to pass detail you MUST use keyword args to disambiguate "I really
want detail" vs. "I have argument 2 which is part of the messsage
tuple for compatibility"
plpy.error('msg', 42, detail='a detail') # produces message '('msg',
42)' and detail 'a detail'
plpy.error('msg', detail=77) # produces message 'msg' and detail '77'
so detail is also converted to str just like message for consistency
# and so on for the others
plpy.error('msg', 42, detail='a detail', hint='a hint')
plpy.error('msg', 42, schema='sch')
Only keyword arguments are treated specially and we know no existing
code has keyword arguments since they didn't work before.
Implementation wise, it's something like this but in C:
def error(*args, **kwargs):
if len(args) == 1:
message = str(args[0])
else:
message = str(args)
# fetch value from dictionary or None if the key is missing
detail = kwargs.pop('detail', None)
hint = kwargs.pop('hint', None)
# use message, detail, hint etc. to raise exception for error and
fatal/call ereport for the other levels
Is it clear now? What do you think?
it doesn't look badly. Is there any possibility how to emulate it with Python2 ? What do you think about some similar implementation on Python2?
Regards
Pavel
On 2/18/16, Pavel Stehule <pavel.stehule@gmail.com> wrote: > it doesn't look badly. Is there any possibility how to emulate it with > Python2 ? What do you think about some similar implementation on Python2? The example code I gave works as is in Python2. The Python3 keyword only arguments are only syntactic sugar. See https://www.python.org/dev/peps/pep-3102 for the details. But, as the PEP notes, def f(a, b, *, key1, key2) is similar to doing this which also works in Python2 def f(a, b, *ignore, key1, key2): if ignore: raise TypeError('too many positional arguments') For our case, we want to accept any number of positional arguments due to compatibility so we don't need or want the check for 'too many positional arguments'. Note that in both Python 2 and 3, invoking f(1, 2, key1='k1', key2='k2') is just syntactic sugar for constructing the (1, 2) tuple and {'key1': 'k1', 'key2': 'k2'} dict and passing those to f which then unpacks them into a, b, key1 and key2. You see that reflected in the C API where you get PyObject* args and PyObject* kw and you unpack them explicitly with PyArg_ParseTupleAndKeywords or just use tuple and dict calls to peek inside. What you loose by not having Python3 is that you can't use PyArg_ParseTupleAndKeywords and tell it that detail and so on are keywork only arguments. But because we don't want to unpack args we probably couldn't use that anyway even if we wouldn't support Python2. Therefore you need to look inside the kw dictionary manually as my example shows. What we could also do is check that the kw dictionary *only* contains detail, hint and so on and raise a TypeError if it has more things to avoid silently accepting stuff like: plpy.error('message', some_param='abc'). In Python3 PyArg_ParseTupleAndKeywords would ensure that, but we need to do it manually.
2016-02-18 17:59 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
On 2/18/16, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> it doesn't look badly. Is there any possibility how to emulate it with
> Python2 ? What do you think about some similar implementation on Python2?
The example code I gave works as is in Python2.
The Python3 keyword only arguments are only syntactic sugar. See
https://www.python.org/dev/peps/pep-3102 for the details. But, as the
PEP notes,
def f(a, b, *, key1, key2)
is similar to doing this which also works in Python2
def f(a, b, *ignore, key1, key2):
if ignore:
raise TypeError('too many positional arguments')
For our case, we want to accept any number of positional arguments due
to compatibility so we don't need or want the check for 'too many
positional arguments'.
Note that in both Python 2 and 3, invoking f(1, 2, key1='k1',
key2='k2') is just syntactic sugar for constructing the (1, 2) tuple
and {'key1': 'k1', 'key2': 'k2'} dict and passing those to f which
then unpacks them into a, b, key1 and key2. You see that reflected in
the C API where you get PyObject* args and PyObject* kw and you unpack
them explicitly with PyArg_ParseTupleAndKeywords or just use tuple and
dict calls to peek inside.
What you loose by not having Python3 is that you can't use
PyArg_ParseTupleAndKeywords and tell it that detail and so on are
keywork only arguments. But because we don't want to unpack args we
probably couldn't use that anyway even if we wouldn't support Python2.
Therefore you need to look inside the kw dictionary manually as my
example shows. What we could also do is check that the kw dictionary
*only* contains detail, hint and so on and raise a TypeError if it has
more things to avoid silently accepting stuff like:
plpy.error('message', some_param='abc'). In Python3
PyArg_ParseTupleAndKeywords would ensure that, but we need to do it
manually.
It looks like good idea. Last version are not breaking compatibility - and I think so it can works.
I wrote the code, that works on Python2 and Python3
Regards
Pavel
Вложения
On Fri, Feb 19, 2016 at 9:41 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > It looks like good idea. Last version are not breaking compatibility - and > I think so it can works. > > I wrote the code, that works on Python2 and Python3 Hi, I've attached a patch on top of yours with some documentation improvements, feel free to fold it in your next version. I think the concept is good. We're down to code level changes. Most of them are cosmetical, misleading comments and so on but there are some bugs in there as far as I see. In object_to_string you don't need to test for Py_None. PyObject_Str will return NULL on failure and whatever str() returns on the underlying object. No need to special case None. In object_to_string, when you're already in a (so != NULL) block, you can use Py_DECREF instead of XDECREF. object_to_string seems buggy to me: it returns the pointer returned by PyString_AsString which points to the internal buffer of the Python object but it also XDECREFs that object. You seem to be returning a pointer to freed space. get_string_attr seems to have the same issue as object_to_string. In PLy_get_error_data query and position will never be set for plpy.Error since you don't offer them to Python and therefore don't set them in PLy_output_kw. So I think you should remove the code related to them, including the char **query, int *position parameters for PLy_get_error_data. Removing position also allows removing get_int_attr and the need to exercise this function in the tests. You're using PLy_get_spi_sqlerrcode in PLy_get_error_data which makes the spi in the name unsuitable. I would rename it to just PLy_get_sqlerrcode. At least the comment at the top of PLy_get_spi_sqlerrcode needs to change since it now also extracts info from Error not just SPIError. /* set value of string pointer on object field */ comments are weird for a function that gets a value. But those functions need to change anyway since they're buggy (see above). The only change in plpy_plpymodule.h is the removal of an empty line unrelated to this patch, probably from previous versions. You should undo it to leave plpy_plpymodule.h untouched. Why rename PLy_output to PLy_output_kw? It only makes the patch bigger without being an improvement. Maybe you also have this from previous versions. In PLy_output_kw you don't need to check message for NULL, just as sv wasn't checked before. In PLy_output_kw you added a FreeErrorData(edata) which didn't exist before. I'm not familiar with that API but I'm wondering if it's needed or not, good to have it or not etc. In PLy_output_kw you didn't update the "Note: If sv came from PyString_AsString(), it points into storage..." comment which still refers to sv which is now deleted. In PLy_output_kw you removed the "return a legal object so the interpreter will continue on its merry way" comment which might not be the most valuable comment in the world but removing it is still unrelated to this patch. In PLy_output_kw you test for kw != NULL && kw != Py_None. The kw != NULL is definitely needed but I'm quite sure Python will never pass Py_None into it so the != Py_None isn't needed. Can't find a reference to prove this at the moment. Some more tests could be added to exercise more parts of the patch: - check that SPIError is enhanced with all the new things: schema_name, table_name etc. - in the plpy.error call use every keyword argument not just detail and hint: it proves you save and restore every field correctly from the Error fields since that's not exercised by the info call above which does use every argument - use a wrong keyword argument to see it gets rejected with you error message - try some other types than str (like detail=42) for error as well since error goes on another code path than info - a test exercising the "invalid sqlstate" code
Вложения
2016-02-25 7:06 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
PyObject_GetAttrString can returns Py_None, and 3.4's PyString_AsString produce a error "ERROR: could not convert Python Unicode object to bytes" when object is None.
On Fri, Feb 19, 2016 at 9:41 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> It looks like good idea. Last version are not breaking compatibility - and
> I think so it can works.
>
> I wrote the code, that works on Python2 and Python3
Hi,
I've attached a patch on top of yours with some documentation
improvements, feel free to fold it in your next version.
merged
I think the concept is good. We're down to code level changes. Most of
them are cosmetical, misleading comments and so on but there are some
bugs in there as far as I see.
In object_to_string you don't need to test for Py_None. PyObject_Str
will return NULL on failure and whatever str() returns on the
underlying object. No need to special case None.
In object_to_string, when you're already in a (so != NULL) block, you
can use Py_DECREF instead of XDECREF.
fixed
object_to_string seems buggy to me: it returns the pointer returned by
PyString_AsString which points to the internal buffer of the Python
object but it also XDECREFs that object. You seem to be returning a
pointer to freed space.
fixed
get_string_attr seems to have the same issue as object_to_string.
use pstrdup, but the test on Py_None is necessary
PyObject_GetAttrString can returns Py_None, and 3.4's PyString_AsString produce a error "ERROR: could not convert Python Unicode object to bytes" when object is None.
So minimally in "get_string_attr" the test on Py_None is necessary
In PLy_get_error_data query and position will never be set for
plpy.Error since you don't offer them to Python and therefore don't
set them in PLy_output_kw. So I think you should remove the code
related to them, including the char **query, int *position parameters
for PLy_get_error_data. Removing position also allows removing
get_int_attr and the need to exercise this function in the tests.
has sense, removed
You're using PLy_get_spi_sqlerrcode in PLy_get_error_data which makes
the spi in the name unsuitable. I would rename it to just
PLy_get_sqlerrcode. At least the comment at the top of
PLy_get_spi_sqlerrcode needs to change since it now also extracts info
from Error not just SPIError.
renamed
/* set value of string pointer on object field */ comments are weird
for a function that gets a value. But those functions need to change
anyway since they're buggy (see above).
fixed
The only change in plpy_plpymodule.h is the removal of an empty line
unrelated to this patch, probably from previous versions. You should
undo it to leave plpy_plpymodule.h untouched.
fixed
Why rename PLy_output to PLy_output_kw? It only makes the patch bigger
without being an improvement. Maybe you also have this from previous
versions.
renamed to PLy_output only
In PLy_output_kw you don't need to check message for NULL, just as sv
wasn't checked before.
It is not necessary, but I am thinking so it better - it is maybe too defensive - but the message can be taken from more sources than in old code, and one check on NULL is correct
In PLy_output_kw you added a FreeErrorData(edata) which didn't exist
before. I'm not familiar with that API but I'm wondering if it's
needed or not, good to have it or not etc.
The previous code used Python memory for message. Now, I used PostgreSQL memory (via pstrdup), so now I have to call FreeErrorData.
In PLy_output_kw you didn't update the "Note: If sv came from
PyString_AsString(), it points into storage..." comment which still
refers to sv which is now deleted.
I moved Py_XDECREF(so) up, and removed comment
In PLy_output_kw you removed the "return a legal object so the
interpreter will continue on its merry way" comment which might not be
the most valuable comment in the world but removing it is still
unrelated to this patch.
returned
In PLy_output_kw you test for kw != NULL && kw != Py_None. The kw !=
NULL is definitely needed but I'm quite sure Python will never pass
Py_None into it so the != Py_None isn't needed. Can't find a reference
to prove this at the moment.
cleaned
Some more tests could be added to exercise more parts of the patch:
- check that SPIError is enhanced with all the new things:
schema_name, table_name etc.
- in the plpy.error call use every keyword argument not just detail
and hint: it proves you save and restore every field correctly from
the Error fields since that's not exercised by the info call above
which does use every argument
- use a wrong keyword argument to see it gets rejected with you error message
- try some other types than str (like detail=42) for error as well
since error goes on another code path than info
- a test exercising the "invalid sqlstate" code
done
Sending updated version
Regards
Pavel
Вложения
On 2/26/16, Pavel Stehule <pavel.stehule@gmail.com> wrote: > Sending updated version I did some changes on top of your last version as that was easier than commenting about them, see attached. 0001 and 0005 are comment changes. 0002 is really needed, without it the tests fail on Python2.4. 0004 removes more code related to the unused position. 0003 is the most controversial. It removes the ability to pass message as keyword argument. My reasoning was that keyword arguments are usually optional and configure extra aspects of the function call while message is required and fundamental so therefore it should be positional. If you allow it as keyword as well, you have to deal with the ambiguity of writing plpy.info('a message', message='a keyword arg message, does this overwrite the first one or what?'). For the code with my patches on top on I ran the PL/Python tests for 2.4, 2.5, 2.6, 2.7 and 3.5. Everything passed. Can you have a look at the patches, fold the ones you agree with on top of yours and send the final version? With that I think this will be Ready for Committer.
Вложения
0003 is the most controversial. It removes the ability to pass message
as keyword argument. My reasoning was that keyword arguments are
usually optional and configure extra aspects of the function call
while message is required and fundamental so therefore it should be
positional. If you allow it as keyword as well, you have to deal with
the ambiguity of writing plpy.info('a message', message='a keyword arg
message, does this overwrite the first one or what?').
I though about it before and I prefer variant with possibility to enter message as keyword parameter. The advantage of this solution is simple usage dictionary value as parameter with possibility to set all fields.
We can check collision and we can raise a error. Same technique is used in plpgsql:
postgres=# do $$ begin raise warning 'kuku' using message='NAZDAR'; end; $$;
ERROR: RAISE option already specified: MESSAGE
CONTEXT: PL/pgSQL function inline_code_block line 1 at RAISE
postgres=#
postgres=# do $$ begin raise warning 'kuku' using message='NAZDAR'; end; $$;
ERROR: RAISE option already specified: MESSAGE
CONTEXT: PL/pgSQL function inline_code_block line 1 at RAISE
postgres=#
What do you think?
Pavel
For the code with my patches on top on I ran the PL/Python tests for
2.4, 2.5, 2.6, 2.7 and 3.5. Everything passed.
Can you have a look at the patches, fold the ones you agree with on
top of yours and send the final version? With that I think this will
be Ready for Committer.
Hi
2016-02-29 17:53 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com>:
0003 is the most controversial. It removes the ability to pass message
as keyword argument. My reasoning was that keyword arguments are
usually optional and configure extra aspects of the function call
while message is required and fundamental so therefore it should be
positional. If you allow it as keyword as well, you have to deal with
the ambiguity of writing plpy.info('a message', message='a keyword arg
message, does this overwrite the first one or what?').I though about it before and I prefer variant with possibility to enter message as keyword parameter. The advantage of this solution is simple usage dictionary value as parameter with possibility to set all fields.We can check collision and we can raise a error. Same technique is used in plpgsql:
postgres=# do $$ begin raise warning 'kuku' using message='NAZDAR'; end; $$;
ERROR: RAISE option already specified: MESSAGE
CONTEXT: PL/pgSQL function inline_code_block line 1 at RAISE
postgres=#What do you think?Pavel
For the code with my patches on top on I ran the PL/Python tests for
2.4, 2.5, 2.6, 2.7 and 3.5. Everything passed.
Can you have a look at the patches, fold the ones you agree with on
top of yours and send the final version? With that I think this will
be Ready for Committer.
I merged your patches without @3 (many thanks for it). Instead @3 I disallow double message specification (+regress test)
Regards
Pavel
Вложения
On 3/1/16, Pavel Stehule <pavel.stehule@gmail.com> wrote: >> I though about it before and I prefer variant with possibility to enter >> message as keyword parameter. That's also ok, but indeed with a check that it's not specified twice which I see you already added. > I merged your patches without @3 (many thanks for it). Instead @3 I > disallow double message specification (+regress test) Great, welcome. Ran the tests for plpython-enhanced-error-06 again on 2.4 - 2.7 and 3.5 versions and they passed. I see the pfree you added isn't allowed on a NULL pointer but as far as I see message is guaranteed not to be NULL as dgettext never returns NULL. I'll mark this Ready for Committer.
Hi
2016-03-01 18:48 GMT+01:00 Catalin Iacob <iacobcatalin@gmail.com>:
On 3/1/16, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> I though about it before and I prefer variant with possibility to enter
>> message as keyword parameter.
That's also ok, but indeed with a check that it's not specified twice
which I see you already added.
> I merged your patches without @3 (many thanks for it). Instead @3 I
> disallow double message specification (+regress test)
Great, welcome. Ran the tests for plpython-enhanced-error-06 again on
2.4 - 2.7 and 3.5 versions and they passed.
I see the pfree you added isn't allowed on a NULL pointer but as far
as I see message is guaranteed not to be NULL as dgettext never
returns NULL.
I'll mark this Ready for Committer.
Thank you very much
Nice day
Pavel
On Wed, Mar 2, 2016 at 12:01 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: >> I see the pfree you added isn't allowed on a NULL pointer but as far >> as I see message is guaranteed not to be NULL as dgettext never >> returns NULL. >> >> I'll mark this Ready for Committer. > > Thank you very much This patch needs a committer. Any volunteers? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> Thank you very much thank you, pushed. Pls, pay attention to buildfarm. -- Teodor Sigaev E-mail: teodor@sigaev.ru WWW: http://www.sigaev.ru/
2016-04-08 17:38 GMT+02:00 Teodor Sigaev <teodor@sigaev.ru>:
Thank you very much
thank you, pushed. Pls, pay attention to buildfarm.
Thank you very much for commit.
And big thanks to Iacob for big help.
Regards
Pavel
--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/
Pavel Stehule <pavel.stehule@gmail.com> writes: > 2016-04-08 17:38 GMT+02:00 Teodor Sigaev <teodor@sigaev.ru>: >> thank you, pushed. Pls, pay attention to buildfarm. > Thank you very much for commit. According to buildfarm member prairiedog, there's a problem in one of the test cases. I suspect that it's using syntax that doesn't exist in Python 2.3, but don't know enough Python to fix it. Please submit a correction -- we are not moving the goalposts on Python version compatibility for the convenience of one test case. regards, tom lane
2016-04-08 20:52 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2016-04-08 17:38 GMT+02:00 Teodor Sigaev <teodor@sigaev.ru>:
>> thank you, pushed. Pls, pay attention to buildfarm.
> Thank you very much for commit.
According to buildfarm member prairiedog, there's a problem in one
of the test cases. I suspect that it's using syntax that doesn't
exist in Python 2.3, but don't know enough Python to fix it.
Please submit a correction -- we are not moving the goalposts on
Python version compatibility for the convenience of one test case.
I'll fix it.
Regards
Pavel
regards, tom lane
Hi
2016-04-08 20:54 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:
2016-04-08 20:52 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2016-04-08 17:38 GMT+02:00 Teodor Sigaev <teodor@sigaev.ru>:
>> thank you, pushed. Pls, pay attention to buildfarm.
> Thank you very much for commit.
According to buildfarm member prairiedog, there's a problem in one
of the test cases. I suspect that it's using syntax that doesn't
exist in Python 2.3, but don't know enough Python to fix it.
Please submit a correction -- we are not moving the goalposts on
Python version compatibility for the convenience of one test case.I'll fix it.
here is patch
I found related discussion - http://postgresql.nabble.com/Minimum-supported-version-of-Python-td5796175.html
Regards
Pavel
RegardsPavel
regards, tom lane
Вложения
Pavel Stehule <pavel.stehule@gmail.com> writes: > here is patch Pushed, thanks. regards, tom lane
I noticed that this new feature in PL/Python gratuitously uses slightly different keyword names than the C and PL/pgSQL APIs, e.g. "schema" instead of "schema_name" etc. I propose to fix that with the attached patch. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
2016-06-10 20:56 GMT+02:00 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>:
I noticed that this new feature in PL/Python gratuitously uses slightly different keyword names than the C and PL/pgSQL APIs, e.g. "schema" instead of "schema_name" etc. I propose to fix that with the attached patch.
good idea
+1
+1
Pavel
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services