Re: [EXTERNAL] Support load balancing in libpq

From: Jelte Fennema <postgres(at)jeltef(dot)nl>
To: Daniel Gustafsson <daniel(at)yesql(dot)se>
Cc: "Gregory Stark (as CFM)" <stark(dot)cfm(at)gmail(dot)com>, Andrey Borodin <amborodin86(at)gmail(dot)com>, Jacob Champion <jchampion(at)timescale(dot)com>, Maxim Orlov <orlovmg(at)gmail(dot)com>, Jelte Fennema <Jelte(dot)Fennema(at)microsoft(dot)com>, Michael Banck <mbanck(at)gmx(dot)net>, Aleksander Alekseev <aleksander(at)timescale(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de>
Subject: Re: [EXTERNAL] Support load balancing in libpq
Date: 2023-03-27 11:50:39
Message-ID: CAGECzQSPkkvgihXJxwA2Xbfd-hYGOzaUmCo=KeCc2vPZzaeAkQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

>+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

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

Attachment Content-Type Size
v16-0001-Copy-and-store-addrinfo-in-libpq-owned-private-m.patch application/octet-stream 11.7 KB
v16-0002-Support-connection-load-balancing-in-libpq.patch application/octet-stream 24.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Daniel Gustafsson 2023-03-27 11:57:46 Re: [EXTERNAL] Support load balancing in libpq
Previous Message Alexander Korotkov 2023-03-27 10:49:22 Re: POC: Lock updated tuples in tuple_update() and tuple_delete()