Обсуждение: proposal: PL/Pythonu - function ereport

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.

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

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.

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.

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.

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.

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.

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.

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

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

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.

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/
 

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

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?

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.

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.

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.

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

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

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.

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?

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.

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

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

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

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.

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

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?

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.

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

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.

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.

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/
 

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

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