On Wed, Nov 25, 2015 at 11:08 PM, Magnus Hagander <[email protected]>
wrote:
> On Wed, Nov 25, 2015 at 3:03 PM, Michael Paquier <
> [email protected]> wrote:
>
>> On Wed, Nov 25, 2015 at 7:18 PM, Magnus Hagander <[email protected]>
>> wrote:
>>
> In particular, it seems that in InitWalSenderSlot, we only initialize the
>>> sent location. Perhaps this is needed?
>>>
>>
>> Yes, that would be nice to start with a clean state. At the same time, I
>> am noticing that pg_stat_get_wal_senders() is comparing flush, apply and
>> write directly with 0. I think those should be InvalidXLogRecPtr. Combined
>> with your patch it gives the attached.
>>
>
> Good point.
>
> Another thought - why do we leave 0/0 in sent location, but turn the other
> three fields to NULL if it's invalid? That seems inconsistent. Is there a
> reason for that, or should we fix that as well while we're at it?
>
In some code paths, values are expected to be equal to 0 as XLogRecPtr
values are doing direct comparisons with each other, so you can think that
0 for a XLogRecPtr is slightly different from InvalidXLogRecPtr that could
be expected to be invalid but *not* minimal, imagine for example that we
decide at some point to redefine it as INT64_MAX. See for example
receivedUpto in xlog.c or WalSndWaitForWal in walsender.c. Honestly, I
think that it would be nice to make all the logic consistent under a unique
InvalidXLogRecPtr banner that is defined at 0 once and for all, adding at
the same time a comment in xlogdefs.h mentioning that this should not be
redefined to a higher value because comparison logic of XlogRecPtr's depend
on that. We have actually a similar constraint with TimeLineID = 0 that is
equivalent to infinite. Patch attached following those lines, which should
be backpatched for correctness.
Btw, I found at the same time a portion of NOT_USED code setting up a
XLogRecPtr incorrectly in GetOldestWALSendPointer.
FWIW, the last time I poked at this code, introducing some kind of
MinimalXLogRecPtr logic different to distinguish from InvalidXLogRecPtr I
hit a wall, others did not like that much:
http://www.postgresql.org/message-id/cab7npqruafytj024w-hiihw4pwfcadzn9hfmsjqfozsqtzt...@mail.gmail.com
Regards,
--
Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f17f834..3988ffd 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -191,7 +191,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;
/*
@@ -4666,7 +4666,8 @@ XLOGShmemInit(void)
*/
allocptr = ((char *) XLogCtl) + sizeof(XLogCtlData);
XLogCtl->xlblocks = (XLogRecPtr *) allocptr;
- memset(XLogCtl->xlblocks, 0, sizeof(XLogRecPtr) * XLOGbuffers);
+ memset(XLogCtl->xlblocks, InvalidXLogRecPtr,
+ sizeof(XLogRecPtr) * XLOGbuffers);
allocptr += sizeof(XLogRecPtr) * XLOGbuffers;
@@ -11189,7 +11190,7 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
curFileTLI = tli;
RequestXLogStreaming(tli, ptr, PrimaryConnInfo,
PrimarySlotName);
- receivedUpto = 0;
+ receivedUpto = InvalidXLogRecPtr;
}
/*
@@ -11463,7 +11464,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 183a3a5..ccea4ca 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -1027,8 +1027,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 4a4643e..14b83b7 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;
@@ -1962,6 +1962,9 @@ InitWalSenderSlot(void)
*/
walsnd->pid = MyProcPid;
walsnd->sentPtr = InvalidXLogRecPtr;
+ walsnd->write = InvalidXLogRecPtr;
+ walsnd->flush = InvalidXLogRecPtr;
+ walsnd->apply = InvalidXLogRecPtr;
walsnd->state = WALSNDSTATE_STARTUP;
walsnd->latch = &MyProc->procLatch;
SpinLockRelease(&walsnd->mutex);
@@ -2819,17 +2822,20 @@ pg_stat_get_wal_senders(PG_FUNCTION_ARGS)
else
{
values[1] = CStringGetTextDatum(WalSndGetStateString(state));
+
+ if (XLogRecPtrIsInvalid(sentPtr))
+ nulls[2] = true;
values[2] = LSNGetDatum(sentPtr);
- if (write == 0)
+ if (XLogRecPtrIsInvalid(write))
nulls[3] = true;
values[3] = LSNGetDatum(write);
- if (flush == 0)
+ if (XLogRecPtrIsInvalid(flush))
nulls[4] = true;
values[4] = LSNGetDatum(flush);
- if (apply == 0)
+ if (XLogRecPtrIsInvalid(apply))
nulls[5] = true;
values[5] = LSNGetDatum(apply);
@@ -2933,7 +2939,7 @@ WalSndKeepaliveIfNecessary(TimestampTz now)
XLogRecPtr
GetOldestWALSendPointer(void)
{
- XLogRecPtr oldest = {0, 0};
+ XLogRecPtr oldest = InvalidXLogRecPtr;
int i;
bool found = false;
diff --git a/src/include/access/xlogdefs.h b/src/include/access/xlogdefs.h
index 18a3e7c..39a660c 100644
--- a/src/include/access/xlogdefs.h
+++ b/src/include/access/xlogdefs.h
@@ -23,7 +23,9 @@ typedef uint64 XLogRecPtr;
/*
* Zero is used indicate an invalid pointer. Bootstrap skips the first possible
* WAL segment, initializing the first WAL page at XLOG_SEG_SIZE, so no XLOG
- * record can begin at zero.
+ * record can begin at zero. As direct comparisons of XLogRecPtr values depend
+ * on this default value being minimal, this should not be redefined to a
+ * higher value.
*/
#define InvalidXLogRecPtr 0
#define XLogRecPtrIsInvalid(r) ((r) == InvalidXLogRecPtr)
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
