Re: [EXTERNAL] Support load balancing in libpq

> On 27 Mar 2023, at 13:50, Jelte Fennema <postgres@jeltef.nl> wrote:
> 
> Looks good overall. I attached a new version with a few small changes:
> 
>>  * Changed store_conn_addrinfo to return int like how all the functions
>>    dealing with addrinfo does.  Also moved the error reporting to inside there
>>    where the error happened.
> 
> I don't feel strong about the int vs bool return type. The existing
> static libpq functions are a bit of a mixed bag around this, so either
> way seems fine to me. And moving the log inside the function seems
> fine too. But it seems you accidentally removed the "goto
> error_return" part as well, so now we're completely ignoring the
> allocation failure. The attached patch fixes that.

Ugh, thanks.  I had a conflict here when rebasing with the load balancing
commit in place and clearly fat-fingered that one.

>> +ok($node1_occurences > 1, "expected at least one execution on node1, found none");
>> +ok($node2_occurences > 1, "expected at least one execution on node2, found none");
>> +ok($node3_occurences > 1, "expected at least one execution on node3, found none");
> 
> I changed the message to be a description of the expected case,
> instead of the failure case. This is in line with the way these
> messages are used in other tests, and indeed seems like the correct
> way because you get output from "meson test -v postgresql:libpq /
> libpq/003_load_balance_host_list" like this:
> ▶ 6/6 - received at least one connection on node1       OK
> ▶ 6/6 - received at least one connection on node2       OK
> ▶ 6/6 - received at least one connection on node3       OK
> ▶ 6/6 - received 50 connections across all nodes        OK

Good point.

> Finally, I changed a few small typos in your updated commit message
> (some of which originated from my earlier commit messages)

+1

--
Daniel Gustafsson