> 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
