Re: [PATCH] /src/backend/access/transam/xlog.c, tiny improvements

At Mon, 27 Jan 2020 16:55:56 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in
> On Sun, Jan 26, 2020 at 06:47:57PM -0800, Mark Dilger wrote:
> > There is something unusual about comparing a XLogSegNo variable in
> > this way, but it seems to go back to 2014 when the replication slots
> > were introduced in commit 858ec11858a914d4c380971985709b6d6b7dd6fc,
> > and XLogSegNo was unsigned then, too. Depending on how you look at
> > it, this could be a thinko, or it could be defensive programming
> > against future changes to the XLogSegNo typedef. I’m betting it was
> > defensive programming, given the context. As such, I don’t think it
> > would be appropriate to remove this defense in your patch.
>
> Yeah. To e honest, I am not actually sure if it is worth bothering
> about any of those three places.

+1.

FWIW, I have reasons for being aganst the first the the last items.

For the first item, The duplicate if blocks seem working as enclosure
of a meaningful set of code. It's annoying that OwnLatch follows a
bunch of "else if() ereport" lines in a block.

For the last item, using '==' in the context of size comparison make
me a bit uneasy. I prefer '< 1' there but I don't bother doing
that. They are logially the same.

For the second item, I don't object to do that but also I'm not
willing to support it.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center