Re: Patch: Implement failover on libpq connect level.

On Mon, 5 Sep 2016 14:03:11 +0300
Aleksander Alekseev <a(dot)alekseev(at)postgrespro(dot)ru> wrote:

> Hello, Victor.
>
>
> 1) It looks like code is not properly formatted.
>

Thanks for pointing to the documentation and formatting problems. I'll
fix them

> > static int
> > connectDBStart(PGconn *conn)
> > {
> > + struct nodeinfo
> > + {
> > + char *host;
> > + char *port;
> > + };
>
> Not sure if all compilers we support can parse this. I suggest to
> declare nodinfo structure outside of procedure, just to be safe.
>

Block-local structs are part of C language standard. I don't remember
when they appeared first (may be in K&R C), but at least since C89 AFAIR
they are allowed explicitely.

And using most local scope possible is always a good thing.

So, I'll leave this structure function local for now.

>
> You should check return values of malloc, calloc and strdup
> procedures, or use safe pg_* equivalents.

Thanks, I'll fix this one.

> 6)
>
> > + for (p = addrs; p->ai_next != NULL; p =
> > p->ai_next);
> > + p->ai_next = this_node_addrs;
>
> Looks a bit scary. I suggest to add an empty scope ( {} ) and a
> comment to make our intention clear here.

Ok, it really would make code more clear.

> 7) Code compiles with following warnings (CLang 3.8):
>
> > 1 warning generated.
> > fe-connect.c:5239:39: warning: if statement has empty body
> > [-Wempty-body]
> > errorMessage,
> > false, true));
> > ^
> > fe-connect.c:5239:39: note: put the semicolon on a separate line to
> > silence this warning
> > 1 warning generated.

Oops, it looks like logic error, which should be fixed. Thanks for
testing my patch by more picky compiler.

> 8) get_next_element procedure implementation is way too smart (read
> "complicated"). You could probably just store current list length and
> generate a random number between 0 and length-1.

No, algorithm here is more complicated. It must ensure that there would
not be second attempt to connect to host, for which unsuccessful
connection attempt was done. So, there is list rearrangement.

Algorithm for pick random list element by single pass is quite trivial.