Re: Minor code de-duplication in fe-connect.c

On Mon, Apr 24, 2023 at 5:14 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:

> > On 21 Apr 2023, at 18:38, Gurjeet Singh <gurjeet(at)singh(dot)im> wrote:
> >
> > On Fri, Apr 21, 2023 at 7:47 AM Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
> >>
> >> On Fri, Apr 21, 2023 at 8:25 AM Daniel Gustafsson <daniel(at)yesql(dot)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.
>
>
Sorry about the errors. This seems to be the older version of the patch
that I had generated before fixing these mistakes. I do remember
encountering the compiler errors and revising the code to fix these,
especially the upper vs. lower case and the partial removal of the coment.
Away from my keyboard, so please expect the newer patch some time later.

Best regards,
Gurjeet
http://Gurje.et