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. 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. -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers