Re: proposal: PL/Pythonu - function ereport

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" <
iacobcatalin(at)gmail(dot)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 <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.
>

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

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

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

ok

The wrong is duality of Error and SPIError, because it is only one real
exception.

The correct naming for current situation are names: PoorError (message
only) and RichError (full set of Edata fields). This is strange from Pg
perspective and still is strange from Python view.

I agree, we should to unify a exception from (raise, elog) functions and
Error has little bit better sense for me. The disadvantage is redundant
code for filling and reading exception properties (I have to support
SPIError), but in this case it is less wrong than strange behave.

Regards

Pavel

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