2016-01-08 7:31 GMT+01:00 Catalin Iacob <iacobcata...@gmail.com>: > shellOn Thu, Dec 31, 2015 at 11:37 AM, Pavel Stehule > <pavel.steh...@gmail.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. >