Re: proposal: PL/Pythonu - function ereport

On Tue, Jan 26, 2016 at 5:42 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> I removed from previous patch all OOP related changes. New patch contains
> raise_xxxx functions only. This interface is new generation of previous
> functions: info, notice, warning, error with keyword parameters interface. I
> didn't changed older functions due keeping compatibility.

Hi,

Even without the OOP changes, the exception classes are still there as
the underlying mechanism that error and raise_error use.

I looked at the patch and what I don't like about it is that
raise_error uses SPIError to transport detail, hint etc while error
uses Error. This is inconsistent and confusing.

Take this example:

CREATE OR REPLACE FUNCTION err()
RETURNS int
AS $$
plpy.error('only msg from plpy.error', 'arg2 for error is part of
tuple', 'arg3 also in tuple')
$$ LANGUAGE plpython3u;

SELECT err();

CREATE OR REPLACE FUNCTION raise_err()
RETURNS int
AS $$
plpy.raise_error('only msg from plpy.raise_error', 'arg2 for
raise_error is detail', 'arg3 is hint')
$$ LANGUAGE plpython3u;

SELECT raise_err();

With your patch, this results in:

CREATE FUNCTION
psql:plpy_error_raise_error_difference:9: ERROR: plpy.Error: ('only
msg from plpy.error', 'arg2 for error is part of tuple', 'arg3 also in
tuple')
CONTEXT: Traceback (most recent call last):
PL/Python function "err", line 2, in <module>
plpy.error('only msg from plpy.error', 'arg2 for error is part of
tuple', 'arg3 also in tuple')
PL/Python function "err"
CREATE FUNCTION
psql:plpy_error_raise_error_difference:17: ERROR: plpy.SPIError: only
msg from plpy.raise_error
DETAIL: arg2 for raise_error is detail
HINT: arg3 is hint
CONTEXT: Traceback (most recent call last):
PL/Python function "raise_err", line 2, in <module>
plpy.raise_error('only msg from plpy.raise_error', 'arg2 for
raise_error is detail', 'arg3 is hint')
PL/Python function "raise_err"

From the output you can already see that plpy.error and
plpy.raise_error (even with a single argument) don't use the same
exception: plpy.error uses Error while raise_error uses SPIError. I
think with a single argument they should be identical and with
multiple arguments raise_error should still use Error but use the
arguments as detail, hint etc. In code you had to export a function to
plpy_spi.h to get to the details in SPIError while plpy.error worked
just fine without that.

I've attached two patches on top of yours: first is a comment fix, the
next one is a rough proof of concept for using plpy.Error to carry the
details. This allows undoing the change to export
PLy_spi_exception_set to plpy_spi.h. And then I realized, the changes
you did to SPIError are actually a separate enhancement (report more
details like schema name, table name and so on for the query executed
by SPI). They should be split into a separate patch. With these
patches the output of the above test is:

CREATE FUNCTION
psql:plpy_error_raise_error_difference:9: ERROR: plpy.Error: ('only
msg from plpy.error', 'arg2 for error is part of tuple', 'arg3 also in
tuple')
CONTEXT: Traceback (most recent call last):
PL/Python function "err", line 2, in <module>
plpy.error('only msg from plpy.error', 'arg2 for error is part of
tuple', 'arg3 also in tuple')
PL/Python function "err"
CREATE FUNCTION
psql:plpy_error_raise_error_difference:17: ERROR: plpy.Error: only
msg from plpy.raise_error
DETAIL: arg2 for raise_error is detail
HINT: arg3 is hint
CONTEXT: Traceback (most recent call last):
PL/Python function "raise_err", line 2, in <module>
plpy.raise_error('only msg from plpy.raise_error', 'arg2 for
raise_error is detail', 'arg3 is hint')
PL/Python function "raise_err"

The two approaches are consistent now, both create a plpy.Error. The
patch is not complete, it only handles detail and hint not the others
but it should illustrate the idea.

Looking at the output above, I don't see who would rely on calling
plpy.error with multiple arguments and getting the tuple so I'm
actually in favor of just breaking backward compatibility. Note that
passing multiple arguments isn't even documented. So I would just
change debug, info, error and friends to do what raise_debug,
raise_info, raise_error do. With a single argument behavior stays the
same, with multiple arguments one gets more useful behavior (detail,
hint) instead of the useless tuple. That's my preference but we can
leave the patch with raise and leave the decision to the committer.

What do you think? Jim doesn't like the separate Error being raised. I
don't agree, but even if I would, it's not this patch's job to change
that and Error is already raised today. This patch should be
consistent with the status quo and therefore be similar to plpy.error.
If Error is bad, it should be replaced by SPIError everywhere
separately.

Next week I'll send a patch to improve the docs.