On Fri, Jul 18, 2014 at 3:12 PM, Воронин Дмитрий <carriingfat...@yandex.ru> wrote: > I make a new version of patch. I corrected your notes for my previous > version of patch. Could you test it? Thank you.
I just had a look at the new version of this patch, and this is lacking a couple of things. First, this patch has been developed using a tarball of 9.3.4. sslinfo is not a module changing frequently so you are lucky that this is not generating diffs with HEAD but you should try to get yourself familiar with git and submit patches that are based on HEAD to facilitate their review and integration. You can have a look here to begin with: https://wiki.postgresql.org/wiki/Working_with_GIT Then, here are more comments about the patch: - Update sslinfo to 1.1. Here are all the details about how to do it: http://www.postgresql.org/docs/devel/static/extend-extensions.html#AEN57147 Well, basically this is only creating sslinfo--1.0--1.1.sql to be able to do an upgrade, copy sslinfo--1.0.sql to sslinfo--1.1.sql with the new objects defined and dumping sslinfo.control. - In your previous patches I saw that you defined the new functions in sslinfo--1.0.sql but in the new version of your patch it is not the case. - Please remove all the diff noise in sslinfo.sgml, like this one: *** 157,167 **** <varlistentry> <term> <function>ssl_client_dn_field(fieldname text) returns text</function> - <indexterm> - <primary>ssl_client_dn_field</primary> - </indexterm> </term> <listitem> <para> --- 157,167 ---- </varlistentry> <varlistentry> + <indexterm> + <primary>ssl_client_dn_field</primary> + </indexterm> Your patch should only include documentation for the new functions. - Please remove whitespaces, there are quite a lot of them. - in 9.5, PG_FUNCTION_INFO_V1 does the function declaration for you, making following block useless: [...] +Datum ssl_is_used(PG_FUNCTION_ARGS); +Datum ssl_version(PG_FUNCTION_ARGS); +Datum ssl_cipher(PG_FUNCTION_ARGS); [...] - Please avoid if blocks of this type, this is not consistent with the project style: if (SRF_IS_FIRSTCALL()) { [...] } One newline for each bracket. Here is the manual page referencing code conventions: http://www.postgresql.org/docs/current/static/source.html Most of those comments have been already mentioned by Andreas in one of his emails upthread but you have obviously not solved the issues he has reported. This looks like a useful feature, but at this point of the commit fest and considering the number of structural changes still needed I will mark this patch as "Returned with feedback". Feel free to resubmit to the next commit fest in August with an updated patch of course! Regards, -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
