On Fri, Feb 19, 2016 at 9:41 PM, Pavel Stehule <pavel.steh...@gmail.com> wrote: > It looks like good idea. Last version are not breaking compatibility - and > I think so it can works. > > I wrote the code, that works on Python2 and Python3
Hi, I've attached a patch on top of yours with some documentation improvements, feel free to fold it in your next version. I think the concept is good. We're down to code level changes. Most of them are cosmetical, misleading comments and so on but there are some bugs in there as far as I see. In object_to_string you don't need to test for Py_None. PyObject_Str will return NULL on failure and whatever str() returns on the underlying object. No need to special case None. In object_to_string, when you're already in a (so != NULL) block, you can use Py_DECREF instead of XDECREF. object_to_string seems buggy to me: it returns the pointer returned by PyString_AsString which points to the internal buffer of the Python object but it also XDECREFs that object. You seem to be returning a pointer to freed space. get_string_attr seems to have the same issue as object_to_string. In PLy_get_error_data query and position will never be set for plpy.Error since you don't offer them to Python and therefore don't set them in PLy_output_kw. So I think you should remove the code related to them, including the char **query, int *position parameters for PLy_get_error_data. Removing position also allows removing get_int_attr and the need to exercise this function in the tests. You're using PLy_get_spi_sqlerrcode in PLy_get_error_data which makes the spi in the name unsuitable. I would rename it to just PLy_get_sqlerrcode. At least the comment at the top of PLy_get_spi_sqlerrcode needs to change since it now also extracts info from Error not just SPIError. /* set value of string pointer on object field */ comments are weird for a function that gets a value. But those functions need to change anyway since they're buggy (see above). The only change in plpy_plpymodule.h is the removal of an empty line unrelated to this patch, probably from previous versions. You should undo it to leave plpy_plpymodule.h untouched. Why rename PLy_output to PLy_output_kw? It only makes the patch bigger without being an improvement. Maybe you also have this from previous versions. In PLy_output_kw you don't need to check message for NULL, just as sv wasn't checked before. In PLy_output_kw you added a FreeErrorData(edata) which didn't exist before. I'm not familiar with that API but I'm wondering if it's needed or not, good to have it or not etc. In PLy_output_kw you didn't update the "Note: If sv came from PyString_AsString(), it points into storage..." comment which still refers to sv which is now deleted. In PLy_output_kw you removed the "return a legal object so the interpreter will continue on its merry way" comment which might not be the most valuable comment in the world but removing it is still unrelated to this patch. In PLy_output_kw you test for kw != NULL && kw != Py_None. The kw != NULL is definitely needed but I'm quite sure Python will never pass Py_None into it so the != Py_None isn't needed. Can't find a reference to prove this at the moment. Some more tests could be added to exercise more parts of the patch: - check that SPIError is enhanced with all the new things: schema_name, table_name etc. - in the plpy.error call use every keyword argument not just detail and hint: it proves you save and restore every field correctly from the Error fields since that's not exercised by the info call above which does use every argument - use a wrong keyword argument to see it gets rejected with you error message - try some other types than str (like detail=42) for error as well since error goes on another code path than info - a test exercising the "invalid sqlstate" code
0001-Doc-improvements.patch
Description: binary/octet-stream
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers