> On 17 Mar 2023, at 09:50, Jelte Fennema <postg...@jeltef.nl> wrote: > >> The documentation lists the modes disabled and random, but I wonder if it's >> worth expanding the docs to mention that "disabled" is pretty much a round >> robin load balancing scheme? It reads a bit odd to present load balancing >> without a mention of round robin balancing given how common it is. > > I think you misunderstood what I meant in that section, so I rewrote > it to hopefully be clearer. Because disabled really isn't the same as > round-robin.
Thinking more about it I removed that section since it adds more confusion than
it resolves I think. It would be interesting to make it a true round-robin
with some form of locally stored pointer to last connection but thats for
future hacking.
>> -#ifndef WIN32
>> +/* MinGW has sys/time.h, but MSVC doesn't */
>> +#ifndef _MSC_VER
>> #include <sys/time.h>
>> This seems unrelated to the patch in question, and should be a separate
>> commit IMO.
>
> It's not really unrelated. This only started to be needed because
> libpq_prng_init calls gettimeofday . That did not work on MinGW
> systems. Before this patch libpq was never calling gettimeofday. So I
> think it makes sense to leave it in the commit.
Gotcha.
>> A test
>> which require root permission level manual system changes stand a very low
>> chance of ever being executed, and as such will equate to dead code that may
>> easily be broken or subtly broken.
>
> While I definitely agree that it makes it hard to execute, I don't
> think that means it will be executed nearly as few times as you
> suggest. Maybe you missed it, but I modified the .cirrus.yml file to
> configure the hosts file for both Linux and Windows runs. So, while I
> agree it is unlikely to be executed manually by many people, it would
> still be run on every commit fest entry (which should capture most
> issues that I can imagine could occur).
I did see it was used in the CI since the jobs there are containerized, what
I'm less happy about is that we wont be able to test this in the BF. That
being said, not having the test at all would mean even less testing so in the
end I agree that including it is the least bad option. Longer term I would
like to rework into something less do-this-manually test, but I have no good
ideas right now.
I've played around some more with this and came up with the attached v15 which
I think is close to the final state. The changes I've made are:
* Added the DNS test back into the main commit
* A few incorrect (referred to how the test worked previously) comments in
the tests fixed.
* The check against PG_TEST_EXTRA performed before any processing done
* Reworked the check for hosts content attempting to make it a bit more
robust
* 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.
* Made the prng init function void as it always returned true anyways.
* Minor comment and docs tweaking.
* I removed the change to geqo, while I don't think it's incorrect it also
hardly seems worth the churn.
* Commit messages are reworded.
I would like to see this wrapped up in the current CF, what do you think about
the attached?
--
Daniel Gustafsson
v15-0002-Support-connection-load-balancing-in-libpq.patch
Description: Binary data
v15-0001-Copy-and-store-addrinfo-in-libpq-owned-private-m.patch
Description: Binary data
