After discussing this patch privately with Andres here's a new version of this
patch. The major differences are:
1. Use the pointer value of the connection as a randomness source
2. Use more precise time as randomness source
3. Move addrinfo changes into a separate commit. This is both to make
the actual change cleaner, and because another patch of mine (non
blocking cancels) benefits from the same change.
4. Use the same type of Fisher-Yates shuffle as is done in two other
places in the PG source code.
5. Move tests depending on hosts file to a separate file. This makes
it clear in the output that tests are skipped, because skip_all shows
a nice message.
6. Only enable hosts file load balancing when loadbalance is included
in PG_TEST_EXTRA, since this test listens on TCP socket and is thus
dangerous on a multi-user system.
On Wed, 18 Jan 2023 at 11:24, Jelte Fennema <postgres(at)jeltef(dot)nl> wrote:
>
> As far as I can tell this is ready for committer feedback now btw. I'd
> really like to get this into PG16.
>
> > It hadn't been my intention to block the patch on it, sorry. Just
> > registering a preference.
>
> No problem. I hadn't looked into the shared PRNG solution closely
> enough to determine if I thought it was better or not. Now that I
> implemented an initial version I personally don't think it brings
> enough advantages to warrant the added complexity. I definitely
> don't think it's required for this patch, if needed this change can
> always be done later without negative user impact afaict. And the
> connection local PRNG works well enough to bring advantages.
>
> > my
> > assumption was that you could use the existing pglock_thread() to
> > handle it, since it didn't seem like the additional contention would
> > hurt too much.
>
> That definitely would have been the easier approach and I considered
> it. But the purpose of pglock_thread seemed so different from this lock
> that it felt weird to combine the two. Another reason I refactored the lock
> code is that it would be probably be necessary for a future round-robin
> load balancing, which would require sharing state between different
> connections.
>
> > > And my thought was that the one-time
> > > initialization could be moved to a place that doesn't need to know the
> > > connection options at all, to make it easier to reason about the
> > > architecture. Say, next to the WSAStartup machinery.
>
> That's an interesting thought, but I don't think it would really simplify
> the initialization code. Mostly it would change its location.
>
> > (And after marinating on this over the weekend, it occurred to me that
> > keeping the per-connection option while making the PRNG global
> > introduces an additional hazard, because two concurrent connections
> > can now fight over the seed value.)
>
> I think since setting the initial seed value is really only meant for testing
> it's not a big deal if it doesn't work with concurrent connections.