Re: proposal: PL/Pythonu - function ereport - Mailing list pgsql-hackers

On Fri, Jan 8, 2016 at 7:56 AM, Pavel Stehule <pavel.stehule@gmail.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

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