Re: proposal: PL/Pythonu - function ereport - Mailing list pgsql-hackers

On Sat, Feb 13, 2016 at 4:26 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> I am sending new version. Current version does:

Hello,

I had a look and I like the big picture.

Tests fail when built against Python 3.5 with stuff like this in
regression.diffs:
-ERROR:  plpy.Error: stop on error
-DETAIL:  some detail
-HINT:  some hint
-CONTEXT:  Traceback (most recent call last):
-  PL/Python function "elog_test", line 18, in <module>
-    plpy.error('stop on error', 'some detail','some hint')
-PL/Python function "elog_test"
+ERROR:  could not convert Python Unicode object to bytes
+DETAIL:  TypeError: bad argument type for built-in operation
+CONTEXT:  PL/Python function "elog_test"

This is related to the use of PyString_AsString and the changed
semantics of that in Python 3 (due to the fact that strings are now
Unicode objects and so on). Didn't have time to dig more deeply into
the exact cause.

Similarly, there are alternative expected test outputs that you didn't
update, for example src/pl/plpython/expected/plpython_types_3.out so
tests fail on some Python versions due to those as well.

> 1. the plpy utility functions can use all ErrorData fields,
> 2. there are no new functions,
> 3. via GUC plpythonu.legacy_custom_exception we can return previous behave,
> 4. only exception Error is raised with natural structure - no composite
> value spidata.
> 5. fields: message, detail and hint are implicitly translated to string - it
> decrease a necessity of legacy mode

I disagree that 5. is a good idea. I think we should just treat
message, detail and hint like the other ones (schema, table etc.). Use
s in PyArg_ParseTupleAndKeywords and let the user explicitly cast to a
string. Explicit is better than implicit. The way you did it you keep
part of the weird old interface which used to cast to string for you.
We shouldn't keep warts of the old behaviour, especially with the GUC
to ask for the old behaviour.

By the way, getting rid of object_to_string and its usage in
PLy_output_kw removed some "ERROR: could not convert Python Unicode
object to bytes" failures in the tests so I'm quite sure that the
usage of PyString_AsString is responsible for those.

I don't like that you set legacy_custom_exception=true in some
existing tests, probably to avoid changing them to the new behaviour.
We should trust that the new behaviour is what we want and the tests
should reflect that. If it's too much work, remember we're asking
users to do the same work to convert their code. We have
elog_test_legacy to test elog_test_legacy=true, the rest of the tests
should use legacy_custom_exception=false.