Re: proposal: PL/Pythonu - function ereport

shellOn Thu, Dec 31, 2015 at 11:37 AM, Pavel Stehule
<pavel(dot)stehule(at)gmail(dot)com> 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.