Re: Minor code de-duplication in fe-connect.c - Mailing list pgsql-hackers

> On 21 Apr 2023, at 18:38, Gurjeet Singh <gurjeet@singh.im> wrote:
>
> On Fri, Apr 21, 2023 at 7:47 AM Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> On Fri, Apr 21, 2023 at 8:25 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>>> The reason I left it like this when reviewing and committing is that I think it
>>> makes for more readable code.  The amount of lines saved is pretty small, and
>>> "shuffle" isn't an exact term so by reading the code it isn't immediate clear
>>> what such a function would do.  By having the shuffle algorithm where it's used
>>> it's clear what the code does and what the outcome is.  If others disagree I
>>> can go ahead and refactor of course, but I personally would not deem it a net
>>> win in code quality.
>>
>> I think we should avoid nitpicking stuff like this. I likely would
>> have used a subroutine if I'd done it myself, but I definitely
>> wouldn't have submitted a patch to change whatever the last person did
>> without some tangible reason for so doing.
>
> Code duplication, and the possibility that a change in one location
> (bugfix or otherwise) does not get applied to the other locations, is
> a good enough reason to submit a patch, IMHO.
>
>> It's not a good use of
>> reviewer and committer time to litigate things like this.
>
> Postgres has a very high bar for code quality, and for documentation.
> It is a major reason that attracts people to, and keeps them in the
> Postgres ecosystem, as users and contributors. If anything, we should
> encourage folks to point out such inconsistencies in code and docs
> that keep the quality high.
>
> This is not a attack on any one commit, or author/committer of the
> commit; sorry if it appeared that way. I was merely reviewing the
> commit that introduced a nice libpq feature. This patch is merely a
> minor improvement to the code that I think deserves a consideration.
> It's not a litigation, by any means.

I didn't actually read the patch earlier, but since Robert gave a +1 to the
idea of refactoring I had a look. I have a few comments:

+static void
+shuffleAddresses(PGConn *conn)
This fails to compile since the struct is typedefed PGconn.


-    /*
-     * This is the "inside-out" variant of the Fisher-Yates shuffle
-     * algorithm. Notionally, we append each new value to the array
-     * and then swap it with a randomly-chosen array element (possibly
-     * including itself, else we fail to generate permutations with
-     * the last integer last).  The swap step can be optimized by
-     * combining it with the insertion.
-     *
      * We don't need to initialize conn->prng_state here, because that
      * already happened in connectOptions2.
      */
This also fails to compile since it removes the starting /* marker of the
comment resulting in a comment starting on *.


-    for (i = 1; i < conn->nconnhost; i++)
-    for (int i = 1; i < conn->naddr; i++)
You are replacing these loops for shuffling an array with a function that does
this:
+    for (int i = 1; i < conn->naddr; i++)
This is not the same thing, one is shuffling the hosts and the other is
shuffling the addresses resolved for the hosts (which may be a n:m
relationship).  If you had compiled and run the tests you would have seen that
the libpq tests now fail with this applied, as would be expected.


Shuffling the arrays can of course be made into a subroutine, but what to
shuffle and where needs to be passed in to it and at that point I doubt the
code readability is much improved over the current coding.

--
Daniel Gustafsson