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

At Mon, 27 Jan 2020 16:55:56 +0900, Michael Paquier <michael@paquier.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