Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2016-06-10 20:56 GMT+02:00 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com>:

> I noticed that this new feature in PL/Python gratuitously uses slightly
> different keyword names than the C and PL/pgSQL APIs, e.g. "schema" instead
> of "schema_name" etc.  I propose to fix that with the attached patch.
>
>
good idea

+1

Pavel


> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Peter Eisentraut]

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
From 1622e74f695fbcee4d9ade1c8736001d3adc6b64 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 10 Jun 2016 14:38:20 -0400
Subject: [PATCH] PL/Python: Rename new keyword arguments of plpy.error() etc.

Rename schema -> schema_name etc. to remaining consistent with C API and
PL/pgSQL.
---
 doc/src/sgml/plpython.sgml|  6 +--
 src/pl/plpython/expected/plpython_ereport.out | 66 -
 src/pl/plpython/plpy_plpymodule.c | 70 +--
 src/pl/plpython/sql/plpython_ereport.sql  | 62 
 4 files changed, 102 insertions(+), 102 deletions(-)

diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
index cff66a2..abc11a9 100644
--- a/doc/src/sgml/plpython.sgml
+++ b/doc/src/sgml/plpython.sgml
@@ -1374,9 +1374,9 @@ Utility Functions
The following keyword-only arguments are accepted:

detail, hint,
-   sqlstate, schema,
-   table, column,
-   datatype , constraint
+   sqlstate, schema_name,
+   table_name, 
column_name,
+   datatype_name , 
constraint_name
.
The string representation of the objects passed as keyword-only arguments
is used to enrich the messages reported to the client. For example:
diff --git a/src/pl/plpython/expected/plpython_ereport.out 
b/src/pl/plpython/expected/plpython_ereport.out
index 8a6dfe4..e32b672 100644
--- a/src/pl/plpython/expected/plpython_ereport.out
+++ b/src/pl/plpython/expected/plpython_ereport.out
@@ -9,11 +9,11 @@ plpy.info('This is message text.',
 detail = 'This is detail text',
 hint = 'This is hint text.',
 sqlstate = 'XX000',
-schema = 'any info about schema',
-table = 'any info about table',
-column = 'any info about column',
-datatype = 'any info about datatype',
-constraint = 'any info about constraint')
+schema_name = 'any info about schema',
+table_name = 'any info about table',
+column_name = 'any info about column',
+datatype_name = 'any info about datatype',
+constraint_name = 'any info about constraint')
 plpy.notice('notice', detail = 'some detail')
 plpy.warning('warning', detail = 'some detail')
 plpy.error('stop on error', detail = 'some detail', hint = 'some hint')
@@ -70,12 +70,12 @@ CONTEXT:  PL/Python anonymous code block
 -- raise exception in python, handle exception in plgsql
 CREATE OR REPLACE FUNCTION raise_exception(_message text, _detail text DEFAULT 
NULL, _hint text DEFAULT NULL,
_sqlstate text DEFAULT NULL,
-   _schema text DEFAULT NULL, 
_table text DEFAULT NULL, _column text DEFAULT NULL,
-   _datatype text DEFAULT NULL, 
_constraint text DEFAULT NULL)
+   _schema_name text DEFAULT NULL, 
_table_name text DEFAULT NULL, _column_name text DEFAULT NULL,
+   _datatype_name text DEFAULT 
NULL, _constraint_name text DEFAULT NULL)
 RETURNS void AS $$
 kwargs = { "message":_message, "detail":_detail, "hint":_hint,
-   "sqlstate":_sqlstate, "schema":_schema, "table":_table,
-   "column":_column, "datatype":_datatype, 
"constraint":_constraint }
+   "sqlstate":_sqlstate, "schema_name":_schema_name, 
"table_name":_table_name,
+   "column_name":_column_name, 
"datatype_name":_datatype_name, "constraint_name":_constraint_name }
 # ignore None values - should work on Python2.3
 dict = {}
 for k in kwargs:
@@ -101,11 +101,11 @@ SELECT raise_exception(_message => 'message text',
_detail => 'detail text',
_hint => 'hint text',
_sqlstate => 'XX555',
-   _schema => 'schema text',
-   _table => 'table text',
-   _column => 'column text',
-   _datatype => 'datatype text',
-   _constraint => 'constraint 
text');
+   _schema_name => 'schema text',
+   _table_name => 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Tom Lane]

Pavel Stehule  writes:
> here is patch

Pushed, thanks.

regards, tom lane


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

Hi

2016-04-08 20:54 GMT+02:00 Pavel Stehule :

>
>
> 2016-04-08 20:52 GMT+02:00 Tom Lane :
>
>> Pavel Stehule  writes:
>> > 2016-04-08 17:38 GMT+02:00 Teodor Sigaev :
>> >> thank you, pushed. Pls, pay attention to buildfarm.
>>
>> > Thank you very much for commit.
>>
>> According to buildfarm member prairiedog, there's a problem in one
>> of the test cases.  I suspect that it's using syntax that doesn't
>> exist in Python 2.3, but don't know enough Python to fix it.
>>
>> Please submit a correction -- we are not moving the goalposts on
>> Python version compatibility for the convenience of one test case.
>>
>
> I'll fix it.
>
>
here is patch

I found related discussion -
http://postgresql.nabble.com/Minimum-supported-version-of-Python-td5796175.html

Regards

Pavel


> Regards
>
> Pavel
>
>>
>> regards, tom lane
>>
>
>
diff --git a/src/pl/plpython/expected/plpython_test.out b/src/pl/plpython/expected/plpython_test.out
new file mode 100644
index 05caba1..bc7707d
*** a/src/pl/plpython/expected/plpython_test.out
--- b/src/pl/plpython/expected/plpython_test.out
*** RETURNS void AS $$
*** 150,171 
  kwargs = { "message":_message, "detail":_detail, "hint":_hint,
  			"sqlstate":_sqlstate, "schema":_schema, "table":_table,
  			"column":_column, "datatype":_datatype, "constraint":_constraint }
! # ignore None values
! plpy.error(**dict((k, v) for k, v in iter(kwargs.items()) if v))
  $$ LANGUAGE plpythonu;
  SELECT raise_exception('hello', 'world');
  ERROR:  plpy.Error: hello
  DETAIL:  world
  CONTEXT:  Traceback (most recent call last):
!   PL/Python function "raise_exception", line 6, in 
! plpy.error(**dict((k, v) for k, v in iter(kwargs.items()) if v))
  PL/Python function "raise_exception"
  SELECT raise_exception('message text', 'detail text', _sqlstate => 'YY333');
  ERROR:  plpy.Error: message text
  DETAIL:  detail text
  CONTEXT:  Traceback (most recent call last):
!   PL/Python function "raise_exception", line 6, in 
! plpy.error(**dict((k, v) for k, v in iter(kwargs.items()) if v))
  PL/Python function "raise_exception"
  SELECT raise_exception(_message => 'message text',
  		_detail => 'detail text',
--- 150,175 
  kwargs = { "message":_message, "detail":_detail, "hint":_hint,
  			"sqlstate":_sqlstate, "schema":_schema, "table":_table,
  			"column":_column, "datatype":_datatype, "constraint":_constraint }
! # ignore None values - should to work on Python2.3
! dict = {}
! for k in kwargs:
! 	if kwargs[k] is not None:
! 		dict[k] = kwargs[k]
! plpy.error(**dict)
  $$ LANGUAGE plpythonu;
  SELECT raise_exception('hello', 'world');
  ERROR:  plpy.Error: hello
  DETAIL:  world
  CONTEXT:  Traceback (most recent call last):
!   PL/Python function "raise_exception", line 10, in 
! plpy.error(**dict)
  PL/Python function "raise_exception"
  SELECT raise_exception('message text', 'detail text', _sqlstate => 'YY333');
  ERROR:  plpy.Error: message text
  DETAIL:  detail text
  CONTEXT:  Traceback (most recent call last):
!   PL/Python function "raise_exception", line 10, in 
! plpy.error(**dict)
  PL/Python function "raise_exception"
  SELECT raise_exception(_message => 'message text',
  		_detail => 'detail text',
*** ERROR:  plpy.Error: message text
*** 180,187 
  DETAIL:  detail text
  HINT:  hint text
  CONTEXT:  Traceback (most recent call last):
!   PL/Python function "raise_exception", line 6, in 
! plpy.error(**dict((k, v) for k, v in iter(kwargs.items()) if v))
  PL/Python function "raise_exception"
  SELECT raise_exception(_message => 'message text',
  		_hint => 'hint text',
--- 184,191 
  DETAIL:  detail text
  HINT:  hint text
  CONTEXT:  Traceback (most recent call last):
!   PL/Python function "raise_exception", line 10, in 
! plpy.error(**dict)
  PL/Python function "raise_exception"
  SELECT raise_exception(_message => 'message text',
  		_hint => 'hint text',
*** SELECT raise_exception(_message => 'mess
*** 191,198 
  ERROR:  plpy.Error: message text
  HINT:  hint text
  CONTEXT:  Traceback (most recent call last):
!   PL/Python function "raise_exception", line 6, in 
! plpy.error(**dict((k, v) for k, v in iter(kwargs.items()) if v))
  PL/Python function "raise_exception"
  DO $$
  DECLARE
--- 195,202 
  ERROR:  plpy.Error: message text
  HINT:  hint text
  CONTEXT:  Traceback (most recent call last):
!   PL/Python function "raise_exception", line 10, in 
! plpy.error(**dict)
  PL/Python function "raise_exception"
  DO $$
  DECLARE
diff --git a/src/pl/plpython/sql/plpython_test.sql b/src/pl/plpython/sql/plpython_test.sql
new file mode 100644
index 6e5b535..294f2dd
*** a/src/pl/plpython/sql/plpython_test.sql
--- b/src/pl/plpython/sql/plpython_test.sql
*** RETURNS void AS $$
*** 102,109 
  kwargs = { "message":_message, "detail":_detail, "hint":_hint,
  			

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2016-04-08 20:52 GMT+02:00 Tom Lane :

> Pavel Stehule  writes:
> > 2016-04-08 17:38 GMT+02:00 Teodor Sigaev :
> >> thank you, pushed. Pls, pay attention to buildfarm.
>
> > Thank you very much for commit.
>
> According to buildfarm member prairiedog, there's a problem in one
> of the test cases.  I suspect that it's using syntax that doesn't
> exist in Python 2.3, but don't know enough Python to fix it.
>
> Please submit a correction -- we are not moving the goalposts on
> Python version compatibility for the convenience of one test case.
>

I'll fix it.

Regards

Pavel

>
> regards, tom lane
>

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Tom Lane]

Pavel Stehule  writes:
> 2016-04-08 17:38 GMT+02:00 Teodor Sigaev :
>> 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


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2016-04-08 17:38 GMT+02:00 Teodor Sigaev :

> Thank you very much
>>
>
> thank you, pushed. Pls, pay attention to buildfarm.
>

Thank you very much for commit.

And big thanks to Iacob for big help.

Regards

Pavel



>
> --
> Teodor Sigaev   E-mail: teo...@sigaev.ru
>WWW:
> http://www.sigaev.ru/
>

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Teodor Sigaev]

Thank you very much


thank you, pushed. Pls, pay attention to buildfarm.

--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Robert Haas]

On Wed, Mar 2, 2016 at 12:01 AM, Pavel Stehule  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


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

Hi

2016-03-01 18:48 GMT+01:00 Catalin Iacob :

> On 3/1/16, Pavel Stehule  wrote:
> >> I though about it before and I prefer variant with possibility to enter
> >> message as keyword parameter.
>
> That's also ok, but indeed with a check that it's not specified twice
> which I see you already added.
>
> > I merged your patches without @3 (many thanks for it). Instead @3 I
> > disallow double message specification (+regress test)
>
> Great, welcome. Ran the tests for plpython-enhanced-error-06 again on
> 2.4 - 2.7 and 3.5 versions and they passed.
>
> I see the pfree you added isn't allowed on a NULL pointer but as far
> as I see message is guaranteed not to be NULL as dgettext never
> returns NULL.
>
> I'll mark this Ready for Committer.
>

Thank you very much

Nice day

Pavel

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On 3/1/16, Pavel Stehule  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.


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

Hi

2016-02-29 17:53 GMT+01:00 Pavel Stehule :

>
>
>
>> 0003 is the most controversial. It removes the ability to pass message
>> as keyword argument. My reasoning was that keyword arguments are
>> usually optional and configure extra aspects of the function call
>> while message is required and fundamental so therefore it should be
>> positional. If you allow it as keyword as well, you have to deal with
>> the ambiguity of writing plpy.info('a message', message='a keyword arg
>> message, does this overwrite the first one or what?').
>>
>
> I though about it before and I prefer variant with possibility to enter
> message as keyword parameter. The advantage of this solution is simple
> usage dictionary value as parameter with possibility to set all fields.
>
> We can check collision and we can raise a error. Same technique is used in
> plpgsql:
>
> postgres=# do $$ begin raise warning 'kuku' using message='NAZDAR'; end;
> $$;
> ERROR:  RAISE option already specified: MESSAGE
> CONTEXT:  PL/pgSQL function inline_code_block line 1 at RAISE
> postgres=#
>
> What do you think?
>
> Pavel
>
>
>>
>> For the code with my patches on top on I ran the PL/Python tests for
>> 2.4, 2.5, 2.6, 2.7 and 3.5. Everything passed.
>>
>> Can you have a look at the patches, fold the ones you agree with on
>> top of yours and send the final version? With that I think this will
>> be Ready for Committer.
>>
>
>
I merged your patches without @3 (many thanks for it). Instead @3 I
disallow double message specification (+regress test)

Regards

Pavel
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index 015bbad..4ed34d6
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 1341,1353 
Utility Functions

 The plpy module also provides the functions
!plpy.debug(msg),
!plpy.log(msg),
!plpy.info(msg),
!plpy.notice(msg),
!plpy.warning(msg),
!plpy.error(msg), and
!plpy.fatal(msg).elogin PL/Python
 plpy.error and
 plpy.fatal actually raise a Python exception
 which, if uncaught, propagates out to the calling query, causing
--- 1341,1353 
Utility Functions

 The plpy module also provides the functions
!plpy.debug(msg, **kwargs),
!plpy.log(msg, **kwargs),
!plpy.info(msg, **kwargs),
!plpy.notice(msg, **kwargs),
!plpy.warning(msg, **kwargs),
!plpy.error(msg, **kwargs), and
!plpy.fatal(msg, **kwargs).elogin PL/Python
 plpy.error and
 plpy.fatal actually raise a Python exception
 which, if uncaught, propagates out to the calling query, causing
*** $$ LANGUAGE plpythonu;
*** 1355,1362 
 raise plpy.Error(msg) and
 raise plpy.Fatal(msg) are
 equivalent to calling
!plpy.error and
!plpy.fatal, respectively.
 The other functions only generate messages of different
 priority levels.
 Whether messages of a particular priority are reported to the client,
--- 1355,1363 
 raise plpy.Error(msg) and
 raise plpy.Fatal(msg) are
 equivalent to calling
!plpy.error(msg) and
!plpy.fatal(msg), respectively but
!the raise form does not allow passing keyword arguments.
 The other functions only generate messages of different
 priority levels.
 Whether messages of a particular priority are reported to the client,
*** $$ LANGUAGE plpythonu;
*** 1367,1372 
--- 1368,1401 

  

+ 
+The msg argument is given as a positional argument.  For
+backward compatibility, more than one positional argument can be given. In
+that case, the string representation of the tuple of positional arguments
+becomes the message reported to the client.
+The following keyword-only arguments are accepted:
+detail, hint, sqlstate, schema, table, column, datatype , constraint.
+The string representation of the objects passed as keyword-only arguments
+is used to enrich the messages reported to the client. For example:
+ 
+ 
+ CREATE FUNCTION raise_custom_exception() RETURNS void AS $$
+ plpy.error("custom exception message", detail = "some info about exception", hint = "hint for users")
+ $$ LANGUAGE plpythonu;
+ 
+ postgres=# select raise_custom_exception();
+ ERROR:  XX000: plpy.Error: custom exception message
+ DETAIL:  some info about exception
+ HINT:  hint for users
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python function "raise_custom_exception", line 2, in module
+ plpy.error("custom exception message", detail = "some info about exception", hint = "hint for users")
+ PL/Python function "raise_custom_exception"
+ LOCATION:  PLy_elog, plpy_elog.c:132
+ 
+   
+ 
+   
 Another set of utility functions are
 plpy.quote_literal(string),
 plpy.quote_nullable(string), and
diff --git a/src/pl/plpython/expected/plpython_test.out b/src/pl/plpython/expected/plpython_test.out
new file mode 100644
index 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

> 0003 is the most controversial. It removes the ability to pass message
> as keyword argument. My reasoning was that keyword arguments are
> usually optional and configure extra aspects of the function call
> while message is required and fundamental so therefore it should be
> positional. If you allow it as keyword as well, you have to deal with
> the ambiguity of writing plpy.info('a message', message='a keyword arg
> message, does this overwrite the first one or what?').
>

I though about it before and I prefer variant with possibility to enter
message as keyword parameter. The advantage of this solution is simple
usage dictionary value as parameter with possibility to set all fields.

We can check collision and we can raise a error. Same technique is used in
plpgsql:

postgres=# do $$ begin raise warning 'kuku' using message='NAZDAR'; end; $$;
ERROR:  RAISE option already specified: MESSAGE
CONTEXT:  PL/pgSQL function inline_code_block line 1 at RAISE
postgres=#

What do you think?

Pavel


>
> For the code with my patches on top on I ran the PL/Python tests for
> 2.4, 2.5, 2.6, 2.7 and 3.5. Everything passed.
>
> Can you have a look at the patches, fold the ones you agree with on
> top of yours and send the final version? With that I think this will
> be Ready for Committer.
>

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On 2/26/16, Pavel Stehule  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.


0001-Comment-improvements.patch
Description: binary/octet-stream


0002-Fix-tests-for-Python-2.4.patch
Description: binary/octet-stream


0003-Pass-message-only-as-positional-argument.patch
Description: binary/octet-stream


0004-Get-rid-of-setting-unused-position.patch
Description: binary/octet-stream


0005-Improve-comments.patch
Description: binary/octet-stream

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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2016-02-25 7:06 GMT+01:00 Catalin Iacob :

> On Fri, Feb 19, 2016 at 9:41 PM, Pavel Stehule 
> wrote:
> >  It looks like good idea. Last version are not breaking compatibility -
> and
> > I think so it can works.
> >
> > I wrote the code, that works on Python2 and Python3
>
> Hi,
>
> I've attached a patch on top of yours with some documentation
> improvements, feel free to fold it in your next version.
>

merged


>
> I think the concept is good. We're down to code level changes. Most of
> them are cosmetical, misleading comments and so on but there are some
> bugs in there as far as I see.
>
>
> In object_to_string you don't need to test for Py_None. PyObject_Str
> will return NULL on failure and whatever str() returns on the
> underlying object. No need to special case None.
>
> In object_to_string, when you're already in a (so != NULL) block, you
> can use Py_DECREF instead of XDECREF.
>

fixed


>
> object_to_string seems buggy to me: it returns the pointer returned by
> PyString_AsString which points to the internal buffer of the Python
> object but it also XDECREFs that object. You seem to be returning a
> pointer to freed space.
>

fixed


>
> get_string_attr seems to have the same issue as object_to_string.
>

use pstrdup, but the test on Py_None is necessary


PyObject_GetAttrString can returns Py_None, and 3.4's PyString_AsString
produce a error "ERROR:  could not convert Python Unicode object to bytes"
when object is None.

So minimally in "get_string_attr" the test on Py_None is necessary


>
> In PLy_get_error_data query and position will never be set for
> plpy.Error since you don't offer them to Python and therefore don't
> set them in PLy_output_kw. So I think you should remove the code
> related to them, including the char **query, int *position parameters
> for PLy_get_error_data. Removing position also allows removing
> get_int_attr and the need to exercise this function in the tests.
>

has sense, removed


>
> You're using PLy_get_spi_sqlerrcode in PLy_get_error_data which makes
> the spi in the name unsuitable. I would rename it to just
> PLy_get_sqlerrcode. At least the comment at the top of
> PLy_get_spi_sqlerrcode needs to change since it now also extracts info
> from Error not just SPIError.
>

renamed


>
> /* set value of string pointer on object field */ comments are weird
> for a function that gets a value. But those functions need to change
> anyway since they're buggy (see above).
>

fixed


>
> The only change in plpy_plpymodule.h is the removal of an empty line
> unrelated to this patch, probably from previous versions. You should
> undo it to leave plpy_plpymodule.h untouched.
>

fixed

>
> Why rename PLy_output to PLy_output_kw? It only makes the patch bigger
> without being an improvement. Maybe you also have this from previous
> versions.
>

renamed to PLy_output only


>
> In PLy_output_kw you don't need to check message for NULL, just as sv
> wasn't checked before.
>

It is not necessary, but I am thinking so it better - it is maybe too
defensive - but the message can be taken from more sources than in old
code, and one check on NULL is correct


> In PLy_output_kw you added a FreeErrorData(edata) which didn't exist
> before. I'm not familiar with that API but I'm wondering if it's
> needed or not, good to have it or not etc.
>

The previous code used Python memory for message. Now, I used PostgreSQL
memory (via pstrdup), so now I have to call FreeErrorData.



>
> In PLy_output_kw you didn't update the "Note: If sv came from
> PyString_AsString(), it points into storage..." comment which still
> refers to sv which is now deleted.
>

I moved Py_XDECREF(so) up, and removed comment


>
> In PLy_output_kw you removed the "return a legal object so the
> interpreter will continue on its merry way" comment which might not be
> the most valuable comment in the world but removing it is still
> unrelated to this patch.
>

returned


>
> In PLy_output_kw you test for kw != NULL && kw != Py_None. The kw !=
> NULL is definitely needed but I'm quite sure Python will never pass
> Py_None into it so the != Py_None isn't needed. Can't find a reference
> to prove this at the moment.
>

cleaned

>
>
> Some more tests could be added to exercise more parts of the patch:
> - check that SPIError is enhanced with all the new things:
> schema_name, table_name etc.
> - in the plpy.error call use every keyword argument not just detail
> and hint: it proves you save and restore every field correctly from
> the Error fields since that's not exercised by the info call above
> which does use every argument
>



> - use a wrong keyword argument to see it gets rejected with you error
> message
> - try some other types than str (like detail=42) for error as well
> since error goes on another code path than info
> - a test exercising the "invalid sqlstate" code
>

done

Sending updated version

Regards

Pavel
diff --git a/doc/src/sgml/plpython.sgml 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Fri, Feb 19, 2016 at 9:41 PM, Pavel Stehule  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


0001-Doc-improvements.patch
Description: binary/octet-stream

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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2016-02-18 17:59 GMT+01:00 Catalin Iacob :

> On 2/18/16, Pavel Stehule  wrote:
> > it doesn't look badly. Is there any possibility how to emulate it with
> > Python2 ? What do you think about some similar implementation on Python2?
>
> The example code I gave works as is in Python2.
>
> The Python3 keyword only arguments are only syntactic sugar. See
> https://www.python.org/dev/peps/pep-3102 for the details. But, as the
> PEP notes,
>
> def f(a, b, *, key1, key2)
>
> is similar to doing this which also works in Python2
>
> def f(a, b, *ignore, key1, key2):
> if ignore:
>raise TypeError('too many positional arguments')
>
> For our case, we want to accept any number of positional arguments due
> to compatibility so we don't need or want the check for 'too many
> positional arguments'.
>
> Note that in both Python 2 and 3, invoking f(1, 2, key1='k1',
> key2='k2') is just syntactic sugar for constructing the (1, 2) tuple
> and {'key1': 'k1', 'key2': 'k2'} dict and passing those to f which
> then unpacks them into a, b, key1 and key2. You see that reflected in
> the C API where you get PyObject* args and PyObject* kw and you unpack
> them explicitly with PyArg_ParseTupleAndKeywords or just use tuple and
> dict calls to peek inside.
>
> What you loose by not having Python3 is that you can't use
> PyArg_ParseTupleAndKeywords and tell it that detail and so on are
> keywork only arguments. But because we don't want to unpack args we
> probably couldn't use that anyway even if we wouldn't support Python2.
>
> Therefore you need to look inside the kw dictionary manually as my
> example shows. What we could also do is check that the kw dictionary
> *only* contains detail, hint and so on and raise a TypeError if it has
> more things to avoid silently accepting stuff like:
> plpy.error('message', some_param='abc'). In Python3
> PyArg_ParseTupleAndKeywords would ensure that, but we need to do it
> manually.
>

 It looks like good idea. Last version are not breaking compatibility - and
I think so it can works.

I wrote the code, that works on Python2 and Python3

Regards

Pavel
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index 015bbad..7d31342
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 1341,1360 
Utility Functions

 The plpy module also provides the functions
!plpy.debug(msg),
!plpy.log(msg),
!plpy.info(msg),
!plpy.notice(msg),
!plpy.warning(msg),
!plpy.error(msg), and
!plpy.fatal(msg).elogin PL/Python
 plpy.error and
 plpy.fatal actually raise a Python exception
 which, if uncaught, propagates out to the calling query, causing
 the current transaction or subtransaction to be aborted.
 raise plpy.Error(msg) and
 raise plpy.Fatal(msg) are
!equivalent to calling
 plpy.error and
 plpy.fatal, respectively.
 The other functions only generate messages of different
--- 1341,1360 
Utility Functions

 The plpy module also provides the functions
!plpy.debug(exception_params),
!plpy.log(exception_params),
!plpy.info(exception_params),
!plpy.notice(exception_params),
!plpy.warning(exception_params),
!plpy.error(exception_params), and
!plpy.fatal(exception_params).elogin PL/Python
 plpy.error and
 plpy.fatal actually raise a Python exception
 which, if uncaught, propagates out to the calling query, causing
 the current transaction or subtransaction to be aborted.
 raise plpy.Error(msg) and
 raise plpy.Fatal(msg) are
!partial equivalent to calling
 plpy.error and
 plpy.fatal, respectively.
 The other functions only generate messages of different
*** $$ LANGUAGE plpythonu;
*** 1367,1372 
--- 1367,1397 

  

+ 
+The exception_params are
+[ message [, detail [, hint [, sqlstate  [, schema  [, table  [, column  [, datatype  [, constraint ].
+These parameters kan be entered as keyword parameters.
+The message, detail, hint
+are automaticly casted to string, other should be string.
+ 
+ 
+ CREATE FUNCTION raise_custom_exception() RETURNS void AS $$
+ plpy.error("custom exception message", detail = "some info about exception", hint = "hint for users")
+ $$ LANGUAGE plpythonu;
+ 
+ postgres=# select raise_custom_exception();
+ ERROR:  XX000: plpy.Error: custom exception message
+ DETAIL:  some info about exception
+ HINT:  hint for users
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python function "raise_custom_exception", line 2, in module
+ plpy.error("custom exception message", detail = "some info about exception", hint = "hint for users")
+ PL/Python function "raise_custom_exception"
+ LOCATION:  PLy_elog, plpy_elog.c:132
+ 
+   
+ 
+   
 Another set of utility functions are
 plpy.quote_literal(string),
 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On 2/18/16, Pavel Stehule  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.


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2016-02-17 16:54 GMT+01:00 Catalin Iacob :

> On Wed, Feb 17, 2016 at 3:32 PM, Pavel Stehule 
> wrote:
> >> Python 3 has keyword only arguments. It occurs to me they're exactly
> >> for "optional extra stuff" like detail, hint etc.
> >> Python 2 doesn't have that notion but you can kind of fake it since
> >> you get an args tuple and a kwargs dictionary.
> >
> >
> > I prefer a possibility to use both ways - positional form is shorter,
> > keywords can help with some parameters.
> >
> > But I cannot to imagine your idea, can you show it in detail?
>
> Sure, what I mean is:
>
> plpy.error('msg') # as before produces message 'msg'
> plpy.error(42) # as before produces message '42', including the
> conversion of the int to str
> plpy.error('msg', 'arg 2 is still part of msg') # as before, produces
> message '('msg', 'arg2 is still part of msg')'
> # and so on for as many positional arguments, nothing changes
> # I still think allowing more than one positional argument is
> unfortunate but for compatibility we keep allowing more
>
> # to pass detail you MUST use keyword args to disambiguate "I really
> want detail" vs. "I have argument 2 which is part of the messsage
> tuple for compatibility"
> plpy.error('msg', 42, detail='a detail') # produces message '('msg',
> 42)' and detail 'a detail'
> plpy.error('msg', detail=77) # produces message 'msg' and detail '77'
> so detail is also converted to str just like message for consistency
> # and so on for the others
> plpy.error('msg', 42, detail='a detail', hint='a hint')
> plpy.error('msg', 42, schema='sch')
>
> Only keyword arguments are treated specially and we know no existing
> code has keyword arguments since they didn't work before.
>
> Implementation wise, it's something like this but in C:
>
> def error(*args, **kwargs):
> if len(args) == 1:
> message = str(args[0])
> else:
> message = str(args)
>
> # fetch value from dictionary or None if the key is missing
> detail = kwargs.pop('detail', None)
> hint = kwargs.pop('hint', None)
>
> # use message, detail, hint etc. to raise exception for error and
> fatal/call ereport for the other levels
>
> Is it clear now? What do you think?
>

it doesn't look badly. Is there any possibility how to emulate it with
Python2 ? What do you think about some similar implementation on Python2?

Regards

Pavel

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Wed, Feb 17, 2016 at 3:32 PM, Pavel Stehule  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?


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

Hi

2016-02-17 7:34 GMT+01:00 Catalin Iacob :

> After I unzip the patch it doesn't apply.
> patch says:
> patch:  Only garbage was found in the patch input.
>
> It's a combined diff, the git-diff manual says this about it:
> Chunk header format is modified to prevent people from accidentally
> feeding it to patch -p1. Combined diff format was created for review
> of merge commit changes, and was not meant for apply.
>
> Thanks for doing the test changes. It definitely shows the change is
> big. The tests at least were heavily relying on both the str
> conversion and on passing more than one argument. Sad face.


> You're going to hate me but seeing this I changed my mind about 5,
> requiring all those extra str calls is too much change in behaviour. I
> was going to propose passing everything through str (so message,
> detail, hint but also schema, table) but thinking about it some more,
> I think I have something better.
>

Ok, it is no problem - this work is +/- research, and there are not direct
way often :)


>
> Python 3 has keyword only arguments. It occurs to me they're exactly
> for "optional extra stuff" like detail, hint etc.
> Python 2 doesn't have that notion but you can kind of fake it since
> you get an args tuple and a kwargs dictionary.
>

I prefer a possibility to use both ways - positional form is shorter,
keywords can help with some parameters.

But I cannot to imagine your idea, can you show it in detail?


>
> What about keeping the same behaviour for multiple positional
> arguments (single one -> make it str, multiple -> use str of the args
> tuple) and requiring users to pass detail, hint only as keyword
> arguments? Code wise it would only mean adding PyObject* kw to
> PLy_output and adding some code to extract detail, hint etc. from kw.
> This would also allow getting rid of the GUC since backward
> compatibility is fully preserved.
>
> Again, sorry for all the back and forth but it still feels like we
> haven't nailed the design to something satisfactory. All the tests you
> needed to change are a hint towards that.
>

It not any problem. I am thankful for cooperation.

Regards

Pavel
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index 015bbad..2bd9bcb
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 1341,1360 
Utility Functions

 The plpy module also provides the functions
!plpy.debug(msg),
!plpy.log(msg),
!plpy.info(msg),
!plpy.notice(msg),
!plpy.warning(msg),
!plpy.error(msg), and
!plpy.fatal(msg).elogin PL/Python
 plpy.error and
 plpy.fatal actually raise a Python exception
 which, if uncaught, propagates out to the calling query, causing
 the current transaction or subtransaction to be aborted.
 raise plpy.Error(msg) and
 raise plpy.Fatal(msg) are
!equivalent to calling
 plpy.error and
 plpy.fatal, respectively.
 The other functions only generate messages of different
--- 1341,1360 
Utility Functions

 The plpy module also provides the functions
!plpy.debug(exception_params),
!plpy.log(exception_params),
!plpy.info(exception_params),
!plpy.notice(exception_params),
!plpy.warning(exception_params),
!plpy.error(exception_params), and
!plpy.fatal(exception_params).elogin PL/Python
 plpy.error and
 plpy.fatal actually raise a Python exception
 which, if uncaught, propagates out to the calling query, causing
 the current transaction or subtransaction to be aborted.
 raise plpy.Error(msg) and
 raise plpy.Fatal(msg) are
!partial equivalent to calling
 plpy.error and
 plpy.fatal, respectively.
 The other functions only generate messages of different
*** $$ LANGUAGE plpythonu;
*** 1367,1372 
--- 1367,1397 

  

+ 
+The exception_params are
+[ message [, detail [, hint [, sqlstate  [, schema  [, table  [, column  [, datatype  [, constraint ].
+These parameters kan be entered as keyword parameters.
+The message, detail, hint
+are automaticly casted to string, other should be string.
+ 
+ 
+ CREATE FUNCTION raise_custom_exception() RETURNS void AS $$
+ plpy.error("custom exception message", "some info about exception", "hint for users")
+ $$ LANGUAGE plpythonu;
+ 
+ postgres=# select raise_custom_exception();
+ ERROR:  XX000: plpy.Error: custom exception message
+ DETAIL:  some info about exception
+ HINT:  hint for users
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python function "raise_custom_exception", line 2, in module
+ plpy.error("custom exception message", "some info about exception", "hint for users")
+ PL/Python function "raise_custom_exception"
+ LOCATION:  PLy_elog, plpy_elog.c:132
+ 
+   
+ 
+   
 Another set of utility functions are
 plpy.quote_literal(string),
 plpy.quote_nullable(string), and

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

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.


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

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

https://commitfest.postgresql.org/9/525/

Regards

Pavel

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

Hi

2016-02-16 17:18 GMT+01:00 Catalin Iacob :

> On Sat, Feb 13, 2016 at 4:26 PM, Pavel Stehule 
> 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 
> -plpy.error('stop on error', 'some detail','some hint')
> -PL/Python function "elog_test"
> +ERROR:  could not convert Python Unicode object to bytes
> +DETAIL:  TypeError: bad argument type for built-in operation
> +CONTEXT:  PL/Python function "elog_test"
>

fixed - the object serialization fails on Py_None

>
> This is related to the use of PyString_AsString and the changed
> semantics of that in Python 3 (due to the fact that strings are now
> Unicode objects and so on). Didn't have time to dig more deeply into
> the exact cause.
>
> Similarly, there are alternative expected test outputs that you didn't
> update, for example src/pl/plpython/expected/plpython_types_3.out so
> tests fail on some Python versions due to those as well.
>
> > 1. the plpy utility functions can use all ErrorData fields,
> > 2. there are no new functions,
> > 3. via GUC plpythonu.legacy_custom_exception we can return previous
> behave,
> > 4. only exception Error is raised with natural structure - no composite
> > value spidata.
> > 5. fields: message, detail and hint are implicitly translated to string
> - it
> > decrease a necessity of legacy mode
>
> I disagree that 5. is a good idea. I think we should just treat
> message, detail and hint like the other ones (schema, table etc.). Use
> s in PyArg_ParseTupleAndKeywords and let the user explicitly cast to a
> string. Explicit is better than implicit. The way you did it you keep
> part of the weird old interface which used to cast to string for you.
> We shouldn't keep warts of the old behaviour, especially with the GUC
> to ask for the old behaviour.
>

removed @5


>
> By the way, getting rid of object_to_string and its usage in
> PLy_output_kw removed some "ERROR: could not convert Python Unicode
> object to bytes" failures in the tests so I'm quite sure that the
> usage of PyString_AsString is responsible for those.
>
> I don't like that you set legacy_custom_exception=true in some
> existing tests, probably to avoid changing them to the new behaviour.
> We should trust that the new behaviour is what we want and the tests
> should reflect that. If it's too much work, remember we're asking
> users to do the same work to convert their code. We have
> elog_test_legacy to test elog_test_legacy=true, the rest of the tests
> should use legacy_custom_exception=false.
>

all regress tests now works in new mode

Regards

Pavel


plpython-enhanced-error-02.patch.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2016-02-16 21:06 GMT+01:00 Catalin Iacob :

> On Tue, Feb 16, 2016 at 5:18 PM, Catalin Iacob 
> wrote:
> > We have
> > elog_test_legacy to test elog_test_legacy=true, the rest of the tests
> > should use legacy_custom_exception=false.
>
> This should have been:
> We have elog_test_legacy to test legacy_custom_exception=true, the
> rest of the tests should use legacy_custom_exception=false.
>

I understand - I fixed regress tests for new behave


>
> I noticed the patch isn't registered in the March CF, please do that.
> It would be a pity to miss 9.6 because of not registering it in time.
>

I'll do it. Now, I fixing 3.4 Python code. There are more issues with
"could not convert Python Unicode object to bytes"

Regards

Pavel

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Tue, Feb 16, 2016 at 5:18 PM, Catalin Iacob  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.


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Sat, Feb 13, 2016 at 4:26 PM, Pavel Stehule  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 
-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.


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

Hi

I am sending new version. Current version does:

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

Curent version doesn't do:
1. touch plpy exception classes.

A doc should be enhanced, but the code should be +/- final.

Regards

Pavel
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index 015bbad..2bd9bcb
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 1341,1360 
Utility Functions

 The plpy module also provides the functions
!plpy.debug(msg),
!plpy.log(msg),
!plpy.info(msg),
!plpy.notice(msg),
!plpy.warning(msg),
!plpy.error(msg), and
!plpy.fatal(msg).elogin PL/Python
 plpy.error and
 plpy.fatal actually raise a Python exception
 which, if uncaught, propagates out to the calling query, causing
 the current transaction or subtransaction to be aborted.
 raise plpy.Error(msg) and
 raise plpy.Fatal(msg) are
!equivalent to calling
 plpy.error and
 plpy.fatal, respectively.
 The other functions only generate messages of different
--- 1341,1360 
Utility Functions

 The plpy module also provides the functions
!plpy.debug(exception_params),
!plpy.log(exception_params),
!plpy.info(exception_params),
!plpy.notice(exception_params),
!plpy.warning(exception_params),
!plpy.error(exception_params), and
!plpy.fatal(exception_params).elogin PL/Python
 plpy.error and
 plpy.fatal actually raise a Python exception
 which, if uncaught, propagates out to the calling query, causing
 the current transaction or subtransaction to be aborted.
 raise plpy.Error(msg) and
 raise plpy.Fatal(msg) are
!partial equivalent to calling
 plpy.error and
 plpy.fatal, respectively.
 The other functions only generate messages of different
*** $$ LANGUAGE plpythonu;
*** 1367,1372 
--- 1367,1397 

  

+ 
+The exception_params are
+[ message [, detail [, hint [, sqlstate  [, schema  [, table  [, column  [, datatype  [, constraint ].
+These parameters kan be entered as keyword parameters.
+The message, detail, hint
+are automaticly casted to string, other should be string.
+ 
+ 
+ CREATE FUNCTION raise_custom_exception() RETURNS void AS $$
+ plpy.error("custom exception message", "some info about exception", "hint for users")
+ $$ LANGUAGE plpythonu;
+ 
+ postgres=# select raise_custom_exception();
+ ERROR:  XX000: plpy.Error: custom exception message
+ DETAIL:  some info about exception
+ HINT:  hint for users
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python function "raise_custom_exception", line 2, in module
+ plpy.error("custom exception message", "some info about exception", "hint for users")
+ PL/Python function "raise_custom_exception"
+ LOCATION:  PLy_elog, plpy_elog.c:132
+ 
+   
+ 
+   
 Another set of utility functions are
 plpy.quote_literal(string),
 plpy.quote_nullable(string), and
diff --git a/src/pl/plpython/expected/plpython_spi.out b/src/pl/plpython/expected/plpython_spi.out
new file mode 100644
index e715ee5..3daf181
*** a/src/pl/plpython/expected/plpython_spi.out
--- b/src/pl/plpython/expected/plpython_spi.out
***
*** 1,3 
--- 1,4 
+ --set plpythonu.legacy_custom_exception = true;
  --
  -- nested calls
  --
diff --git a/src/pl/plpython/expected/plpython_test.out b/src/pl/plpython/expected/plpython_test.out
new file mode 100644
index 7b76faf..bfe5c6f
*** a/src/pl/plpython/expected/plpython_test.out
--- b/src/pl/plpython/expected/plpython_test.out
*** select module_contents();
*** 50,55 
--- 50,96 
  
  CREATE FUNCTION elog_test() RETURNS void
  AS $$
+ plpy.debug('debug','some detail')
+ plpy.log('log','some detail')
+ plpy.info('info','some detail')
+ plpy.info()
+ plpy.info('the question', 42);
+ plpy.info('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.',
+ sqlstate = 'XX000',
+ schema = 'any info about schema',
+ table = 'any info about table',
+ column = 'any info about column',
+ datatype = 'any info about datatype',
+ constraint = 'any info about constraint')
+ plpy.notice('notice','some detail')
+ plpy.warning('warning','some detail')
+ plpy.error('stop on error', 'some detail','some hint')
+ $$ LANGUAGE plpythonu;
+ SELECT elog_test();
+ INFO:  info
+ DETAIL:  some detail
+ INFO:  missing error text
+ INFO:  the question
+ 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

Hi



>
>> Actually I imagine that if there's no agreement between author and first
>> reviewer, there might not *be* a committer in the first place.  Perhaps
>> try to get someone else to think about it and make a decision.  It is
>> possible that some other committer is able to decide by themselves but I
>> wouldn't count on it.
>>
>
> +1.
>
> FWIW, I'd think it's better to not break backwards compatibility, but I'm
> also far from a python expert. It might well be worth adding a plpython GUC
> to control the behavior so that there's a migration path forward, or maybe
> do something like the 'import __future__' that python is doing to ease
> migration to python 3.
>
>
>
Iacob is maybe little bit too defensive - but why not. The implementation
of GUC should not be hard. I see it as best way now. Tomorrow I'll try to
find last versions of this patch in mailbox and try to design compatibility
mode.

I don't like too much a introduction of new general function raise
(proposed by Jim). It is not consistent with current design and it needs a
introduction of enums visible from user level. The work with it isn't too
much comfortable. If it will be required, then we have it. The original
implementation of this proposal was designed in exactly same style.

Regards

Pavel

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Jim Nasby]

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


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Alvaro Herrera]

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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Jim Nasby]

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


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Tue, Feb 2, 2016 at 11: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.
>
> 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 
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.

# simulated PG elog
def elog(message):
ereport(message)

# simulated PG ereport
def ereport(message, detail=None, hint=None):
print 'PG elog/ereport with message: %s detail: %s hint: %s' % (message, detail, hint)


# existing code behaves like this:
#   takes an unlimited number of arguments
#   doesn't take keyword arguments
#   makes a string representation of the tuple of all arguments unless that tuple has size 1 in which case it only makes a string representation of the first one
def info_existing(*args):
if len(args) == 1:
# special case added by Peter in 2e3b16
str_to_log = str(args[0])
else:
str_to_log = str(args)

elog(str_to_log)

# and I'm proposing to change it to do this:
#   take 1 required argument and extra optional arguments for every argument accepted by ereport
#   accepts keyword arguments
#   passing too many arguments will get rejected
def info_new(message, detail=None, hint=None):
ereport(message, detail, hint)

print 'existing behaviour'.center(40)
info_existing('1: hi')
info_existing('2: hi', 'another argument')
info_existing('3: hi', 'another argument', 2)
info_existing('4: hi', 'another argument', 2, 'lots', 'of', 'arguments')

print

print 'new behaviour'.center(40)
info_new('1: hi') # for the documented single argument case same behaviour as existing
info_new('2: hi', 'another argument')
info_new('3: hi', 'another argument', 2)
info_new('4: hi', 'another argument', 2, 'lots', 'of', 'arguments')



-- 
Sent via pgsql-hackers 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Robert Haas]

On Tue, Feb 2, 2016 at 10:39 AM, Catalin Iacob  wrote:
> On Tue, Feb 2, 2016 at 3:48 PM, Pavel Stehule  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


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Alvaro Herrera]

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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

Dne 2. 2. 2016 7:30 napsal uživatel "Catalin Iacob" :
>
> On Mon, Feb 1, 2016 at 5:37 PM, Pavel Stehule 
wrote:
> > Dne 29. 1. 2016 18:09 napsal uživatel "Catalin Iacob"
> > :
> >> 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.

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

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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Tue, Feb 2, 2016 at 3:48 PM, Pavel Stehule  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.


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Jim Nasby]

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


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

hi

I am sorry, I am writing from mobile phone and I cannot to cutting text
well.

Dne 29. 1. 2016 18:09 napsal uživatel "Catalin Iacob" <
iacobcata...@gmail.com>:
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 
> 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 
> 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.
>

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 Error is better.

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

This patch is simple, and maintaining more patches has not sense.

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

if breaking compatibility, then raise* functions are useless, and should be
removed.

> What do you think? Jim doesn't like the 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Mon, Feb 1, 2016 at 5:37 PM, Pavel Stehule  wrote:
> Dne 29. 1. 2016 18:09 napsal uživatel "Catalin Iacob"
> :
>> 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.


0003-Improve-docs.patch
Description: binary/octet-stream

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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Tue, Jan 26, 2016 at 5:42 PM, Pavel Stehule  wrote:
> I removed from previous patch all OOP related changes. New patch contains
> raise_ 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 
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 
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 
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 
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.


0002-use-plpy.Error-to-hold-the-data-not-plpy.SPIError.patch
Description: binary/octet-stream


0001-fix-comment.patch
Description: 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

Hi

>
> > I though about it lot of, and I see a  the core of problems in orthogonal
> > constructed exceptions in Python and Postgres. We working with statements
> > elog, ereport, RAISE EXCEPTION - and these statements are much more like
> > templates - can generate any possible exception. Python is based on
> working
> > with instances of predefined exceptions. And it is core of problem.
> > Template like class cannot be ancestor of instance oriented classes. This
> > is task for somebody who knows well OOP and meta OOP in Python - total
>
> I've been following this discussion with great interest, because PL/Java
> also is rather overdue for tackling the same issues: it has some partial
> ability to catch things from PostgreSQL and even examine them in proper
> detail, but very limited ability to throw things as information-rich as
> is possible from C with ereport. And that work is not as far along as
> you are with PL/Python, there is just a preliminary design/discussion
> wiki document at
>   https://github.com/tada/pljava/wiki/Thoughts-on-logging
>
> I was unaware of this project in PL/Pythonu when I began it, then added
> the reference when I saw this discussion.
>
>
I read your article and it is exactly same situation.

It is conflict between procedural (PostgreSQL) and OOP (Python, Java) API.
I see possible solution in design independent class hierarchies - static
(buildin exceptions) and dynamic (custom exceptions). It cannot be mixed,
but there can be some abstract ancestor. Second solution is defensive -
using procedural API for custom exceptions - what i doing in PLPythonu.

Regards

Pavel

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Chapman Flack]

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


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

Hi


>>> But in my opinion this discussion shouldn't really even be about
>>> catching these things, most of the times you won't catch them and
>>> instead you'll let them go to Postgres. The discussion should be
>>> whether raise plpy.Error(...), plpy.raise_error, plpy.raise_info(,,,)
>>> etc. all with keyword argument support are a good PLPython interface
>>> to Postgres' ereport. I think they are.
>>>
>>
I removed from previous patch all OOP related changes. New patch contains
raise_ functions only. This interface is new generation of previous
functions: info, notice, warning, error with keyword parameters interface.
I didn't changed older functions due keeping compatibility.

I spent lot of time with experiments how to merge PostgreSQL exception
system with Python exception system. But I didn't find a solution without
some design issues. These concepts(s) are too different (note: there are
two concepts: PostgreSQL levels and ANSI SQL SQLSTATE (class, subclass
codes).

I hope so raise_ has benefit for PLPythonu users too. Currently users
has to use ugly workaround to raise rich PostgreSQL exception from Python.
With new function all available functionality related to PostgreSQL
exception can be used simply.

plpy.raise_warning("some doesn't work", detail = "user registration ... ")

This patch is reduced previous patch (no new features, no different
solution, no different code), so I use same entry in commitfest.

Regards

Pavel
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index 015bbad..705413e
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 1367,1372 
--- 1367,1401 

  

+The plpy module also provides the functions
+plpy.raise_debug(args),
+plpy.raise_log(args),
+plpy.raise_info(args),
+plpy.raise_notice(args),
+plpy.raise_warning(args),
+plpy.raise_error(args), and
+plpy.raise_fatal(args).elogin PL/Python
+plpy.raise_error and
+plpy.raise_fatal actually raise a Python exception
+which, if uncaught, propagates out to the calling query, causing
+the current transaction or subtransaction to be aborted.
+raise plpy.Error(msg) and
+raise plpy.Fatal(msg) are
+equivalent to calling
+plpy.raise_error and
+plpy.raise_fatal, respectively.
+The other functions only generate messages of different
+priority levels.
+Whether messages of a particular priority are reported to the client,
+written to the server log, or both is controlled by the
+ and
+ configuration
+variables. See  for more information.
+These functions allows to using keyword parameters:
+[ message [, detail [, hint [, sqlstate  [, schema  [, table  [, column  [, datatype  [, constraint ].
+   
+ 
+   
 Another set of utility functions are
 plpy.quote_literal(string),
 plpy.quote_nullable(string), and
diff --git a/src/pl/plpython/expected/plpython_test.out b/src/pl/plpython/expected/plpython_test.out
new file mode 100644
index 7b76faf..834b14a
*** a/src/pl/plpython/expected/plpython_test.out
--- b/src/pl/plpython/expected/plpython_test.out
*** contents.sort()
*** 43,51 
  return ", ".join(contents)
  $$ LANGUAGE plpythonu;
  select module_contents();
!module_contents
! --
!  Error, Fatal, SPIError, cursor, debug, error, execute, fatal, info, log, notice, prepare, quote_ident, quote_literal, quote_nullable, spiexceptions, subtransaction, warning
  (1 row)
  
  CREATE FUNCTION elog_test() RETURNS void
--- 43,51 
  return ", ".join(contents)
  $$ LANGUAGE plpythonu;
  select module_contents();
!  module_contents 
! -
!  Error, Fatal, SPIError, cursor, debug, error, execute, fatal, info, log, notice, prepare, quote_ident, quote_literal, quote_nullable, raise_debug, raise_error, raise_fatal, raise_info, raise_log, raise_notice, raise_warning, spiexceptions, subtransaction, warning
  (1 row)
  
  CREATE FUNCTION elog_test() RETURNS void
*** CONTEXT:  Traceback (most recent call la
*** 72,74 
--- 72,111 
PL/Python function 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On 1/21/16, Pavel Stehule  wrote:
> 2016-01-14 17:16 GMT+01:00 Catalin Iacob :
>> Consider this call chain: SQL code 1 -> PL/Python code 1 -> SPI (via
>> plpy.execute) -> SQL code 2 -> PL/Python code 2
>>
>> If I'm writing PL/Python code 1 and I want to raise an error toward
>> SQL code 1 I use raise plpy.Error. plpy.SPIError is what I get if I
>> call into SQL code 2 and that has an error. That error could indeed
>> come from a plpy.Error thrown by PL/Python code 2 or it could come
>> from a SQL syntax error or whatever. But the symmetry holds:
>> plpy.Error is what you raise to stop the query and return errors to
>> your SQL caller and plpy.SPIError is what you get back if you use SPI
>> to execute some other piece of SQL which has an error. I *think* (I'm
>> an internals newbie though so I could be wrong) that SQL code 1
>> doesn't call into PL/Python code 1 via SPI so why would the latter use
>> something called SPIError to inform the former about an error?
>>
>
> It is not correct - outside PLPython you got a Error (PostgreSQL error has
> not any classes), and isn't important the raising class (Error or
> SPIError). Inside PL/Python you will got SPIError or successors (based on
> SQLcode).

What exactly is not correct? Indeed, *outside* PLPython you don't get
a Python exception because Postgres isn't Python, you get Postgres'
way of representing an exception (error code, message, hint and so on
that might go to the client or not etc.). But that's normal, it's just
the impedance mismatch between the fact that you embed Python with its
rules into Postgres with its other rules. It's normal that the "host"
(Postgres) wins in the end and uses its mechanism to report errors.
The "guest" (PLPython) is there only to give mechanisms to activate
the "host".

But *inside* PLPython what I wrote is true, see this example for what I mean:

CREATE FUNCTION test()
  RETURNS int
AS $$
  def a_func():
raise plpy.Error('an error')

  try:
a_func()
  except plpy.Error as exc:
plpy.info('have exc {} of type {}'.format(exc, type(exc)))
plpy.info('have spidata {}'.format(hasattr(exc, 'spidata')))
$$ LANGUAGE plpython3u;

Running this produces:

DROP FUNCTION
CREATE FUNCTION
psql:plpy_demo:16: INFO:  have exc an error of type 
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-qaor...@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.


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2016-01-26 8:22 GMT+01:00 Pavel Stehule :

>
>
> 2016-01-26 7:31 GMT+01:00 Catalin Iacob :
>
>> On 1/21/16, Pavel Stehule  wrote:
>> > 2016-01-14 17:16 GMT+01:00 Catalin Iacob :
>> >> Consider this call chain: SQL code 1 -> PL/Python code 1 -> SPI (via
>> >> plpy.execute) -> SQL code 2 -> PL/Python code 2
>> >>
>> >> If I'm writing PL/Python code 1 and I want to raise an error toward
>> >> SQL code 1 I use raise plpy.Error. plpy.SPIError is what I get if I
>> >> call into SQL code 2 and that has an error. That error could indeed
>> >> come from a plpy.Error thrown by PL/Python code 2 or it could come
>> >> from a SQL syntax error or whatever. But the symmetry holds:
>> >> plpy.Error is what you raise to stop the query and return errors to
>> >> your SQL caller and plpy.SPIError is what you get back if you use SPI
>> >> to execute some other piece of SQL which has an error. I *think* (I'm
>> >> an internals newbie though so I could be wrong) that SQL code 1
>> >> doesn't call into PL/Python code 1 via SPI so why would the latter use
>> >> something called SPIError to inform the former about an error?
>> >>
>> >
>> > It is not correct - outside PLPython you got a Error (PostgreSQL error
>> has
>> > not any classes), and isn't important the raising class (Error or
>> > SPIError). Inside PL/Python you will got SPIError or successors (based
>> on
>> > SQLcode).
>>
>> What exactly is not correct? Indeed, *outside* PLPython you don't get
>> a Python exception because Postgres isn't Python, you get Postgres'
>> way of representing an exception (error code, message, hint and so on
>> that might go to the client or not etc.). But that's normal, it's just
>> the impedance mismatch between the fact that you embed Python with its
>> rules into Postgres with its other rules. It's normal that the "host"
>> (Postgres) wins in the end and uses its mechanism to report errors.
>> The "guest" (PLPython) is there only to give mechanisms to activate
>> the "host".
>>
>> But *inside* PLPython what I wrote is true, see this example for what I
>> mean:
>>
>> CREATE FUNCTION test()
>>   RETURNS int
>> AS $$
>>   def a_func():
>> raise plpy.Error('an error')
>>
>>   try:
>> a_func()
>>   except plpy.Error as exc:
>> plpy.info('have exc {} of type {}'.format(exc, type(exc)))
>> plpy.info('have spidata {}'.format(hasattr(exc, 'spidata')))
>> $$ LANGUAGE plpython3u;
>>
>> Running this produces:
>>
>> DROP FUNCTION
>> CREATE FUNCTION
>> psql:plpy_demo:16: INFO:  have exc an error of type 
>> 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-qaor...@mail.gmail.com
>> need to be fixed.
>>
>> Would it help maybe to remove myself as reviewer in the CF? Maybe that
>> makes somebody else pick it up and move it further.
>>
>
please, wait. We can finish the part, where is a agreement. And although it
is not final solution from Python perspective - I hope, so it can be really
useful for PLPython user - there will be one way how to raise rich
exception without ugly workaround

Regards

Pavel


>
>> I could also code the version I'm describing but that design would be
>> contrary to what Jim and Pavel lean towards now. It could help to
>> compare approaches side by side but I don't like the idea of dueling
>> patches.
>>
>
> I would to reduce this patch and don't try to touch on buildin exceptions.
> I hope, so there isn't any disagreement?
>
> I though about it lot of, and I see a  the core of problems in orthogonal
> constructed exceptions in Python and Postgres. We working with statements
> elog, ereport, RAISE EXCEPTION - and these statements are much 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2016-01-26 7:31 GMT+01:00 Catalin Iacob :

> On 1/21/16, Pavel Stehule  wrote:
> > 2016-01-14 17:16 GMT+01:00 Catalin Iacob :
> >> Consider this call chain: SQL code 1 -> PL/Python code 1 -> SPI (via
> >> plpy.execute) -> SQL code 2 -> PL/Python code 2
> >>
> >> If I'm writing PL/Python code 1 and I want to raise an error toward
> >> SQL code 1 I use raise plpy.Error. plpy.SPIError is what I get if I
> >> call into SQL code 2 and that has an error. That error could indeed
> >> come from a plpy.Error thrown by PL/Python code 2 or it could come
> >> from a SQL syntax error or whatever. But the symmetry holds:
> >> plpy.Error is what you raise to stop the query and return errors to
> >> your SQL caller and plpy.SPIError is what you get back if you use SPI
> >> to execute some other piece of SQL which has an error. I *think* (I'm
> >> an internals newbie though so I could be wrong) that SQL code 1
> >> doesn't call into PL/Python code 1 via SPI so why would the latter use
> >> something called SPIError to inform the former about an error?
> >>
> >
> > It is not correct - outside PLPython you got a Error (PostgreSQL error
> has
> > not any classes), and isn't important the raising class (Error or
> > SPIError). Inside PL/Python you will got SPIError or successors (based on
> > SQLcode).
>
> What exactly is not correct? Indeed, *outside* PLPython you don't get
> a Python exception because Postgres isn't Python, you get Postgres'
> way of representing an exception (error code, message, hint and so on
> that might go to the client or not etc.). But that's normal, it's just
> the impedance mismatch between the fact that you embed Python with its
> rules into Postgres with its other rules. It's normal that the "host"
> (Postgres) wins in the end and uses its mechanism to report errors.
> The "guest" (PLPython) is there only to give mechanisms to activate
> the "host".
>
> But *inside* PLPython what I wrote is true, see this example for what I
> mean:
>
> CREATE FUNCTION test()
>   RETURNS int
> AS $$
>   def a_func():
> raise plpy.Error('an error')
>
>   try:
> a_func()
>   except plpy.Error as exc:
> plpy.info('have exc {} of type {}'.format(exc, type(exc)))
> plpy.info('have spidata {}'.format(hasattr(exc, 'spidata')))
> $$ LANGUAGE plpython3u;
>
> Running this produces:
>
> DROP FUNCTION
> CREATE FUNCTION
> psql:plpy_demo:16: INFO:  have exc an error of type 
> 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-qaor...@mail.gmail.com
> need to be fixed.
>
> Would it help maybe to remove myself as reviewer in the CF? Maybe that
> makes somebody else pick it up and move it further.
>
> I could also code the version I'm describing but that design would be
> contrary to what Jim and Pavel lean towards now. It could help to
> compare approaches side by side but I don't like the idea of dueling
> patches.
>

I would to reduce this patch and don't try to touch on buildin exceptions.
I hope, so there isn't any disagreement?

I though about it lot of, and I see a  the core of problems in orthogonal
constructed exceptions in Python and Postgres. We working with statements
elog, ereport, RAISE EXCEPTION - and these statements are much more like
templates - can generate any possible exception. Python is based on working
with instances of predefined exceptions. And it is core of problem.
Template like class cannot be ancestor of instance oriented classes. This
is task for somebody who knows well OOP and meta OOP in Python - total
different chapter, different patch

Regards

Pavel

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Jim Nasby]

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

08000EERRCODE_CONNECTION_EXCEPTION 
 connection_exception
08003EERRCODE_CONNECTION_DOES_NOT_EXIST 
 connection_does_not_exist
08006EERRCODE_CONNECTION_FAILURE 
 connection_failure
08001EERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION 
 sqlclient_unable_to_establish_sqlconnection
08004EERRCODE_SQLSERVER_REJECTED_ESTABLISHMENT_OF_SQLCONNECTION 
 sqlserver_rejected_establishment_of_sqlconnection
08007EERRCODE_TRANSACTION_RESOLUTION_UNKNOWN 
 transaction_resolution_unknown
08P01EERRCODE_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


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2016-01-14 17:16 GMT+01:00 Catalin Iacob :

> On Wed, Jan 13, 2016 at 7:40 PM, Jim Nasby 
> wrote:
> > On 1/12/16 11:25 AM, Catalin Iacob wrote:
> >> They're similar but not really the same thing. raise Error and
> >> plpy.error are both ways to call ereport(ERROR, ...) while SPIError is
> >> raised when coming back after calling into Postgres to execute SQL
> >> that itself raises an error. Now indeed, that executed SQL raised an
> >> error itself via another ereport(ERROR, ...) somewhere but that's a
> >> different thing.
> >
> >
> > Why should they be different? An error is an error. You either want to
> trap
> > a specific type of error or you don't. Having two completely different
> ways
> > to do the same thing is just confusing.
>
> With my (indeed limited) understanding, I don't agree they're the same
> thing and I tried to explain above why I think they're quite
> different. You may not agree. If they are different things, using
> different exception types is natural.
>
> Consider this call chain: SQL code 1 -> PL/Python code 1 -> SPI (via
> plpy.execute) -> SQL code 2 -> PL/Python code 2
>
> If I'm writing PL/Python code 1 and I want to raise an error toward
> SQL code 1 I use raise plpy.Error. plpy.SPIError is what I get if I
> call into SQL code 2 and that has an error. That error could indeed
> come from a plpy.Error thrown by PL/Python code 2 or it could come
> from a SQL syntax error or whatever. But the symmetry holds:
> plpy.Error is what you raise to stop the query and return errors to
> your SQL caller and plpy.SPIError is what you get back if you use SPI
> to execute some other piece of SQL which has an error. I *think* (I'm
> an internals newbie though so I could be wrong) that SQL code 1
> doesn't call into PL/Python code 1 via SPI so why would the latter use
> something called SPIError to inform the former about an error?
>

It is not correct - outside PLPython you got a Error (PostgreSQL error has
not any classes), and isn't important the raising class (Error or
SPIError). Inside PL/Python you will got SPIError or successors (based on
SQLcode).

Currently If I raise plpy.Error, then it is immediately trasformed to
PostgreSQL, and and then to SPIError and successors.

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Wed, Jan 13, 2016 at 7:40 PM, Jim Nasby  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?


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Jim Nasby]

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


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Tue, Jan 12, 2016 at 5:50 PM, Pavel Stehule  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.


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2016-01-12 17:59 GMT+01:00 Catalin Iacob :

> On Tue, Jan 12, 2016 at 5:50 PM, Pavel Stehule 
> wrote:
> > Error and Fatal exception classes are introduced in my patch - it was
> Peter'
> > request (if I remember it well), and now I am thinking so it is not good
> > idea.
>
> Now everybody is confused :). Your patch does:
>
> -PLy_exc_error = PyErr_NewException("plpy.Error", NULL, NULL);
> -PLy_exc_fatal = PyErr_NewException("plpy.Fatal", NULL, NULL);
> -PLy_exc_spi_error = PyErr_NewException("plpy.SPIError", NULL, NULL);
>
> [snip]
>
> +PLy_exc_error = PyErr_NewException("plpy.Error", PLy_exc_base, NULL);
> +PLy_exc_fatal = PyErr_NewException("plpy.Fatal", PLy_exc_base, NULL);
> +PLy_exc_spi_error = PyErr_NewException("plpy.SPIError",
> PLy_exc_base, NULL);
>
> So they are there without the patch, you now make them inherit from
> the new BaseError previously they just inherited from Exception.
>

I was wrong, I am sorry.


>
> More soon in another reply I was just typing when this came in.
>

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2016-01-11 20:11 GMT+01:00 Jim Nasby :

> On 1/11/16 12:46 PM, Pavel Stehule wrote:
>
>> 2016-01-11 19:41 GMT+01:00 Jim Nasby > >:
>>
>> On 1/11/16 12:33 PM, Pavel Stehule wrote:
>>
>> 1. break compatibility and SPIError replace by Error
>>
>>
>> At this point I've lost track... what's the incompatibility between
>> the two?
>>
>>
>> the name and internal format (but this structure can be visible to user
>> space)
>>
>
> Were Error and Fatal ever documented as classes? All I see is "raise
> plpy.Error(msg) and raise plpy.Fatal(msg) are equivalent to calling
> plpy.error and plpy.fatal, respectively." which doesn't lead me to believe
> I should be trapping on those.
>

Error and Fatal exception classes are introduced in my patch - it was
Peter' request (if I remember it well), and now I am thinking so it is not
good idea.


>
> It's not clear to me why you'd want to handle error and fatal differently
> anyway; an error is an error. Unless fatal isn't supposed to be trappable?
> [1] leads me to believe that you shouldn't be able to trap a FATAL because
> it's supposed to cause the entire session to abort.
>

> Since spiexceptions and SPIError are the only documented exceptions
> classes, I'd say we should stick with those and get rid of the others.
> Worst-case, we can have a compatability GUC, but I think plpy.Error and
> plpy.Fatal were just poorly thought out.
>

I have same opinion now. I remove it from my patch.

Pavel



>
> [1]
> http://www.postgresql.org/docs/9.5/static/runtime-config-logging.html#RUNTIME-CONFIG-SEVERITY-LEVELS
>
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
>

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Mon, Jan 11, 2016 at 7:33 PM, Pavel Stehule  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.


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Fri, Jan 8, 2016 at 7:56 AM, Pavel Stehule  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.


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Jim Nasby]

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


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2016-01-11 17:05 GMT+01:00 Catalin Iacob :

> On Fri, Jan 8, 2016 at 7:56 AM, Pavel Stehule 
> wrote:
> >> 3. PLy_error__init__ is used for BaseError but always sets an
> >> attribute called spidata, I would expect that to only happen for
> >> SPIError not for BaseError. You'll need to pick some other way of
> >> attaching the error details to BaseError instances. Similarly,
> >> PLy_get_spi_error_data became PLy_get_error_data and it's invoked on
> >> other classes than SPIError but it still always looks at the spidata
> >> attribute.
> >
> >
> > I afraid of compatibility issues - so I am not changing internal format
> > simply. Some legacy application can be based on detection of spidata
> > attribute. So I cannot to change this structure for SPIError.
>
> Indeed, I wasn't suggesting changing it for SPIError, that needs to
> stay spidata.
>
> > I can change it for BaseError, but this is question. Internally BaseError
> > and SPIError share same data. So it can be strange if BaseError and
> SPIError
> > uses different internal formats.
> >
> > I am interesting your opinion??
>
> Yes, it's not great if BaseError and SPIError have different ways but
> I think having BaseError have a member (spidata) named after one of
> its subclasses is even worse.
>
> I would say detail, hint etc belong to BaseError and I would make them
> different attributes of BaseError instances instead of one big
> BaseError.data tuple similar to what SPIError.spidata is now. SPIError
> would keep its spidata for compatibility but would get the same
> information into its detail, hint etc. attributes since it's derived
> from BaseError.
>
> So an equivalent to this Python (pseudo)code:
>
> # base class for all exceptions raised by PL/Python
> class BaseError:
> def __init__(self, msg, detail=None, hint=None, and so on for
> every accepted argument):
> self.msg = msg
> self.detail = detail
> # and so on
>
> class Error(BaseError):
>  pass
>
> class Fatal(BaseError):
> pass
>
> class SPIError(BaseError):
>  def __init__(self, msg):
>  # call BaseError.__init__ so that SPIError also gets .msg,
> .detail and so on like all other BaseError instances
>  # do what's done today to fill spidata to keep backward
> compatibility
>
> If you don't like that spidata duplicates the other fields, it's
> probably also ok to not make inherit from BaseError at all. I actually
> like this better. Then I would call the base class something like
> UserRaisedError (ugly name but can't think of something better to
> emphasize that it's about errors that the PL/Python developer is
> supposed to raise rather SPIError which is Postgres raising them) and
> it would be:
>
> # base class for exceptions raised by users in their PL/Python code
> class UserRaisedError:
> def __init__(self, msg, detail=None, hint=None, and so on for
> every accepted argument):
> self.msg = msg
> self.detail = detail
> # and so on
>
> class Error(UserRaisedError):
>  pass
>
> class Fatal(UserRaisedError):
> pass
>
> # base class for exceptions raised by Postgres when executing sql code
> class SPIError:
>  # no change to this class
>
>
I see it.

it looks like distinguish between Error and SPIError is wrong way. And I
have any other ugly use case.

If I raise a Error from one PLPythonu function, then in other PLPython
function I'll trap a SPIError exception, because the information about
class was lost inside Postgres. And it should be pretty messy.
I have not information if any exception was User based or SPI based.

The differentiation between Error and SPIError is wrong, because there
isn't any difference in reality. There are two ways.

1. break compatibility and SPIError replace by Error

2. don't introduce Error class

-- @1 is better - the SPIError isn't the best name, but breaking
compatibility is pretty unhappy - so only one solution is @2 :(


Comments, notes ??

Regards

Pavel



> >
> > a Fatal cannnot be raised - it is a session end. Personally - support of
> > Fatal level is wrong, and it should not be propagated to user level, but
> it
> > is now. And then due consistency I wrote support for fatal level. But
> hard
> > to test it.
> >
>
> I see, then never mind.
>

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2016-01-11 19:41 GMT+01:00 Jim Nasby :

> On 1/11/16 12:33 PM, Pavel Stehule wrote:
>
>> 1. break compatibility and SPIError replace by Error
>>
>
> At this point I've lost track... what's the incompatibility between the
> two?
>

the name and internal format (but this structure can be visible to user
space)

Pavel


> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
>

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Jim Nasby]

On 1/11/16 12:46 PM, Pavel Stehule wrote:

2016-01-11 19:41 GMT+01:00 Jim Nasby >:

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


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

shellOn Thu, Dec 31, 2015 at 11:37 AM, Pavel Stehule
 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.


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2016-01-08 7:31 GMT+01:00 Catalin Iacob :

> shellOn Thu, Dec 31, 2015 at 11:37 AM, Pavel Stehule
>  wrote:
> > here is new version.
>
> And here's a new review, sorry for the delay.
>
> > Now I use a common ancestor "plpy.BaseError" for plpy builtin classes. So
> > plpy.SPIError isn't descendant of plpy.Error and then there are not
> possible
> > incompatibility issues.
>
> That's good.
>
> > Instead modification builtin function plpy.debug, plpy.info, ... and
> > introduction incompatibility I wrote new set of functions with keyword
> > parameters (used mainly  for elevel < ERROR):
> >
> > plpy.raise_debug, plpy.raise_info, plpy.raise_notice, plpy.raise_warning,
> > plpy.raise_error and plpy.raise_fatal.
>
> I'm on the fence whether these are good ideas. On one hand having them
> is nice and they avoid changing the existing plpy.debug etc. in
> backward incompatible ways, on the other hand they're similar to those
> so it can be confusing to know which ones to pick. Any opinions from
> others on whether it's better to add these or not?
>
> The docs need more work, especially if raise_* are kept, as the docs
> should then clearly explain the differences between the 2 sets and
> nudge users toward the new raise_* functions. I can help with that
> though if there are objections to these functions I wouldn't mind
> hearing it before I document them.
>

ok, we will wait.


>
> As for the code:
>
> 1. there are quite some out of date comments referring to SPIError in
> places that now handle more types (like /* prepare dictionary with
> __init__ method for SPIError class */ in plpy_plpymodule.c)
>
> 2. you moved PLy_spi_exception_set from plpy_spi.c to plpy_elog.c and
> renamed it to PLy_base_exception_set but it's still spi specific: it
> still sets an attribute called spidata. You needed this because you
> call it in PLy_output_kw but can't you instead make PLy_output_kw
> similar to PLy_output and just call PLy_exception_set in the PG_CATCH
> block like PLy_output does? With the patch plpy.error(msg) will raise
> an error object without an spidata attribute but plpy.raise_error(msg)
> will raise an error with an spidata attribute.
>
> 3. PLy_error__init__ is used for BaseError but always sets an
> attribute called spidata, I would expect that to only happen for
> SPIError not for BaseError. You'll need to pick some other way of
> attaching the error details to BaseError instances. Similarly,
> PLy_get_spi_error_data became PLy_get_error_data and it's invoked on
> other classes than SPIError but it still always looks at the spidata
> attribute.
>

I afraid of compatibility issues - so I am not changing internal format
simply. Some legacy application can be based on detection of spidata
attribute. So I cannot to change this structure for SPIError.

I can change it for BaseError, but this is question. Internally BaseError
and SPIError share same data. So it can be strange if BaseError and
SPIError uses different internal formats.

I am interesting your opinion??


>
> 4. it wouldn't hurt to expand the tests a bit to also raise plpy.Fatal
> with keyword arguments and maybe catch BaseError and inspect it a bit
> to see it contains reasonable values (per 4. have spidata when raising
> an SPIError but not when raising an Error or BaseError or Fatal etc.)
>

a Fatal cannnot be raised - it is a session end. Personally - support of
Fatal level is wrong, and it should not be propagated to user level, but it
is now. And then due consistency I wrote support for fatal level. But hard
to test it.



>
> As seen from 1, 2, and 3 the patch is still too much about SPIError.
> As I see it, it should only touch SPIError to make it inherit from the
> new BaseError but for the rest, the patch shouldn't change it and
> shouldn't propagate spidata attributes and SPIError comments. As it
> stands, the patch has historical artifacts that show it was initially
> a patch for SPIError but it's now a patch for error reporting so those
> should go away.
>
> I'll set the patch back to waiting for author.
>

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

Hi

here is new version.

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.

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.

With this patch we can write:

plpy.raise_warning('some is wrong', hint = 'bla bla')
raise plpy.Error(some is wrong', sqlcode = 'XX543')

Regards

Pavel
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index 015bbad..089b143
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 1205,1210 
--- 1205,1228 
  approximately the same functionality
 

+ 
+   
+Raising Errors
+ 
+
+ A plpy.Error can be raised from PL/Python, the constructor accepts
+ keyword parameters:
+ plpy.Error([ message [, detail [, hint [, sqlstate  [, schema  [, table  [, column  [, datatype  [, constraint ]).
+
+
+ An example of raising custom exception could be written as:
+ 
+ DO $$
+   raise plpy.Error('custom message', hint = 'It is test only');
+ $$ LANGUAGE plpythonu;
+ 
+
+   
   
  
   
*** $$ LANGUAGE plpythonu;
*** 1367,1372 
--- 1385,1421 

  

+The plpy module also provides the functions
+plpy.raise_debug(args),
+plpy.raise_log(args),
+plpy.raise_info(args),
+plpy.raise_notice(args),
+plpy.raise_warning(args),
+plpy.raise_error(args), and
+plpy.raise_fatal(args).elogin PL/Python
+plpy.raise_error and
+plpy.raise_fatal actually raise a Python exception
+which, if uncaught, propagates out to the calling query, causing
+the current transaction or subtransaction to be aborted.
+raise plpy.Error(msg) and
+raise plpy.Fatal(msg) are
+equivalent to calling
+plpy.raise_error and
+plpy.raise_fatal, respectively.
+The other functions only generate messages of different
+priority levels.
+Whether messages of a particular priority are reported to the client,
+written to the server log, or both is controlled by the
+ and
+ configuration
+variables. See  for more information.
+These functions allows to using keyword parameters:
+[ message [, detail [, hint [, sqlstate  [, schema  [, table  [, column  [, datatype  [, constraint ].
+ 
+   
+ 
+ 
+   
 Another set of utility functions are
 plpy.quote_literal(string),
 plpy.quote_nullable(string), and
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
new file mode 100644
index 1f52af7..cb792eb
*** a/src/pl/plpython/expected/plpython_error.out
--- b/src/pl/plpython/expected/plpython_error.out
*** EXCEPTION WHEN SQLSTATE 'SILLY' THEN
*** 422,424 
--- 422,486 
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ /* the possibility to set fields of custom exception
+  */
+ DO $$
+ raise plpy.Error('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.Error: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 4, in 
+ hint = 'This is hint text.')
+ PL/Python anonymous code block
+ \set VERBOSITY verbose
+ DO $$
+ raise plpy.Error('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.',
+ sqlstate = 'SILLY',
+ schema = 'any info about schema',
+ table = 'any info about table',
+ column = 'any info about column',
+ datatype = 'any info about datatype',
+ constraint = 'any info about constraint')
+ $$ LANGUAGE plpythonu;
+ ERROR:  SILLY: plpy.Error: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 10, in 
+ constraint = 'any info about constraint')
+ PL/Python anonymous code block
+ SCHEMA NAME:  any info about schema
+ TABLE NAME:  any info about table
+ COLUMN NAME:  any info about column
+ DATATYPE NAME:  any info about datatype
+ CONSTRAINT NAME:  any info about constraint
+ LOCATION:  PLy_elog, plpy_elog.c:122
+ \set VERBOSITY default
+ DO $$
+ raise plpy.Error(detail = 'This is detail text')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.Error: 
+ DETAIL:  This is detail text
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 2, in 
+ raise plpy.Error(detail = 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

Hi

2015-12-08 7:06 GMT+01:00 Catalin Iacob :

> On Thu, Dec 3, 2015 at 6:54 PM, Pavel Stehule 
> wrote:
> > Don't understand - if Fatal has same behave as Error, then why it cannot
> be
> > inherited from Error?
> >
> > What can be broken?
>
> Existing code that did "except plpy.Error as exc" will now also be
> called if plpy.Fatal is raised. That wasn't the case so this changes
> meaning of existing code, therefore it introduces an incompatibility.
>

I read some notes about Python naming convention

and we can use different names for new exception classes

* common ancestor - plpy.BaseError

* descendants - plpy.ExecError, plpy.SPIError, plpy.FatalError


It should to decrease compatibility issues to minimum. SPIError was used
mostly for error trapping (see doc).

Is it ok for you?

Regards

Pavel

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

Hi

2015-12-08 7:06 GMT+01:00 Catalin Iacob :

> On Thu, Dec 3, 2015 at 6:54 PM, Pavel Stehule 
> wrote:
> > Don't understand - if Fatal has same behave as Error, then why it cannot
> be
> > inherited from Error?
> >
> > What can be broken?
>
> Existing code that did "except plpy.Error as exc" will now also be
> called if plpy.Fatal is raised. That wasn't the case so this changes
> meaning of existing code, therefore it introduces an incompatibility.
>

yes, there should be some common ancestor for plpy.Error and plpy.Fatal

Have you any idea, how this ancestor should be named?


> >>
> >> 5. all the reporting functions: plpy.debug...plpy.fatal functions
> >> should also be changed to allow more arguments than the message and
> >> allow them as keyword arguments
> >
> >
> > this is maybe bigger source of broken compatibility
> >
> > lot of people uses plpy.debug(var1, var2, var3)
> >
> > rich constructor use $1 for message, $2 for detail, $3 for hint. So it
> was a
> > reason, why didn't touch these functions
>
> No, if you use PyArg_ParseTupleAndKeywords you'll have Python accept
> all of these so previous ways are still accepted:
>
> plpy.error('a_msg', 'a_detail', 'a_hint')
> plpy.error'a_msg', 'a_detail', hint='a_hint')
> plpy.error('a_msg', detail='a_detail', hint='a_hint')
> plpy.error(msg='a_msg', detail='a_detail', hint='a_hint')
> plpy.error('a_msg')
> plpy.error(msg='a_msg')
> etc.
>
> But I now see that even though the docs say plpy.error and friends
> take a single msg argument, they actually allow any number of
> arguments. If multiple arguments are passed they are converted to a
> tuple and the string representation of that tuple goes into
> ereport/plpy.Error:
>
> CREATE FUNCTION test()
>   RETURNS int
> AS $$
>   try:
> plpy.error('an error message', 'detail for error', 'hint for error')
>   except plpy.Error as exc:
> plpy.info('have exc {}'.format(exc))
> plpy.info('have exc.args |{}| of type {}'.format(exc.args,
> type(exc.args)))
> $$ LANGUAGE plpython3u;
> SELECT test();
> [catalin@archie pg]$ psql -f plpy test
> CREATE FUNCTION
> psql:plpy:13: INFO:  have exc ('an error message', 'detail for error',
> 'hint for error')
> psql:plpy:13: INFO:  have exc.args |("('an error message', 'detail for
> error', 'hint for error')",)| of type 
>
> In the last line note that exc.args (the args tuple passed in the
> constructor of plpy.Error) is a tuple with a single element which is a
> string which is a representation of the tuple passed into plpy.error.
> Don't know why this was thought useful, it was certainly surprising to
> me. Anyway, the separate $2, $3 etc are currently not detail and hint,
> they're just more stuff that gets appended to this tuple. They don't
> get passed to clients as hints. And you can pass lots of them not just
> detail and hint.
>

using tuple is less work for you, you don't need to concat more values into
one. I don't know, how often this technique is used - probably it has sense
only for NOTICE and lower levels - for adhoc debug messages. Probably not
used elsewhere massively.



>
> My proposal would change the meaning of this to actually interpret the
> second argument as detail, third as hint and to only allow a limited
> number (the ones with meaning to ereport). The hint would go to
> ereport so it would be a real hint: it would go to clients as HINT and
> so on. I think that's way more useful that what is done now. But I now
> see my proposal is also backward incompatible.
>

It was my point


>
> > I am not against to plpy.ereport function - it can live together with
> rich
> > exceptions class, and users can use what they prefer.
>
> I wasn't suggesting introducing ereport, I was saying the existing
> functions and exceptions are very similar to your proposed ereport.
> Enhancing them to take keyword arguments would make them a bit more
> powerful. Adding ereport would be another way of doing the same thing
> so that's more confusing than useful.
>

ok


>
> All in all it's hard to improve this cleanly. I'm still not sure the
> latest patch is a good idea but now I'm also not sure what I proposed
> is better than leaving it as it is.
>

We can use different names, we should not to implement all changes at once.

My main target is possibility to raise rich exception instead dirty
workaround. Changing current functions isn't necessary - although if we are
changing API, is better to do it once.

Regards

Pavel

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Thu, Dec 3, 2015 at 6:54 PM, Pavel Stehule  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('have exc {}'.format(exc))
plpy.info('have exc.args |{}| of type {}'.format(exc.args, type(exc.args)))
$$ LANGUAGE plpython3u;
SELECT test();
[catalin@archie pg]$ psql -f plpy test
CREATE FUNCTION
psql:plpy:13: INFO:  have exc ('an error message', 'detail for error',
'hint for error')
psql:plpy:13: INFO:  have exc.args |("('an error message', 'detail for
error', 'hint for error')",)| of type 

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.


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Thu, Dec 3, 2015 at 7:58 AM, Pavel Stehule  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 via the
exceptions or the utility functions.

I think the above doesn't break anything, doesn't confuse user
exceptions with backend SPIError exceptions, enhances error reporting
features for the PL/Python user to bring them up to par with ereport
and PL/pgSQL RAISE and solve your initial use case at the top of the
thread (but with slightly different spelling to match what already
exists in PL/Python):

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

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

Is what I mean more clear now? Do you (and others) agree?


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2015-12-03 16:57 GMT+01:00 Catalin Iacob :

> On Thu, Dec 3, 2015 at 7:58 AM, Pavel Stehule 
> wrote:
> > I am sorry, I don't understand. Now due inheritance plpy.Fatal and
> > plpy.SPIError has availability to use keyword parameters.
>
> Indeed, I didn't realize this, but I don't think it changes the main
> argument.
>
> What I think should happen:
>
> 1. Error should take keyword arguments
> 2. same for Fatal
>

understand


> 3. Fatal should not be changed to inherit from Error - it should stay
> like it is now, just another exception class
> You can argue a Fatal error is an Error but these classes already
> exist and are independent, changing their relationship is backward
> incompatible.
>

Don't understand - if Fatal has same behave as Error, then why it cannot be
inherited from Error?

What can be broken?



> 4. SPIError should not be changed at all
>  It's for errors raised by query execution not user PL/Python code
> so doing raise SPIError in PL/Python doesn't make sense.
>  And again, changing the inheritance relationship of these
> existing classes changes meaning of existing code that catches the
> exceptions.
>

can be


> 5. all the reporting functions: plpy.debug...plpy.fatal functions
> should also be changed to allow more arguments than the message and
> allow them as keyword arguments
>

this is maybe bigger source of broken compatibility

lot of people uses plpy.debug(var1, var2, var3)

rich constructor use $1 for message, $2 for detail, $3 for hint. So it was
a reason, why didn't touch these functions


>  They are Python wrappers for ereport and this would make them
> similar in capabilities to the PL/pgSQL RAISE
>  This will make plpy.error(...) stay equivalent in capabilities
> with raise plpy.Error(...), same for fatal and Fatal
> 6. the docs should move to the "Utility Functions" section
>  There, it's already described how to raise errors either via the
> exceptions or the utility functions.
>
> I think the above doesn't break anything, doesn't confuse user
> exceptions with backend SPIError exceptions, enhances error reporting
> features for the PL/Python user to bring them up to par with ereport
> and PL/pgSQL RAISE and solve your initial use case at the top of the
> thread (but with slightly different spelling to match what already
> exists in PL/Python):
>
> "We cannot to raise PostgreSQL exception with setting all possible
> fields. I propose new function
>
> plpy.ereport(level, [ message [, detail [, hint [, sqlstate, ... )"
>

I am not against to plpy.ereport function - it can live together with rich
exceptions class, and users can use what they prefer.

It is few lines more


>
> Is what I mean more clear now? Do you (and others) agree?
>

not too much  :)

Regards

Pavel

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Tue, Dec 1, 2015 at 2:17 AM, Pavel Stehule  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.


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2015-12-03 7:04 GMT+01:00 Catalin Iacob :

> On Tue, Dec 1, 2015 at 2:17 AM, Pavel Stehule 
> 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.
>


I am sorry, I don't understand. Now due inheritance plpy.Fatal and
plpy.SPIError has availability to use keyword parameters.

postgres=# do $$ raise plpy.Fatal('AHOJ','NAZDAR');
$$ language plpythonu;
FATAL:  38000: plpy.Fatal: AHOJ
DETAIL:  NAZDAR
CONTEXT:  Traceback (most recent call last):
  PL/Python anonymous code block, line 1, in 
raise plpy.Fatal('AHOJ','NAZDAR');
PL/Python anonymous code block

Regards

Pavel

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

Hi

>
>> > Do you have some ideas about the name of this class?
>>
>> I think plpy.Error is fine.
>>
>>
> here is updated patch - work with 2.x Python.
>
> I have 3.x Python broken on my fedora, so I should not do tests on 3.x.
>

here is complete patch - regress tests for all supported Python branches


>
> Regards
>
> Pavel
>
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index 015bbad..ee6a42a
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 1205,1210 
--- 1205,1228 
  approximately the same functionality
 

+ 
+   
+Raising Errors
+ 
+
+ A plpy.Error can be raised from PL/Python, the constructor accepts
+ keyword parameters:
+ plpy.Error([ message [, detail [, hint [, sqlstate  [, schema  [, table  [, column  [, datatype  [, constraint ]).
+
+
+ An example of raising custom exception could be written as:
+ 
+ DO $$
+   raise plpy.Error('custom message', hint = 'It is test only');
+ $$ LANGUAGE plpythonu;
+ 
+
+   
   
  
   
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
new file mode 100644
index 1f52af7..cb792eb
*** a/src/pl/plpython/expected/plpython_error.out
--- b/src/pl/plpython/expected/plpython_error.out
*** EXCEPTION WHEN SQLSTATE 'SILLY' THEN
*** 422,424 
--- 422,486 
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ /* the possibility to set fields of custom exception
+  */
+ DO $$
+ raise plpy.Error('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.Error: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 4, in 
+ hint = 'This is hint text.')
+ PL/Python anonymous code block
+ \set VERBOSITY verbose
+ DO $$
+ raise plpy.Error('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.',
+ sqlstate = 'SILLY',
+ schema = 'any info about schema',
+ table = 'any info about table',
+ column = 'any info about column',
+ datatype = 'any info about datatype',
+ constraint = 'any info about constraint')
+ $$ LANGUAGE plpythonu;
+ ERROR:  SILLY: plpy.Error: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 10, in 
+ constraint = 'any info about constraint')
+ PL/Python anonymous code block
+ SCHEMA NAME:  any info about schema
+ TABLE NAME:  any info about table
+ COLUMN NAME:  any info about column
+ DATATYPE NAME:  any info about datatype
+ CONSTRAINT NAME:  any info about constraint
+ LOCATION:  PLy_elog, plpy_elog.c:122
+ \set VERBOSITY default
+ DO $$
+ raise plpy.Error(detail = 'This is detail text')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.Error: 
+ DETAIL:  This is detail text
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 2, in 
+ raise plpy.Error(detail = 'This is detail text')
+ PL/Python anonymous code block
+ DO $$
+ raise plpy.Error();
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.Error: 
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 2, in 
+ raise plpy.Error();
+ PL/Python anonymous code block
+ DO $$
+ raise plpy.Error(sqlstate = 'wrong sql state');
+ $$ LANGUAGE plpythonu;
+ ERROR:  could not create Error object (invalid SQLSTATE code)
+ CONTEXT:  PL/Python anonymous code block
diff --git a/src/pl/plpython/expected/plpython_error_0.out b/src/pl/plpython/expected/plpython_error_0.out
new file mode 100644
index 5323906..3d83a53
*** a/src/pl/plpython/expected/plpython_error_0.out
--- b/src/pl/plpython/expected/plpython_error_0.out
*** EXCEPTION WHEN SQLSTATE 'SILLY' THEN
*** 422,424 
--- 422,486 
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ /* the possibility to set fields of custom exception
+  */
+ DO $$
+ raise plpy.Error('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.Error: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 4, in 
+ hint = 'This is hint text.')
+ PL/Python anonymous code block
+ \set VERBOSITY verbose
+ DO $$
+ raise plpy.Error('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.',
+ sqlstate = 'SILLY',
+ schema = 'any info about schema',
+ 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

Hi

2015-11-16 23:40 GMT+01:00 Peter Eisentraut :

> On 11/15/15 11:29 PM, Pavel Stehule wrote:
> >
> >
> > 2015-11-16 5:20 GMT+01:00 Peter Eisentraut  > >:
> >
> > I don't think it's right to reuse SPIError for this.  SPIError is
> > clearly meant to signal an error in the SPI calls.  Of course, we
> can't
> > stop users from raising whatever exception they want, but if we're
> going
> > to advertise that users can raise exceptions, then we should create
> > separate exception classes.
> >
> > I suppose the proper way to set this up would be to create a base
> class
> > like plpy.Error and derive SPIError from that.
> >
> >
> > Do you have some ideas about the name of this class?
>
> I think plpy.Error is fine.
>
>
here is updated patch - work with 2.x Python.

I have 3.x Python broken on my fedora, so I should not do tests on 3.x.

Regards

Pavel
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index 015bbad..51bc48e
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 1205,1210 
--- 1205,1228 
  approximately the same functionality
 

+ 
+   
+Raising Errors
+ 
+
+ 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 ]).
+
+
+ An example of raising custom exception could be written as:
+ 
+ DO $$
+   raise plpy.SPIError('custom message', hint = 'It is test only');
+ $$ LANGUAGE plpythonu;
+ 
+
+   
   
  
   
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
new file mode 100644
index 1f52af7..cb792eb
*** a/src/pl/plpython/expected/plpython_error.out
--- b/src/pl/plpython/expected/plpython_error.out
*** EXCEPTION WHEN SQLSTATE 'SILLY' THEN
*** 422,424 
--- 422,486 
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ /* the possibility to set fields of custom exception
+  */
+ DO $$
+ raise plpy.Error('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.Error: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 4, in 
+ hint = 'This is hint text.')
+ PL/Python anonymous code block
+ \set VERBOSITY verbose
+ DO $$
+ raise plpy.Error('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.',
+ sqlstate = 'SILLY',
+ schema = 'any info about schema',
+ table = 'any info about table',
+ column = 'any info about column',
+ datatype = 'any info about datatype',
+ constraint = 'any info about constraint')
+ $$ LANGUAGE plpythonu;
+ ERROR:  SILLY: plpy.Error: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 10, in 
+ constraint = 'any info about constraint')
+ PL/Python anonymous code block
+ SCHEMA NAME:  any info about schema
+ TABLE NAME:  any info about table
+ COLUMN NAME:  any info about column
+ DATATYPE NAME:  any info about datatype
+ CONSTRAINT NAME:  any info about constraint
+ LOCATION:  PLy_elog, plpy_elog.c:122
+ \set VERBOSITY default
+ DO $$
+ raise plpy.Error(detail = 'This is detail text')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.Error: 
+ DETAIL:  This is detail text
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 2, in 
+ raise plpy.Error(detail = 'This is detail text')
+ PL/Python anonymous code block
+ DO $$
+ raise plpy.Error();
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.Error: 
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 2, in 
+ raise plpy.Error();
+ PL/Python anonymous code block
+ DO $$
+ raise plpy.Error(sqlstate = 'wrong sql state');
+ $$ LANGUAGE plpythonu;
+ ERROR:  could not create Error object (invalid SQLSTATE code)
+ CONTEXT:  PL/Python anonymous code block
diff --git a/src/pl/plpython/expected/plpython_error_0.out b/src/pl/plpython/expected/plpython_error_0.out
new file mode 100644
index 5323906..19b1a9b
*** a/src/pl/plpython/expected/plpython_error_0.out
--- b/src/pl/plpython/expected/plpython_error_0.out
*** EXCEPTION WHEN SQLSTATE 'SILLY' THEN
*** 422,424 
--- 422,486 
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ /* the possibility to set fields of custom exception
+  */
+ DO $$
+ raise plpy.SPIError('This is message text.',
+ detail = 'This is detail 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Michael Paquier]

On Sun, Nov 29, 2015 at 4:00 PM, Pavel Stehule 
wrote:

> 2015-11-28 13:11 GMT+01:00 Michael Paquier :
>
>> On Sat, Nov 28, 2015 at 1:57 AM, Pavel Stehule 
>> wrote:
>> >
>> >
>> > 2015-11-27 17:54 GMT+01:00 Teodor Sigaev :
>> >>
>> >> Is this patch in 'Waiting on Author' state actually?
>>
>> I am marking it as returned with feedback for this CF then.
>>
>
> please, can revert it?
>

I have moved it to the next CF instead as you are still working on it. It
is going to be time to finish the current CF soon.
-- 
Michael

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2015-11-29 13:42 GMT+01:00 Michael Paquier :

>
>
> On Sun, Nov 29, 2015 at 4:00 PM, Pavel Stehule 
> wrote:
>
>> 2015-11-28 13:11 GMT+01:00 Michael Paquier :
>>
>>> On Sat, Nov 28, 2015 at 1:57 AM, Pavel Stehule 
>>> wrote:
>>> >
>>> >
>>> > 2015-11-27 17:54 GMT+01:00 Teodor Sigaev :
>>> >>
>>> >> Is this patch in 'Waiting on Author' state actually?
>>>
>>> I am marking it as returned with feedback for this CF then.
>>>
>>
>> please, can revert it?
>>
>
> I have moved it to the next CF instead as you are still working on it. It
> is going to be time to finish the current CF soon.
>

ook

Regards

Pavel


> --
> Michael
>

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Michael Paquier]

On Sat, Nov 28, 2015 at 1:57 AM, Pavel Stehule  wrote:
>
>
> 2015-11-27 17:54 GMT+01:00 Teodor Sigaev :
>>
>> Is this patch in 'Waiting on Author' state actually?

I am marking it as returned with feedback for this CF then.
-- 
Michael


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2015-11-28 13:11 GMT+01:00 Michael Paquier :

> On Sat, Nov 28, 2015 at 1:57 AM, Pavel Stehule 
> wrote:
> >
> >
> > 2015-11-27 17:54 GMT+01:00 Teodor Sigaev :
> >>
> >> Is this patch in 'Waiting on Author' state actually?
>
> I am marking it as returned with feedback for this CF then.
>

please, can revert it?

I though so Peter request will require some massive change, but I had to
change few lines.

So this patch is still ready for review - minimally for Peter's review.

Regards

Pavel


> --
> Michael
>

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2015-11-27 17:54 GMT+01:00 Teodor Sigaev :

> Is this patch in 'Waiting on Author' state actually?


yes

I'll try redesign patch by Peter's proposal

Pavel


>
>
>  I don't think it's right to reuse SPIError for this.  SPIError is
>>>  clearly meant to signal an error in the SPI calls.  Of course, we
>>> can't
>>>  stop users from raising whatever exception they want, but if we're
>>> going
>>>  to advertise that users can raise exceptions, then we should create
>>>  separate exception classes.
>>>
>>>  I suppose the proper way to set this up would be to create a base
>>> class
>>>  like plpy.Error and derive SPIError from that.
>>>
>>>
>>> Do you have some ideas about the name of this class?
>>>
>>
>> I think plpy.Error is fine.
>>
>>
>>
>>
> --
> Teodor Sigaev   E-mail: teo...@sigaev.ru
>WWW:
> http://www.sigaev.ru/
>

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Teodor Sigaev]

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: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Peter Eisentraut]

On 11/15/15 11:29 PM, Pavel Stehule wrote:
> 
> 
> 2015-11-16 5:20 GMT+01:00 Peter Eisentraut  >:
> 
> 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.



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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Peter Eisentraut]

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.



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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2015-11-16 5:20 GMT+01:00 Peter Eisentraut :

> I don't think it's right to reuse SPIError for this.  SPIError is
> clearly meant to signal an error in the SPI calls.  Of course, we can't
> stop users from raising whatever exception they want, but if we're going
> to advertise that users can raise exceptions, then we should create
> separate exception classes.
>
> I suppose the proper way to set this up would be to create a base class
> like plpy.Error and derive SPIError from that.
>

Do you have some ideas about the name of this class?

Regards

Pavel

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2015-11-05 7:24 GMT+01:00 Catalin Iacob :

> On Wed, Nov 4, 2015 at 10:12 AM, Pavel Stehule 
> 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
> plpy.SPIError can be raised right?
>
> And most importantly, for Python 3.5 there is a plpython_error_5.out
> which is needed because of an alternative exception message in that
> version. You didn't update this file, this makes the tests fail on
> Python3.5.
>
> Since you might not have Python 3.5 easily available I've attached a
> patch to plpython_error_5.out which makes the tests pass, you can fold
> this into your patch.
>

I needed to understand the support for Python 3.5.

The patch with the fix is attached regress tests 3.5 Python

Regards

Pavel
commit f3ab75afe0d9b31118d461a346e3c56ceb09c238
Author: Pavel Stehule 
Date:   Sat Oct 17 20:11:56 2015 +0200

inital

diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
index 015bbad..51bc48e 100644
--- a/doc/src/sgml/plpython.sgml
+++ b/doc/src/sgml/plpython.sgml
@@ -1205,6 +1205,24 @@ $$ LANGUAGE plpythonu;
 approximately the same functionality

   
+
+  
+   Raising Errors
+
+   
+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 ]).
+   
+   
+An example of raising custom exception could be written as:
+
+DO $$
+  raise plpy.SPIError('custom message', hint = 'It is test only');
+$$ LANGUAGE plpythonu;
+
+   
+  
  
 
  
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
index 1f52af7..435a5c2 100644
--- a/src/pl/plpython/expected/plpython_error.out
+++ b/src/pl/plpython/expected/plpython_error.out
@@ -422,3 +422,65 @@ EXCEPTION WHEN SQLSTATE 'SILLY' THEN
 	-- NOOP
 END
 $$ LANGUAGE plpgsql;
+/* the possibility to set fields of custom exception
+ */
+DO $$
+raise plpy.SPIError('This is message text.',
+detail = 'This is detail text',
+hint = 'This is hint text.')
+$$ LANGUAGE plpythonu;
+ERROR:  plpy.SPIError: This is message text.
+DETAIL:  This is detail text
+HINT:  This is hint text.
+CONTEXT:  Traceback (most recent call last):
+  PL/Python anonymous code block, line 4, in 
+hint = 'This is hint text.')
+PL/Python anonymous code block
+\set VERBOSITY verbose
+DO $$
+raise plpy.SPIError('This is message text.',
+detail = 'This is detail text',
+hint = 'This is hint text.',
+sqlstate = 'SILLY',
+schema = 'any info about schema',
+table = 'any info about table',
+column = 'any info about column',
+datatype = 'any info about datatype',
+constraint = 'any info about constraint')
+$$ LANGUAGE plpythonu;
+ERROR:  SILLY: plpy.SPIError: This is message text.
+DETAIL:  This is detail text
+HINT:  This is hint text.
+CONTEXT:  Traceback (most recent call last):
+  PL/Python anonymous code block, line 10, in 
+constraint = 'any info about constraint')
+PL/Python anonymous code block
+SCHEMA NAME:  any info about schema
+TABLE NAME:  any info about table
+COLUMN NAME:  any info about column
+DATATYPE NAME:  any info about datatype
+CONSTRAINT NAME:  any info about constraint
+LOCATION:  PLy_elog, plpy_elog.c:122
+\set VERBOSITY default
+DO $$
+raise plpy.SPIError(detail = 'This is detail text')
+$$ LANGUAGE plpythonu;
+ERROR:  plpy.SPIError: 
+DETAIL:  This is detail text
+CONTEXT:  Traceback (most recent call last):
+  PL/Python anonymous code block, line 2, in 
+raise plpy.SPIError(detail = 'This is detail text')
+PL/Python anonymous code block
+DO $$
+raise plpy.SPIError();
+$$ LANGUAGE plpythonu;
+ERROR:  plpy.SPIError: 
+CONTEXT:  Traceback (most recent call last):
+  PL/Python anonymous code block, line 2, in 
+raise plpy.SPIError();
+PL/Python anonymous code block
+DO $$
+raise plpy.SPIError(sqlstate = 'wrong sql state');
+$$ LANGUAGE plpythonu;
+ERROR:  could not create SPIError object (invalid SQLSTATE code)
+CONTEXT:  PL/Python anonymous code block
diff --git a/src/pl/plpython/expected/plpython_error_5.out b/src/pl/plpython/expected/plpython_error_5.out
index 5ff46ca..8788bde 100644
--- a/src/pl/plpython/expected/plpython_error_5.out
+++ b/src/pl/plpython/expected/plpython_error_5.out
@@ -422,3 +422,65 @@ EXCEPTION WHEN SQLSTATE 'SILLY' THEN
 	-- NOOP
 END
 $$ LANGUAGE plpgsql;
+/* the possibility to set fields of custom exception
+ */
+DO $$
+raise plpy.SPIError('This is message text.',
+

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Mon, Nov 9, 2015 at 6:31 PM, Pavel Stehule  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


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2015-11-09 19:21 GMT+01:00 Catalin Iacob :

> On Mon, Nov 9, 2015 at 6:31 PM, Pavel Stehule 
> wrote:
> > merged your patch
>
> So, I just that tested version 08 is the same as the previous patch +
> my change and I already tested that on Python 2.4, 2.5, 2.6, 2.7 and
> 3.5.
>
> All previous comments addressed so I'll mark this Ready for Committer.
>

Thank you very much for help

Regards

Pavel


>
> Thanks Pavel
>

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2015-11-09 16:46 GMT+01:00 Catalin Iacob :

> On Mon, Nov 9, 2015 at 2:58 PM, Pavel Stehule 
> wrote:
> > I needed to understand the support for Python 3.5.
> >
> > The patch with the fix is attached regress tests 3.5 Python
>
> I wanted to type a reply this morning to explain and then I noticed
> there's another file (_0.out) for Python2.4 and older (as explained by
> src/pl/plpython/expected/README) so tests there fail as well. I built
> Python 2.4.6 and then Postgres against it and updated the expected
> output for 2.4 as well. Find the changes for 2.4 attached. You can add
> those to your patch.
>

I forgot it


> While I was at it I tested 2.5 and 2.6 as well, they also pass all
> tests. So with the 2.4 changes attached I think we're done.
>

merged your patch


>
> BTW, the alternative output files for pg_regress are explained at
> http://www.postgresql.org/docs/devel/static/regress-variant.html


I didn't know it - nice, maybe I use it in orafce.

Thank you

Regards

Pavel
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index 015bbad..51bc48e
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 1205,1210 
--- 1205,1228 
  approximately the same functionality
 

+ 
+   
+Raising Errors
+ 
+
+ 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 ]).
+
+
+ An example of raising custom exception could be written as:
+ 
+ DO $$
+   raise plpy.SPIError('custom message', hint = 'It is test only');
+ $$ LANGUAGE plpythonu;
+ 
+
+   
   
  
   
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
new file mode 100644
index 1f52af7..435a5c2
*** a/src/pl/plpython/expected/plpython_error.out
--- b/src/pl/plpython/expected/plpython_error.out
*** EXCEPTION WHEN SQLSTATE 'SILLY' THEN
*** 422,424 
--- 422,486 
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ /* the possibility to set fields of custom exception
+  */
+ DO $$
+ raise plpy.SPIError('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.SPIError: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 4, in 
+ hint = 'This is hint text.')
+ PL/Python anonymous code block
+ \set VERBOSITY verbose
+ DO $$
+ raise plpy.SPIError('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.',
+ sqlstate = 'SILLY',
+ schema = 'any info about schema',
+ table = 'any info about table',
+ column = 'any info about column',
+ datatype = 'any info about datatype',
+ constraint = 'any info about constraint')
+ $$ LANGUAGE plpythonu;
+ ERROR:  SILLY: plpy.SPIError: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 10, in 
+ constraint = 'any info about constraint')
+ PL/Python anonymous code block
+ SCHEMA NAME:  any info about schema
+ TABLE NAME:  any info about table
+ COLUMN NAME:  any info about column
+ DATATYPE NAME:  any info about datatype
+ CONSTRAINT NAME:  any info about constraint
+ LOCATION:  PLy_elog, plpy_elog.c:122
+ \set VERBOSITY default
+ DO $$
+ raise plpy.SPIError(detail = 'This is detail text')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.SPIError: 
+ DETAIL:  This is detail text
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 2, in 
+ raise plpy.SPIError(detail = 'This is detail text')
+ PL/Python anonymous code block
+ DO $$
+ raise plpy.SPIError();
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.SPIError: 
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 2, in 
+ raise plpy.SPIError();
+ PL/Python anonymous code block
+ DO $$
+ raise plpy.SPIError(sqlstate = 'wrong sql state');
+ $$ LANGUAGE plpythonu;
+ ERROR:  could not create SPIError object (invalid SQLSTATE code)
+ CONTEXT:  PL/Python anonymous code block
diff --git a/src/pl/plpython/expected/plpython_error_0.out b/src/pl/plpython/expected/plpython_error_0.out
new file mode 100644
index 5323906..19b1a9b
*** a/src/pl/plpython/expected/plpython_error_0.out
--- b/src/pl/plpython/expected/plpython_error_0.out
*** EXCEPTION WHEN SQLSTATE 'SILLY' THEN
*** 422,424 
--- 422,486 
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ /* the possibility to set 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Mon, Nov 9, 2015 at 2:58 PM, Pavel Stehule  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


adjust_py24_expected.patch
Description: binary/octet-stream

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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2015-11-05 7:24 GMT+01:00 Catalin Iacob :

> On Wed, Nov 4, 2015 at 10:12 AM, Pavel Stehule 
> 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
> plpy.SPIError can be raised right?
>
>
fixed


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

this fix will be pretty hard - if I'll fix for 3.5, then any other will be
broken

I can move these tests to separate file and run some tests for 3.5 and
other for older. But it is pretty ugly - and we have not any similar
workaround elsewhere.

I checked the diff and looks so only language identifiers are different


7c7
< +$$ LANGUAGE plpython3u;
---
> +$$ LANGUAGE plpythonu;
26c26
< +$$ LANGUAGE plpython3u;
---
> +$$ LANGUAGE plpythonu;
43c43
< +$$ LANGUAGE plpython3u;
---
> +$$ LANGUAGE plpythonu;
52c52
< +$$ LANGUAGE plpython3u;
---
> +$$ LANGUAGE plpythonu;
60c60
< +$$ LANGUAGE plpython3u;
---
> +$$ LANGUAGE plpythonu;

It is strange - I cannot to understand how is possible so other Python's
tests are working in your comp. I don't know where is the core of this
issue, but I am inclined to think so some wrong is in your environment. The
identifier plpython3u shouldn't be used in tests.

Regards

Pavel


> Since you might not have Python 3.5 easily available I've attached a
> patch to plpython_error_5.out which makes the tests pass, you can fold
> this into your patch.
>
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index 015bbad..51bc48e
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 1205,1210 
--- 1205,1228 
  approximately the same functionality
 

+ 
+   
+Raising Errors
+ 
+
+ 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 ]).
+
+
+ An example of raising custom exception could be written as:
+ 
+ DO $$
+   raise plpy.SPIError('custom message', hint = 'It is test only');
+ $$ LANGUAGE plpythonu;
+ 
+
+   
   
  
   
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
new file mode 100644
index 1f52af7..435a5c2
*** a/src/pl/plpython/expected/plpython_error.out
--- b/src/pl/plpython/expected/plpython_error.out
*** EXCEPTION WHEN SQLSTATE 'SILLY' THEN
*** 422,424 
--- 422,486 
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ /* the possibility to set fields of custom exception
+  */
+ DO $$
+ raise plpy.SPIError('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.SPIError: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 4, in 
+ hint = 'This is hint text.')
+ PL/Python anonymous code block
+ \set VERBOSITY verbose
+ DO $$
+ raise plpy.SPIError('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.',
+ sqlstate = 'SILLY',
+ schema = 'any info about schema',
+ table = 'any info about table',
+ column = 'any info about column',
+ datatype = 'any info about datatype',
+ constraint = 'any info about constraint')
+ $$ LANGUAGE plpythonu;
+ ERROR:  SILLY: plpy.SPIError: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 10, in 
+ constraint = 'any info about constraint')
+ PL/Python anonymous code block
+ SCHEMA NAME:  any info about schema
+ TABLE NAME:  any info about table
+ COLUMN NAME:  any info about column
+ DATATYPE NAME:  any info about datatype
+ CONSTRAINT NAME:  any info about constraint
+ LOCATION:  PLy_elog, plpy_elog.c:122
+ \set VERBOSITY default
+ DO $$
+ raise plpy.SPIError(detail = 'This is detail text')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.SPIError: 
+ DETAIL:  This is detail text
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 2, in 
+ raise plpy.SPIError(detail = 'This is detail text')
+ PL/Python anonymous code block
+ DO $$
+ raise plpy.SPIError();
+ $$ LANGUAGE plpythonu;
+ ERROR:  

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Wed, Nov 4, 2015 at 10:12 AM, Pavel Stehule  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
plpy.SPIError 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.


adjust_py35_expected.patch
Description: binary/octet-stream

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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

Hi

2015-11-04 7:06 GMT+01:00 Catalin Iacob :

> Sorry, you're right, I didn't notice the x = plpy.SPIError() test.
>
> I did notice that you included the kw != NULL, I was explaining why it
> really is needed even though it *seems* the code also works without
>
it.
>

It helped me lot of, thank you


>
> There's just the doc part left then.
>

done

Regards

Pavel
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index 015bbad..10aeb35
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 1205,1210 
--- 1205,1228 
  approximately the same functionality
 

+ 
+   
+Raising Errors
+ 
+
+ 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 ]).
+
+
+ An example of raising custom excation could be written as:
+ 
+ DO $$
+   raise plpy.SPIError('custom message', hint = 'It is test only');
+ $$ LANGUAGE plpythonu;
+ 
+
+   
   
  
   
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
new file mode 100644
index 1f52af7..435a5c2
*** a/src/pl/plpython/expected/plpython_error.out
--- b/src/pl/plpython/expected/plpython_error.out
*** EXCEPTION WHEN SQLSTATE 'SILLY' THEN
*** 422,424 
--- 422,486 
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ /* the possibility to set fields of custom exception
+  */
+ DO $$
+ raise plpy.SPIError('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.SPIError: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 4, in 
+ hint = 'This is hint text.')
+ PL/Python anonymous code block
+ \set VERBOSITY verbose
+ DO $$
+ raise plpy.SPIError('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.',
+ sqlstate = 'SILLY',
+ schema = 'any info about schema',
+ table = 'any info about table',
+ column = 'any info about column',
+ datatype = 'any info about datatype',
+ constraint = 'any info about constraint')
+ $$ LANGUAGE plpythonu;
+ ERROR:  SILLY: plpy.SPIError: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 10, in 
+ constraint = 'any info about constraint')
+ PL/Python anonymous code block
+ SCHEMA NAME:  any info about schema
+ TABLE NAME:  any info about table
+ COLUMN NAME:  any info about column
+ DATATYPE NAME:  any info about datatype
+ CONSTRAINT NAME:  any info about constraint
+ LOCATION:  PLy_elog, plpy_elog.c:122
+ \set VERBOSITY default
+ DO $$
+ raise plpy.SPIError(detail = 'This is detail text')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.SPIError: 
+ DETAIL:  This is detail text
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 2, in 
+ raise plpy.SPIError(detail = 'This is detail text')
+ PL/Python anonymous code block
+ DO $$
+ raise plpy.SPIError();
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.SPIError: 
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 2, in 
+ raise plpy.SPIError();
+ PL/Python anonymous code block
+ DO $$
+ raise plpy.SPIError(sqlstate = 'wrong sql state');
+ $$ LANGUAGE plpythonu;
+ ERROR:  could not create SPIError object (invalid SQLSTATE code)
+ CONTEXT:  PL/Python anonymous code block
diff --git a/src/pl/plpython/plpy_elog.c b/src/pl/plpython/plpy_elog.c
new file mode 100644
index 15406d6..a835af9
*** a/src/pl/plpython/plpy_elog.c
--- b/src/pl/plpython/plpy_elog.c
*** PyObject   *PLy_exc_spi_error = NULL;
*** 23,29 
  
  static void PLy_traceback(char **xmsg, char **tbmsg, int *tb_depth);
  static void PLy_get_spi_error_data(PyObject *exc, int *sqlerrcode, char **detail,
! 	   char **hint, char **query, int *position);
  static char *get_source_line(const char *src, int lineno);
  
  
--- 23,32 
  
  static void PLy_traceback(char **xmsg, char **tbmsg, int *tb_depth);
  static void PLy_get_spi_error_data(PyObject *exc, int *sqlerrcode, char **detail,
! 	   char **hint, char **query, int *position,
! 	   char **schema_name, char **table_name, char **column_name,
! 	   char **datatype_name, char **constraint_name);
! 
  static char *get_source_line(const char *src, int lineno);
  
  
*** PLy_elog(int elevel, const char *fmt,...
*** 51,62 
  	char	   *hint = NULL;
  	char	   *query 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2015-11-02 17:01 GMT+01:00 Catalin Iacob :

> Hello,
>
> Here's a detailed review:
>
> 1. in PLy_spi_error__init__ you need to check kw for NULL before doing
> PyDict_Size(kw) otherwise for plpy.SPIError() you get Bad internal
> call because PyDict_Size expects a real dictionary not NULL
>

PyDict_Size returns -1 when the dictionary is NULL

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

done


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

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


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


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

done


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

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


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


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

done


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


done


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

done


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

done


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

done


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

dome


>
>
> Will mark the patch as Waiting for author.
>

attached new update

Regards

Pavel
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index 015bbad..9333fbe
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 1205,1210 
--- 1205,1235 
  approximately the same functionality
 

+ 
+   
+Raising Errors
+ 
+
+ The plpy module provides several possibilities to
+ to raise a exception:
+
+ 
+
+ 
+  SPIError([ message [, detail [, hint [, sqlstate  [, schema  [, table  [, column  [, datatype  [, constraint ])
+  
+
+An SPIError instance can be constructed using keyword parameters.
+ 
+ DO $$
+   raise plpy.SPIError('custom message', hint = 'It is test only');
+ $$ LANGUAGE plpythonu;
+ 
+
+  
+ 
+
+   
   
  
   
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
new file mode 100644
index 1f52af7..435a5c2
*** a/src/pl/plpython/expected/plpython_error.out
--- b/src/pl/plpython/expected/plpython_error.out
*** EXCEPTION WHEN SQLSTATE 'SILLY' THEN
*** 422,424 
--- 422,486 
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ /* the possibility to set fields of custom exception
+  */
+ DO $$
+ raise plpy.SPIError('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.SPIError: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 4, in 
+ hint = 'This is hint text.')
+ PL/Python anonymous code block
+ \set VERBOSITY verbose
+ DO $$
+ raise plpy.SPIError('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.',
+ sqlstate = 'SILLY',
+ schema = 'any info about schema',
+ table = 'any info about table',
+ column = 'any info about column',
+ datatype = 'any 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2015-11-03 17:13 GMT+01:00 Catalin Iacob :

> On Tue, Nov 3, 2015 at 12:49 PM, Pavel Stehule 
> wrote:
> >> 1. in PLy_spi_error__init__ you need to check kw for NULL before doing
> >> PyDict_Size(kw) otherwise for plpy.SPIError() you get Bad internal
> >> call because PyDict_Size expects a real dictionary not NULL
> >
> >
> > PyDict_Size returns -1 when the dictionary is NULL
> >
> >
> http://grokbase.com/t/python/python-dev/042ft6qaqq/pydict-size-error-return
>
> Yes, but it also sets the error indicator to BadInternalCall. In 2.7
> the code is:
> Py_ssize_t
> PyDict_Size(PyObject *mp)
> {
> if (mp == NULL || !PyDict_Check(mp)) {
> PyErr_BadInternalCall();
> return -1;
> }
> return ((PyDictObject *)mp)->ma_used;
> }
>
> I had a PLy_elog right after the PyDict_Size call for debugging and
> that one was raising BadInternalCall since it checked the error
> indicator. So the NULL check is needed.
>

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


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

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


> >
> >
> > please, can you write some example - I am not able raise described error
>
> The example was plpy.SPIError() but I now realize that, in order to
> see a failure, you need the extra PLy_elog which I had in there.
> But this basic form of the constructor is still an important thing to
> test so please add this as well to the regression test.
>
> >> 5. there is conceptual code duplication between PLy_spi_exception_set
> >> in plpy_spi.c, since that code also constructs an SPIError but from C
> >> and with more info available (edata->internalquery and
> >> edata->internalpos). But making a tuple and assigning it to spidata is
> >> in both. Not sure how this can be improved.
> >
> >
> > I see it, but I don't think, so current code should be changed.
> > PLy_spi_exception_set is controlled directly by fully filled ErrorData
> > structure, __init__ is based only on keyword parameters with possibility
> use
> > inherited data. If I'll build ErrorData in __init__ function and call
> > PLy_spi_exception_set, then the code will be longer and more complex.
>
> Indeed, I don't really see how to improve this but it does bug me a bit.
>
> One more thing,
> +The plpy 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 plpy.SPIError everywhere to
> be consistent.
>

I'll do it tomorrow

>
> Maybe something like (needs markup):
> A plpy.SPIError can be raised from PL/Python, the constructor accepts
> keyword parameters:
>   plpy.SPIError([ message [, detail [, hint [, sqlstate [, schema [,
> table [, column [, datatype [, constraint ])
> then the example
>
> If you fix the doc and add the plpy.SPIError() test I'm happy. I'll
> give it one more test on Python2.7 and 3.5 and mark it Ready for
> Committer.
>

Regards

Pavel

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

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

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

There's just the doc part left then.


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Tue, Nov 3, 2015 at 12:49 PM, Pavel Stehule  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 plpy 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 plpy.SPIError everywhere to
be consistent.

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

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


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

Hello,

Here's a detailed review:

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

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

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

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

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

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

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

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

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

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

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

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


Will mark the patch as Waiting for author.


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2015-10-28 7:25 GMT+01:00 Catalin Iacob :

> On Tue, Oct 27, 2015 at 9:34 AM, Pavel Stehule 
> wrote:
> > Hi
> >
> > 2015-10-23 7:34 GMT+02:00 Catalin Iacob :
> >> The current code doesn't build on Python3 because the 3rd argument of
> >> PyMethod_New, the troubled one you need set to NULL has been removed.
> >> This has to do with the distinction between bound and unbound methods
> >> which is gone in Python3.
> >
> >
> > this part of is well isolated, and we can do switch for Python2 and
> Python3
> > without significant problems.
>
> I had a quick look at this and at least 2 things are needed for Python3:
>
> * using PyInstanceMethod_New instead of PyMethod_New; as
> http://bugs.python.org/issue1587 and
> https://docs.python.org/3/c-api/method.html explain that's the new way
> of binding a PyCFunction to a class, PyMethod_New(func, NULL) fails
>
> * in the PLy_spi_error_methods definition, __init__ has to take
> METH_VARARGS | METH_KEYWORDS not just METH_KEYWORDS; in Python2
> METH_KEYWORDS implied METH_VARARGS so it's not needed (it also doesn't
> hurt) but if I don't add it, in Python3 I get:
> ERROR:  SystemError: Bad call flags in PyCFunction_Call. METH_OLDARGS
> is no longer supported!
>
> >> Still, the above link shows a (more verbose but maybe better)
> >> alternative: don't use PyErr_NewException and instead define an
> >> SPIError type with each slot spelled out explicitly. This will remove
> >> the "is it safe to set the third argument to NULL" question.
> >
> > Should be there some problems, if we lost dependency on basic exception
> > class? Some compatibility issues? Or is possible to create full type with
> > inheritance support?
>
> It's possible to give it a base type, see at
> https://hg.python.org/cpython/rev/429cadbc5b10/ this line before
> calling PyType_Ready:
> PyComError_Type.tp_base = (PyTypeObject*)PyExc_Exception;
>
> PyErr_NewException is a shortcut for defining simple Exception
> deriving types, usually one defines a type with the full PyTypeObject
> definition.
>
> In the meantime, I had a deeper look at the 2.7.10 code and I trust
> that PyMethod_New with the last argument set to NULL is ok. Setting
> that to NULL will lead to the PyMethod representing __init__ im_class
> being NULL. But that PyMethod object is not held onto by C code, it's
> added to the SPIError class' dict. From there, it is always retrieved
> from Python via an instance or via the class (so SPIError().__init__
> or SPIError.__init__) which will lead to instancemethod_descr_get
> being called because it's the tp_descr_get slot of PyMethod_Type and
> that code knows the class you're retrieving the attribute from
> (SPIError in this case), checks if the PyMethod already has a not NULL
> im_class (which SPIError.__init__ doesn't) and, if not, calls
> PyMethod_New again and passes the class (SPIError) as the 3rd
> argument.
>
> Given this, I think it's ok to keep using PyErr_NewException rather
> than spelling out the type explicitly.
>

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

Regards

Pavel
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index 015bbad..bf468e1
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 1205,1210 
--- 1205,1235 
  approximately the same functionality
 

+ 
+   
+Raising Errors
+ 
+
+ The plpy module provides several possibilities to
+ to raise a exception:
+
+ 
+
+ 
+  SPIError([ message [, detail [, hint [, sqlstate  [, schema  [, table  [, column  [, datatype  [, constraint ])
+  
+
+ The constructor of SPIError exception (class) supports keyword parameters. 
+ 
+ DO $$
+   raise plpy.SPIError('custom message', hint = 'It is test only');
+ $$ LANGUAGE plpythonu;
+ 
+
+  
+ 
+
+   
   
  
   
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
new file mode 100644
index 1f52af7..ac985c6
*** a/src/pl/plpython/expected/plpython_error.out
--- b/src/pl/plpython/expected/plpython_error.out
*** EXCEPTION WHEN SQLSTATE 'SILLY' THEN
*** 422,424 
--- 422,473 
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ /* possibility to set all accessable fields in custom exception
+  */
+ DO $$
+ raise plpy.SPIError('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.SPIError: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 4, in 
+ hint = 'This is hint text.')
+ PL/Python anonymous code block
+ \set VERBOSITY verbose
+ 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Tue, Oct 27, 2015 at 9:34 AM, Pavel Stehule  wrote:
> Hi
>
> 2015-10-23 7:34 GMT+02:00 Catalin Iacob :
>> The current code doesn't build on Python3 because the 3rd argument of
>> PyMethod_New, the troubled one you need set to NULL has been removed.
>> This has to do with the distinction between bound and unbound methods
>> which is gone in Python3.
>
>
> this part of is well isolated, and we can do switch for Python2 and Python3
> without significant problems.

I had a quick look at this and at least 2 things are needed for Python3:

* using PyInstanceMethod_New instead of PyMethod_New; as
http://bugs.python.org/issue1587 and
https://docs.python.org/3/c-api/method.html explain that's the new way
of binding a PyCFunction to a class, PyMethod_New(func, NULL) fails

* in the PLy_spi_error_methods definition, __init__ has to take
METH_VARARGS | METH_KEYWORDS not just METH_KEYWORDS; in Python2
METH_KEYWORDS implied METH_VARARGS so it's not needed (it also doesn't
hurt) but if I don't add it, in Python3 I get:
ERROR:  SystemError: Bad call flags in PyCFunction_Call. METH_OLDARGS
is no longer supported!

>> Still, the above link shows a (more verbose but maybe better)
>> alternative: don't use PyErr_NewException and instead define an
>> SPIError type with each slot spelled out explicitly. This will remove
>> the "is it safe to set the third argument to NULL" question.
>
> Should be there some problems, if we lost dependency on basic exception
> class? Some compatibility issues? Or is possible to create full type with
> inheritance support?

It's possible to give it a base type, see at
https://hg.python.org/cpython/rev/429cadbc5b10/ this line before
calling PyType_Ready:
PyComError_Type.tp_base = (PyTypeObject*)PyExc_Exception;

PyErr_NewException is a shortcut for defining simple Exception
deriving types, usually one defines a type with the full PyTypeObject
definition.

In the meantime, I had a deeper look at the 2.7.10 code and I trust
that PyMethod_New with the last argument set to NULL is ok. Setting
that to NULL will lead to the PyMethod representing __init__ im_class
being NULL. But that PyMethod object is not held onto by C code, it's
added to the SPIError class' dict. From there, it is always retrieved
from Python via an instance or via the class (so SPIError().__init__
or SPIError.__init__) which will lead to instancemethod_descr_get
being called because it's the tp_descr_get slot of PyMethod_Type and
that code knows the class you're retrieving the attribute from
(SPIError in this case), checks if the PyMethod already has a not NULL
im_class (which SPIError.__init__ doesn't) and, if not, calls
PyMethod_New again and passes the class (SPIError) as the 3rd
argument.

Given this, I think it's ok to keep using PyErr_NewException rather
than spelling out the type explicitly.


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

Hi

2015-10-23 7:34 GMT+02:00 Catalin Iacob :

> On Sat, Oct 17, 2015 at 8:18 PM, Pavel Stehule 
> wrote:
> > here is new patch
> >
> > cleaned, all unwanted artefacts removed. I am not sure if used way for
> > method registration is 100% valid, but I didn't find any related
> > documentation.
>
> Pavel, some notes about the patch, not a full review (yet?).
>
> In PLy_add_exceptions PyDict_New is not checked for failure.
>
> In PLy_spi_error__init__, kw will be NULL if SPIError is called with
> no arguments. With the current code NULL will get passed to
> PyDict_Size which will generate something like SystemError Bad
> internal function call. This also means a test using just SPIError()
> is needed.
> It seems args should never be NULL by the way, if there are no
> arguments it's an empty tuple so the other side of the check is ok.
>
> The current code doesn't build on Python3 because the 3rd argument of
> PyMethod_New, the troubled one you need set to NULL has been removed.
> This has to do with the distinction between bound and unbound methods
> which is gone in Python3.
>

this part of is well isolated, and we can do switch for Python2 and Python3
without significant problems.


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

Should be there some problems, if we lost dependency on basic exception
class? Some compatibility issues? Or is possible to create full type with
inheritance support?


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

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

Regards

Pavel

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Sat, Oct 17, 2015 at 8:18 PM, Pavel Stehule  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  str  im_class 
so the SPIError class name is set in im_class from somewhere. But this
is all moot if you use the explicit SPIError type definition.

>> Any help is welcome

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


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

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

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

here is new patch

cleaned, all unwanted artefacts removed. I am not sure if used way for
method registration is 100% valid, but I didn't find any related
documentation.

Regards

Pavel


>
> Any help is welcome
>
> Regards
>
> Pavel
>
>
>>
>>
>> --
>>  Craig Ringer   http://www.2ndQuadrant.com/
>>  PostgreSQL Development, 24x7 Support, Training & Services
>>
>
>
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index 015bbad..bf468e1
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 1205,1210 
--- 1205,1235 
  approximately the same functionality
 

+ 
+   
+Raising Errors
+ 
+
+ The plpy module provides several possibilities to
+ to raise a exception:
+
+ 
+
+ 
+  SPIError([ message [, detail [, hint [, sqlstate  [, schema  [, table  [, column  [, datatype  [, constraint ])
+  
+
+ The constructor of SPIError exception (class) supports keyword parameters. 
+ 
+ DO $$
+   raise plpy.SPIError('custom message', hint = 'It is test only');
+ $$ LANGUAGE plpythonu;
+ 
+
+  
+ 
+
+   
   
  
   
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
new file mode 100644
index 1f52af7..ac985c6
*** a/src/pl/plpython/expected/plpython_error.out
--- b/src/pl/plpython/expected/plpython_error.out
*** EXCEPTION WHEN SQLSTATE 'SILLY' THEN
*** 422,424 
--- 422,473 
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ /* possibility to set all accessable fields in custom exception
+  */
+ DO $$
+ raise plpy.SPIError('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.SPIError: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 4, in 
+ hint = 'This is hint text.')
+ PL/Python anonymous code block
+ \set VERBOSITY verbose
+ DO $$
+ raise plpy.SPIError('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.',
+ sqlstate = 'SILLY',
+ schema = 'any info about schema',
+ table = 'any info about table',
+ column = 'any info about column',
+ datatype = 'any info about datatype',
+ constraint = 'any info about constraint')
+ $$ LANGUAGE plpythonu;
+ ERROR:  SILLY: plpy.SPIError: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 10, in 
+ constraint = 'any info about constraint')
+ PL/Python anonymous code block
+ SCHEMA NAME:  any info about schema
+ TABLE NAME:  any info about table
+ COLUMN NAME:  any info about column
+ DATATYPE NAME:  any info about datatype
+ CONSTRAINT NAME:  any info about constraint
+ LOCATION:  PLy_elog, plpy_elog.c:122
+ \set VERBOSITY default
+ DO $$
+ raise plpy.SPIError(detail = 'This is detail text')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.SPIError: 
+ DETAIL:  This is detail text
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 2, in 
+ raise plpy.SPIError(detail = 'This is detail text')
+ PL/Python anonymous code block
diff --git a/src/pl/plpython/plpy_elog.c b/src/pl/plpython/plpy_elog.c
new file mode 100644
index 15406d6..a835af9
*** a/src/pl/plpython/plpy_elog.c
--- b/src/pl/plpython/plpy_elog.c
*** PyObject   *PLy_exc_spi_error = NULL;
*** 23,29 
  
  static void PLy_traceback(char **xmsg, char **tbmsg, int *tb_depth);
  static void PLy_get_spi_error_data(PyObject *exc, int *sqlerrcode, char **detail,
! 	   char **hint, char **query, int *position);
  static char *get_source_line(const char *src, int lineno);
  
  
--- 23,32 
  
  static void PLy_traceback(char **xmsg, char **tbmsg, int *tb_depth);
  static void PLy_get_spi_error_data(PyObject *exc, int *sqlerrcode, char **detail,
! 	   char **hint, char **query, int *position,
! 	   char **schema_name, char **table_name, char **column_name,
! 	   char **datatype_name, char **constraint_name);
! 
  static char *get_source_line(const char *src, int lineno);
  
  
*** PLy_elog(int elevel, const char *fmt,...
*** 51,62 
  	char	   *hint = NULL;
  	char	   *query = NULL;
  	int			position = 0;
  
  	PyErr_Fetch(, , );
  	if (exc != NULL)
  	{
  		if (PyErr_GivenExceptionMatches(val, PLy_exc_spi_error))
! 			PLy_get_spi_error_data(val, , , , , );
  		else 

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

2015-10-16 8:12 GMT+02:00 Craig Ringer :

> On 16 October 2015 at 02:47, Pavel Stehule 
> wrote:
>
> >  postgres=# do $$
> > x = plpy.SPIError('Nazdarek');
> > x.spidata = (100, "Some detail", "some hint", None, None);
> > raise x;
> > $$ language plpythonu;
>
> Shouldn't that look more like
>
> raise plpy.SPIError(msg="Message", sqlstate="0P001", hint="Turn it on
> and off again") ?
>
> Keyword args are very much the norm for this sort of thing. I recall
> them being pretty reasonable to deal with in the CPython API too, but
> otherwise a trivial Python wrapper in the module can easily adapt the
> interface.
>

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

The code is working

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

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

Any help is welcome

Regards

Pavel


>
>
> --
>  Craig Ringer   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
new file mode 100644
index 1f52af7..c9b5e69
*** a/src/pl/plpython/expected/plpython_error.out
--- b/src/pl/plpython/expected/plpython_error.out
*** EXCEPTION WHEN division_by_zero THEN
*** 408,413 
--- 408,420 
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ ERROR:  spiexceptions.DivisionByZero: None
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python function "plpy_raise_spiexception", line 2, in 
+ raise plpy.spiexceptions.DivisionByZero()
+ PL/Python function "plpy_raise_spiexception"
+ SQL statement "SELECT plpy_raise_spiexception()"
+ PL/pgSQL function inline_code_block line 3 at SQL statement
  /* setting a custom sqlstate should be handled
   */
  CREATE FUNCTION plpy_raise_spiexception_override() RETURNS void AS $$
*** EXCEPTION WHEN SQLSTATE 'SILLY' THEN
*** 422,424 
--- 429,487 
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ ERROR:  spiexceptions.DivisionByZero: None
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python function "plpy_raise_spiexception_override", line 4, in 
+ raise exc
+ PL/Python function "plpy_raise_spiexception_override"
+ SQL statement "SELECT plpy_raise_spiexception_override()"
+ PL/pgSQL function inline_code_block line 3 at SQL statement
+ CREATE FUNCTION nested_error_ereport() RETURNS text
+ 	AS
+ 'def fun1():
+ 	raise plpy.SPIError("HiHi");
+ 
+ def fun2():
+ 	fun1()
+ 
+ def fun3():
+ 	fun2()
+ 
+ fun3()
+ return "not reached"
+ '
+ 	LANGUAGE plpythonu;
+ SELECT nested_error_ereport();
+ ERROR:  plpy.SPIError: HiHi
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python function "nested_error_ereport", line 10, in 
+ fun3()
+   PL/Python function "nested_error_ereport", line 8, in fun3
+ fun2()
+   PL/Python function "nested_error_ereport", line 5, in fun2
+ fun1()
+   PL/Python function "nested_error_ereport", line 2, in fun1
+ raise plpy.SPIError("HiHi");
+ PL/Python function "nested_error_ereport"
+ \set VERBOSITY verbose
+ do $$
+ x = plpy.SPIError("pokus",hint = "some info", sqlstate='AA234'); raise x;
+ $$ language plpythonu;
+ ERROR:  AA234: plpy.SPIError: pokus
+ HINT:  some info
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 2, in 
+ x = plpy.SPIError("pokus",hint = "some info", sqlstate='AA234'); raise x;
+ PL/Python anonymous code block
+ LOCATION:  PLy_elog, plpy_elog.c:119
+ do $$
+ raise plpy.SPIError("pokus",hint = "some info", sqlstate='AA234');
+ $$ language plpythonu;
+ ERROR:  AA234: plpy.SPIError: pokus
+ HINT:  some info
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 2, in 
+ raise plpy.SPIError("pokus",hint = "some info", sqlstate='AA234');
+ PL/Python anonymous code block
+ LOCATION:  PLy_elog, plpy_elog.c:119
+ \set VERBOSITY default
diff --git a/src/pl/plpython/plpy_elog.c b/src/pl/plpython/plpy_elog.c
new file mode 100644
index 15406d6..6825732
*** a/src/pl/plpython/plpy_elog.c
--- b/src/pl/plpython/plpy_elog.c
*** PyObject   *PLy_exc_fatal = NULL;
*** 21,29 
  PyObject   *PLy_exc_spi_error = NULL;
  
  
- static void PLy_traceback(char **xmsg, char **tbmsg, int *tb_depth);
  static void PLy_get_spi_error_data(PyObject *exc, int *sqlerrcode, char **detail,
! 	   char **hint, char **query, int *position);
  static char *get_source_line(const char *src, int lineno);
  
  
--- 21,30 
  PyObject   *PLy_exc_spi_error = NULL;
  
  
  static