Incorrect initialization of sentPtr in walsender.c
Hi all,
In walsender.c, sentPtr is initialized as follows:
static XLogRecPtr sentPtr = 0;
Isn't that incorrect and shouldn't we use InvalidXLogRecPtr instead?
Patch is attached.
Regards,
--
Michael
Attachments:
20140912_walsender_sentptr.patchtext/x-diff; charset=US-ASCII; name=20140912_walsender_sentptr.patchDownload
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 844a5de..1568a6c 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -140,7 +140,7 @@ static XLogRecPtr sendTimeLineValidUpto = InvalidXLogRecPtr;
* How far have we sent WAL already? This is also advertised in
* MyWalSnd->sentPtr. (Actually, this is the next WAL location to send.)
*/
-static XLogRecPtr sentPtr = 0;
+static XLogRecPtr sentPtr = InvalidXLogRecPtr;
/* Buffers for constructing outgoing messages and processing reply messages. */
static StringInfoData output_message;
On Fri, Sep 12, 2014 at 9:08 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
In walsender.c, sentPtr is initialized as follows:
static XLogRecPtr sentPtr = 0;
Isn't that incorrect and shouldn't we use InvalidXLogRecPtr instead?
Actually by looking more around I found a couple of extra places where
the same inconsistencies are present, mainly in xlog.c and
walreceiver.c. Updated patch attached for all those things.
Regards,
--
Michael
Attachments:
20140912_wal_incorrect_ptr.patchtext/x-diff; charset=US-ASCII; name=20140912_wal_incorrect_ptr.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 34f2fc0..a6575f3 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -163,7 +163,7 @@ HotStandbyState standbyState = STANDBY_DISABLED;
static XLogRecPtr LastRec;
/* Local copy of WalRcv->receivedUpto */
-static XLogRecPtr receivedUpto = 0;
+static XLogRecPtr receivedUpto = InvalidXLogRecPtr;
static TimeLineID receiveTLI = 0;
/*
@@ -11003,7 +11003,7 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
curFileTLI = tli;
RequestXLogStreaming(tli, ptr, PrimaryConnInfo,
PrimarySlotName);
- receivedUpto = 0;
+ receivedUpto = InvalidXLogRecPtr;
}
/*
@@ -11266,7 +11266,7 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
static int
emode_for_corrupt_record(int emode, XLogRecPtr RecPtr)
{
- static XLogRecPtr lastComplaint = 0;
+ static XLogRecPtr lastComplaint = InvalidXLogRecPtr;
if (readSource == XLOG_FROM_PG_XLOG && emode == LOG)
{
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index c2d4ed3..c5103f7 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -1037,8 +1037,8 @@ XLogWalRcvFlush(bool dying)
static void
XLogWalRcvSendReply(bool force, bool requestReply)
{
- static XLogRecPtr writePtr = 0;
- static XLogRecPtr flushPtr = 0;
+ static XLogRecPtr writePtr = InvalidXLogRecPtr;
+ static XLogRecPtr flushPtr = InvalidXLogRecPtr;
XLogRecPtr applyPtr;
static TimestampTz sendTime = 0;
TimestampTz now;
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 844a5de..1568a6c 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -140,7 +140,7 @@ static XLogRecPtr sendTimeLineValidUpto = InvalidXLogRecPtr;
* How far have we sent WAL already? This is also advertised in
* MyWalSnd->sentPtr. (Actually, this is the next WAL location to send.)
*/
-static XLogRecPtr sentPtr = 0;
+static XLogRecPtr sentPtr = InvalidXLogRecPtr;
/* Buffers for constructing outgoing messages and processing reply messages. */
static StringInfoData output_message;
On 09/12/2014 03:17 AM, Michael Paquier wrote:
On Fri, Sep 12, 2014 at 9:08 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:In walsender.c, sentPtr is initialized as follows:
static XLogRecPtr sentPtr = 0;
Isn't that incorrect and shouldn't we use InvalidXLogRecPtr instead?Actually by looking more around I found a couple of extra places where
the same inconsistencies are present, mainly in xlog.c and
walreceiver.c. Updated patch attached for all those things.
InvalidXLogRecPtr == 0, so it's just a style issue which one is more
correct.
I haven't looked at those places closely, but it seems possible that at
least some of those variables are supposed to be initialized to a value
smaller than any valid WAL position, rather than just Invalid. In other
words, if we defined InvalidXLogRecPtr as INT64_MAX rather than 0, we
would still want those variables to be initialized to zero. As I said, I
didn't check the code, but before we change those that ought to be checked.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Sep 12, 2014 at 4:55 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:
I haven't looked at those places closely, but it seems possible that at
least some of those variables are supposed to be initialized to a value
smaller than any valid WAL position, rather than just Invalid. In other
words, if we defined InvalidXLogRecPtr as INT64_MAX rather than 0, we would
still want those variables to be initialized to zero. As I said, I didn't
check the code, but before we change those that ought to be checked.
Ah, OK. I just had a look at that, and receivedUpto and lastComplaint
in xlog.c need to use the lowest pointer value possible as they do a
couple of comparisons with other positions. This is as well the case
of sentPtr in walsender.c. However, that's not the case of writePtr
and flushPtr in walreceiver.c as those positions are just used for
direct comparison with LogstreamResult, so we could use
InvalidXLogRecPtr in this case.
What do you think of the addition of a #define for the lowest possible
XLOG location pointer? I've wanted that as well a couple of times when
working on clients using replication connections for for example
START_REPLICATION. That would be better than hardcoding a position
like '0/0', and would make the current code more solid.
Patch attached in case.
Regards,
--
Michael
Attachments:
20140912_wal_incorrect_ptr_v2.patchtext/x-diff; charset=US-ASCII; name=20140912_wal_incorrect_ptr_v2.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 34f2fc0..fc42b5f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -163,7 +163,7 @@ HotStandbyState standbyState = STANDBY_DISABLED;
static XLogRecPtr LastRec;
/* Local copy of WalRcv->receivedUpto */
-static XLogRecPtr receivedUpto = 0;
+static XLogRecPtr receivedUpto = LowestXLogRecPtr;
static TimeLineID receiveTLI = 0;
/*
@@ -11003,7 +11003,7 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
curFileTLI = tli;
RequestXLogStreaming(tli, ptr, PrimaryConnInfo,
PrimarySlotName);
- receivedUpto = 0;
+ receivedUpto = LowestXLogRecPtr;
}
/*
@@ -11266,7 +11266,7 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
static int
emode_for_corrupt_record(int emode, XLogRecPtr RecPtr)
{
- static XLogRecPtr lastComplaint = 0;
+ static XLogRecPtr lastComplaint = LowestXLogRecPtr;
if (readSource == XLOG_FROM_PG_XLOG && emode == LOG)
{
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index c2d4ed3..c5103f7 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -1037,8 +1037,8 @@ XLogWalRcvFlush(bool dying)
static void
XLogWalRcvSendReply(bool force, bool requestReply)
{
- static XLogRecPtr writePtr = 0;
- static XLogRecPtr flushPtr = 0;
+ static XLogRecPtr writePtr = InvalidXLogRecPtr;
+ static XLogRecPtr flushPtr = InvalidXLogRecPtr;
XLogRecPtr applyPtr;
static TimestampTz sendTime = 0;
TimestampTz now;
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 844a5de..819ac28 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -140,7 +140,7 @@ static XLogRecPtr sendTimeLineValidUpto = InvalidXLogRecPtr;
* How far have we sent WAL already? This is also advertised in
* MyWalSnd->sentPtr. (Actually, this is the next WAL location to send.)
*/
-static XLogRecPtr sentPtr = 0;
+static XLogRecPtr sentPtr = LowestXLogRecPtr;
/* Buffers for constructing outgoing messages and processing reply messages. */
static StringInfoData output_message;
diff --git a/src/include/access/xlogdefs.h b/src/include/access/xlogdefs.h
index 3b8e738..f7d88b4 100644
--- a/src/include/access/xlogdefs.h
+++ b/src/include/access/xlogdefs.h
@@ -25,9 +25,13 @@ typedef uint64 XLogRecPtr;
* WAL segment, initializing the first WAL page at XLOG_SEG_SIZE, so no XLOG
* record can begin at zero.
*/
-#define InvalidXLogRecPtr 0
+#define InvalidXLogRecPtr ((XLogRecPtr) 0)
#define XLogRecPtrIsInvalid(r) ((r) == InvalidXLogRecPtr)
+/* Minimum value possible for a location pointer of XLOG */
+#define LowestXLogRecPtr ((XLogRecPtr) 0)
+#define XLogRecPtrIsLowest(r) ((r) == LowestXLogRecPtr)
+
/*
* XLogSegNo - physical log file sequence number.
*/
On Fri, Sep 12, 2014 at 09:16:42PM +0900, Michael Paquier wrote:
On Fri, Sep 12, 2014 at 4:55 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:I haven't looked at those places closely, but it seems possible that at
least some of those variables are supposed to be initialized to a value
smaller than any valid WAL position, rather than just Invalid. In other
words, if we defined InvalidXLogRecPtr as INT64_MAX rather than 0, we would
still want those variables to be initialized to zero. As I said, I didn't
check the code, but before we change those that ought to be checked.Ah, OK. I just had a look at that, and receivedUpto and lastComplaint
in xlog.c need to use the lowest pointer value possible as they do a
couple of comparisons with other positions. This is as well the case
of sentPtr in walsender.c. However, that's not the case of writePtr
and flushPtr in walreceiver.c as those positions are just used for
direct comparison with LogstreamResult, so we could use
InvalidXLogRecPtr in this case.What do you think of the addition of a #define for the lowest possible
XLOG location pointer? I've wanted that as well a couple of times when
working on clients using replication connections for for example
START_REPLICATION. That would be better than hardcoding a position
like '0/0', and would make the current code more solid.Patch attached in case.
I like this. Can we apply it Heikki?
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Oct 14, 2014 at 5:18 AM, Bruce Momjian <bruce@momjian.us> wrote:
On Fri, Sep 12, 2014 at 09:16:42PM +0900, Michael Paquier wrote:
Patch attached in case.
I like this. Can we apply it Heikki?
I actually registered it to the next CF so as it does not fall into
oblivion, simply forgot to mention it:
https://commitfest.postgresql.org/action/patch_view?id=1572
Note that this patch still sets InvalidXLogRecPtr to 0. I think that
it should be INT64_MAX for clarity.
Regards
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 12 September 2014 13:16, Michael Paquier <michael.paquier@gmail.com> wrote:
On Fri, Sep 12, 2014 at 4:55 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:I haven't looked at those places closely, but it seems possible that at
least some of those variables are supposed to be initialized to a value
smaller than any valid WAL position, rather than just Invalid. In other
words, if we defined InvalidXLogRecPtr as INT64_MAX rather than 0, we would
still want those variables to be initialized to zero. As I said, I didn't
check the code, but before we change those that ought to be checked.Ah, OK. I just had a look at that, and receivedUpto and lastComplaint
in xlog.c need to use the lowest pointer value possible as they do a
couple of comparisons with other positions. This is as well the case
of sentPtr in walsender.c. However, that's not the case of writePtr
and flushPtr in walreceiver.c as those positions are just used for
direct comparison with LogstreamResult, so we could use
InvalidXLogRecPtr in this case.
I don't see this patch gives us anything. All it will do is prevent
easy backpatching of related fixes.
-1 for changing the code in this kind of way
I find it confusing that the "Lowest" pointer value is also "Invalid".
Valid != Invalid
-1 for this patch
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Oct 16, 2014 at 7:09 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
I find it confusing that the "Lowest" pointer value is also "Invalid".
Valid != Invalid
In complement to that, note that I mentioned Invalid should be UINT_MAX for
clarity.
--
Michael
On Thu, Oct 16, 2014 at 9:01 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:
On Thu, Oct 16, 2014 at 7:09 PM, Simon Riggs <simon@2ndquadrant.com>
wrote:I find it confusing that the "Lowest" pointer value is also "Invalid".
Valid != InvalidIn complement to that, note that I mentioned Invalid should be UINT_MAX
for clarity.
Worth noticing that this patch has been marked as returned with feedback.
--
Michael