Re: Patch: Implement failover on libpq connect level.

On Thu, Oct 13, 2016 at 5:53 AM, Victor Wagner <vitus(at)wagner(dot)pp(dot)ru> wrote:
> On Thu, 13 Oct 2016 12:30:59 +0530
> Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com> wrote:
>> On Fri, Sep 30, 2016 at 2:14 PM, Victor Wagner <vitus(at)wagner(dot)pp(dot)ru>
>> wrote:
>> Okay but for me consistency is also important. Since we agree to
>> disagree on some of the comments and others have not expressed any
>> problem I am moving it to committer.
>
> Thank you for your efforts improving my patch

I haven't deeply reviewed this patch, but on a quick read-through it
certainly seems to need a lot of cleanup work.

+ <varlistentry id="libpq-connect-hostorder" xreflabel="hostorder">
+ <term><literal>hostorder</literal></term>
+ <listitem>

I don't think that putting everything at the same indentation level
matches the style of the surrounding documentation.

@@ -985,7 +1042,6 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
</para>
</listitem>
</varlistentry>
-
<varlistentry id="libpq-connect-connect-timeout"
xreflabel="connect_timeout">
<term><literal>connect_timeout</literal></term>
<listitem>

Spurious hunk.

+ /* Host has explicitely specified port */

Spelling. This same error is repeated elsewhere.

+ /* Check if port is numeric */
+ for (nptr=r+1;nptr<q;nptr++)
+ {
+ if (*nptr<'0' ||*nptr >'9')

Formatting. Consider using pgindent to get this into correct PostgreSQL style.

+ * behavoir.

Spelling.

+ * the beginning (and putting its addres into given pointer.

Spelling.

+ * Returns 1 if address is choosen and 0 if there are no more addresses

Spelling.

+ * Initialize random number generator in case if nobody have done it
+ * before. Use value from rand along with time in case random number

Grammar ("in case nobody has done it already"). Also, is it really OK
for libpq to call srand() as a side effect? I think that seems rather
unfriendly.

+ * taget_server_type is set to 'any' or is not exlicitely

Multiple spelling mistakes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company