Re: proposal: PL/Pythonu - function ereport

2016-01-08 7:31 GMT+01:00 Catalin Iacob <iacobcatalin(at)gmail(dot)com>:

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

ok, we will wait.

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

I afraid of compatibility issues - so I am not changing internal format
simply. Some legacy application can be based on detection of spidata
attribute. So I cannot to change this structure for SPIError.

I can change it for BaseError, but this is question. Internally BaseError
and SPIError share same data. So it can be strange if BaseError and
SPIError uses different internal formats.

I am interesting your opinion??

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

a Fatal cannnot be raised - it is a session end. Personally - support of
Fatal level is wrong, and it should not be propagated to user level, but it
is now. And then due consistency I wrote support for fatal level. But hard
to test it.

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