[PATCH] /src/backend/access/transam/xlog.c, tiny improvements
Hi,
There are 3 tiny improvements to xlog.c code:
1. At function StartupXLOG (line 6370), the second test if
(ArchiveRecoveryRequested) is redundant and can secure removed.
2. At function StartupXLOG (line 7254) the var switchedTLI already been
tested before and the second test can secure removed.
3. At function KeepLogSeg (line 9357) the test if (slotSegNo <= 0), the
var slotSegNo is uint64 and not can be < 0.
As it is a critical file, even though small, these improvements, I believe
are welcome, because they improve readability.
regards,
Ranier Vilela
Attachments:
xlog.patchapplication/octet-stream; name=xlog.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 7f4f784c0e..0ae699cfd3 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6361,14 +6361,13 @@ StartupXLOG(void)
else
ereport(LOG,
(errmsg("starting archive recovery")));
- }
-
- /*
- * Take ownership of the wakeup latch if we're going to sleep during
- * recovery.
- */
- if (ArchiveRecoveryRequested)
+
+ /*
+ * Take ownership of the wakeup latch if we're going to sleep during
+ * recovery.
+ */
OwnLatch(&XLogCtl->recoveryWakeupLatch);
+ }
/* Set up XLOG reader facility */
MemSet(&private, 0, sizeof(XLogPageReadPrivate));
@@ -7251,7 +7250,7 @@ StartupXLOG(void)
* Wake up any walsenders to notice that we are on a new
* timeline.
*/
- if (switchedTLI && AllowCascadeReplication())
+ if (AllowCascadeReplication())
WalSndWakeup();
}
@@ -9354,7 +9353,7 @@ KeepLogSeg(XLogRecPtr recptr, XLogSegNo *logSegNo)
XLByteToSeg(keep, slotSegNo, wal_segment_size);
- if (slotSegNo <= 0)
+ if (slotSegNo == 0)
segno = 1;
else if (slotSegNo < segno)
segno = slotSegNo;
On Jan 24, 2020, at 12:48 PM, Ranier Vilela <ranier.vf@gmail.com> wrote:
3. At function KeepLogSeg (line 9357) the test if (slotSegNo <= 0), the var slotSegNo is uint64 and not can be < 0.
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.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
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.
--
Michael
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
Em dom., 26 de jan. de 2020 às 23:48, Mark Dilger <
mark.dilger@enterprisedb.com> escreveu:
On Jan 24, 2020, at 12:48 PM, Ranier Vilela <ranier.vf@gmail.com> wrote:
3. At function KeepLogSeg (line 9357) the test if (slotSegNo <= 0), the
var slotSegNo is uint64 and not can be < 0.
There is something unusual about comparing 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.
while in general terms I am in favor of defensive programming, it is not
needed everywhere.
I am surprised that the final code contains a lot of code that is not
actually executed.
It should be an interesting exercise to debug postgres, which I haven't
done yet.
See variables being declared, functions called, memories being touched,
none of which occurs in production.
In the case of XLogSegNo, it is almost impossible to change it to signed,
as this would limit the number of segments that could be used.
Even so, the test could be removed and put into comment, the warning that
in the future, in case of change to signed, it should be tested less than
zero.
regards,
Ranier Vilela