Re: [HACKERS] Incorrect initialization of sentPtr in walsender.c
On Thu, Oct 16, 2014 at 9:01 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Thu, Oct 16, 2014 at 7:09 PM, Simon Riggs si...@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. Worth noticing that this patch has been marked as returned with feedback. -- Michael
Re: [HACKERS] Incorrect initialization of sentPtr in walsender.c
On 12 September 2014 13:16, Michael Paquier michael.paqu...@gmail.com wrote: On Fri, Sep 12, 2014 at 4:55 PM, Heikki Linnakangas hlinnakan...@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
Re: [HACKERS] Incorrect initialization of sentPtr in walsender.c
On Thu, Oct 16, 2014 at 7:09 PM, Simon Riggs si...@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
Re: [HACKERS] Incorrect initialization of sentPtr in walsender.c
On Fri, Sep 12, 2014 at 09:16:42PM +0900, Michael Paquier wrote: On Fri, Sep 12, 2014 at 4:55 PM, Heikki Linnakangas hlinnakan...@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 br...@momjian.ushttp://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
Re: [HACKERS] Incorrect initialization of sentPtr in walsender.c
On 09/12/2014 03:17 AM, Michael Paquier wrote: On Fri, Sep 12, 2014 at 9:08 AM, Michael Paquier michael.paqu...@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
Re: [HACKERS] Incorrect initialization of sentPtr in walsender.c
On Fri, Sep 12, 2014 at 4:55 PM, Heikki Linnakangas
hlinnakan...@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
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.
*/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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 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; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Incorrect initialization of sentPtr in walsender.c
On Fri, Sep 12, 2014 at 9:08 AM, Michael Paquier
michael.paqu...@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
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;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
