Re: [PATCH] Fix documentation about PL/Python exception handling

Hi lists,

It seems that PL/Python exception functions (plpy.error, plpy.fatal)
and exception types (plpy.Error, plpy.Fatal) have never worked as
documented, and the behavior is quite surprising.

This is an attempt at properly documenting the current semantics. I
tested these behaviors on PostgreSQL 8.1.22 as well as 9.1alpha2.

Patch 1 updates the documentation in doc/src/sgml/plpython.sgml
Patch 2 adds tests to src/pl/plpython/sql/plpython_error.sql

Should I submit separate documentation patches for older maintenance
releases? Versions 8.1 - 8.4 contain the same incorrect text, but in
the "Database Access" section.

Regards,
Marti

Marti Raudsepp escreveu:
> Should I submit separate documentation patches for older maintenance
> releases? Versions 8.1 - 8.4 contain the same incorrect text, but in
> the "Database Access" section.
>
Sure. But bear in mind that we don't usually backpatch documentation unless
there is incorrect information; improvement belongs to HEAD.

--
Euler Taveira de Oliveira
http://www.timbira.com/

On Wed, Nov 10, 2010 at 16:07, Euler Taveira de Oliveira
<euler(at)timbira(dot)com> wrote:
> Sure. But bear in mind that we don't usually backpatch documentation unless
> there is incorrect information; improvement belongs to HEAD.

Yeah, it is quite incorrect (as you can see by comparing test results
from 1st post with the documentation).

Attached is a documentation patch that will apply to PostgreSQL 8.1 - 8.4

Regards,
Marti

Marti Raudsepp <marti(at)juffo(dot)org> writes:
> On Wed, Nov 10, 2010 at 16:07, Euler Taveira de Oliveira
> <euler(at)timbira(dot)com> wrote:
>> Sure. But bear in mind that we don't usually backpatch documentation unless
>> there is incorrect information; improvement belongs to HEAD.

> Yeah, it is quite incorrect (as you can see by comparing test results
> from 1st post with the documentation).

> Attached is a documentation patch that will apply to PostgreSQL 8.1 - 8.4

Hmmm ... I'm less than excited about documenting agreed-to-be-broken
behavior in exquisite detail. Doing so will just encourage people to
rely on it. It's particularly unfriendly to document it in detail
without pointing out that it's likely to change.

My inclination is to forget about these doc changes and spend our time
on fixing the behavior instead.

regards, tom lane

On Mon, Nov 15, 2010 at 22:25, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> My inclination is to forget about these doc changes and spend our time
> on fixing the behavior instead.

I thought the first step in fixing a problem is admitting the problem ;)
But you raise a fair point.

Regards,
Marti

Tom Lane wrote:
> Hmmm ... I'm less than excited about documenting agreed-to-be-broken
> behavior in exquisite detail. Doing so will just encourage people to
> rely on it. It's particularly unfriendly to document it in detail
> without pointing out that it's likely to change.
>
> My inclination is to forget about these doc changes and spend our time
> on fixing the behavior instead.
>

The open question for this one then is the right way for exceptions to
act. Marti, since you've looked at this a bit already, any thoughts on
what that should look like? You initially commented that what happens
was surprising, but I'm not clear on what you expected to happen instead.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services and Support www.2ndQuadrant.us

On Fri, Nov 19, 2010 at 05:29, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
> The open question for this one then is the right way for exceptions to act.
>  Marti, since you've looked at this a bit already, any thoughts on what that
> should look like?  You initially commented that what happens was surprising,
> but I'm not clear on what you expected to happen instead.

The documented behavior would make a lot more sense.

For instance, currently, when you call plpy.error(), the exception
that's thrown can be caught, but even if you handle it, the function
still ends up failing. Similarly you CANNOT handle SQL errors -- at
all! All errors are fatal.

CREATE FUNCTION foo() RETURNS text
AS
'try:
plpy.execute("SELECT syntax error")
except:
plpy.notice("exception caught but cannot ignore")
return "retval"
'
LANGUAGE plpythonu;

WARNING: PL/Python: plpy.SPIError: unrecognized error in PLy_spi_execute_query
CONTEXT: PL/Python function "foo"
NOTICE: exception caught but cannot ignore
CONTEXT: PL/Python function "foo"
ERROR: column "syntax" does not exist
LINE 1: SELECT syntax error
^
QUERY: SELECT syntax error
CONTEXT: PL/Python function "foo"

Notice how the return value in the above is ignored, although the
NOTICE is still output. Any new exceptions raised in the exception
handler -- such as coding errors -- are also silently ignored.

Another surprise is that plpy.Error and plpy.Fatal are just empty
exception subclasses -- they behave nothing like
plpy.error()/plpy.fatal(). Since they're normal Python exceptions,
they can be caught and silenced. But an unhandled plpy.Fatal still
results in an ERROR level message, go figure.

I'm surprised that PL/Python got merged into Postgres at all without
proper error handling.

Regards,
Marti

Added to TODO:

/* PL/Python */ Add: |Improve behaviour of exception functions
and types

---------------------------------------------------------------------------

Marti Raudsepp wrote:
> Hi lists,
>
> It seems that PL/Python exception functions (plpy.error, plpy.fatal)
> and exception types (plpy.Error, plpy.Fatal) have never worked as
> documented, and the behavior is quite surprising.
>
> This is an attempt at properly documenting the current semantics. I
> tested these behaviors on PostgreSQL 8.1.22 as well as 9.1alpha2.
>
> Patch 1 updates the documentation in doc/src/sgml/plpython.sgml
> Patch 2 adds tests to src/pl/plpython/sql/plpython_error.sql
>
> Should I submit separate documentation patches for older maintenance
> releases? Versions 8.1 - 8.4 contain the same incorrect text, but in
> the "Database Access" section.
>
> Regards,
> Marti

[ Attachment, skipping... ]

[ Attachment, skipping... ]

>
> --
> Sent via pgsql-docs mailing list (pgsql-docs(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-docs

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian wrote:
> Added to TODO:
>
> /* PL/Python */ Add: |Improve behaviour of exception functions
> and types
>

I updated that to also link to Marti's message with more details about
the problem and potential solutions here, to speed up future archive
trawling. At this point I think the only sensible thing is to mark the
submitted documentation patch "Returned with feedback". Then we wait to
see if someone gets motivated to provide the more invasive behavior fix
that seems the preferred improvement here instead. Maybe there's an
advance slimmed down docs patch that makes sense too, for briefly
documenting the limitations without so much detail that the description
turns into a backward compatibility requirement.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD

On tis, 2010-11-09 at 18:40 +0200, Marti Raudsepp wrote:
> It seems that PL/Python exception functions (plpy.error, plpy.fatal)
> and exception types (plpy.Error, plpy.Fatal) have never worked as
> documented, and the behavior is quite surprising.

This is now changed in 9.1devel, so I consider this matter closed.