Re: proposal: PL/Pythonu - function ereport

2016-01-11 17:05 GMT+01:00 Catalin Iacob <iacobcatalin(at)gmail(dot)com>:

> On Fri, Jan 8, 2016 at 7:56 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >> 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.
>
> Indeed, I wasn't suggesting changing it for SPIError, that needs to
> stay spidata.
>
> > 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??
>
> Yes, it's not great if BaseError and SPIError have different ways but
> I think having BaseError have a member (spidata) named after one of
> its subclasses is even worse.
>
> I would say detail, hint etc belong to BaseError and I would make them
> different attributes of BaseError instances instead of one big
> BaseError.data tuple similar to what SPIError.spidata is now. SPIError
> would keep its spidata for compatibility but would get the same
> information into its detail, hint etc. attributes since it's derived
> from BaseError.
>
> So an equivalent to this Python (pseudo)code:
>
> # base class for all exceptions raised by PL/Python
> class BaseError:
> def __init__(self, msg, detail=None, hint=None, and so on for
> every accepted argument):
> self.msg = msg
> self.detail = detail
> # and so on
>
> class Error(BaseError):
> pass
>
> class Fatal(BaseError):
> pass
>
> class SPIError(BaseError):
> def __init__(self, msg):
> # call BaseError.__init__ so that SPIError also gets .msg,
> .detail and so on like all other BaseError instances
> # do what's done today to fill spidata to keep backward
> compatibility
>
> If you don't like that spidata duplicates the other fields, it's
> probably also ok to not make inherit from BaseError at all. I actually
> like this better. Then I would call the base class something like
> UserRaisedError (ugly name but can't think of something better to
> emphasize that it's about errors that the PL/Python developer is
> supposed to raise rather SPIError which is Postgres raising them) and
> it would be:
>
> # base class for exceptions raised by users in their PL/Python code
> class UserRaisedError:
> def __init__(self, msg, detail=None, hint=None, and so on for
> every accepted argument):
> self.msg = msg
> self.detail = detail
> # and so on
>
> class Error(UserRaisedError):
> pass
>
> class Fatal(UserRaisedError):
> pass
>
> # base class for exceptions raised by Postgres when executing sql code
> class SPIError:
> # no change to this class
>
>
I see it.

it looks like distinguish between Error and SPIError is wrong way. And I
have any other ugly use case.

If I raise a Error from one PLPythonu function, then in other PLPython
function I'll trap a SPIError exception, because the information about
class was lost inside Postgres. And it should be pretty messy.
I have not information if any exception was User based or SPI based.

The differentiation between Error and SPIError is wrong, because there
isn't any difference in reality. There are two ways.

1. break compatibility and SPIError replace by Error

2. don't introduce Error class

-- @1 is better - the SPIError isn't the best name, but breaking
compatibility is pretty unhappy - so only one solution is @2 :(

Comments, notes ??

Regards

Pavel

> >
> > 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.
> >
>
> I see, then never mind.
>