> On 27 Mar 2023, at 13:50, Jelte Fennema <postg...@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
