On 2013-01-17 16:23:44 +0100, Andres Freund wrote:
> On 2013-01-17 17:18:14 +0200, Heikki Linnakangas wrote:
> > On 17.01.2013 15:05, Andres Freund wrote:
> > >On 2013-01-17 13:47:41 +0900, Michael Paquier wrote:
> > >>I think that bug has been introduced by commit 7fcbf6a.
> > >>Before splitting xlog reading as a separate facility things worked
> > >>correctly.
> > >>There are also no delay problems before this commit.
> > >
> > >Ok, my inkling proved to be correct, its two related issues:
> > >
> > >a ) The error handling in XLogReadRecord is inconsistent, it doesn't
> > >always reset the necessary things.
> > >
> > >b) ReadRecord: We cannot not break out of the retry loop in readRecord
> > >just so, just removing the break seems correct.
> > >
> > >c) ReadRecord: (minor): We should log an error even if errormsg is not
> > >set, otherwise we wont jump out far enough.
> > >
> > >I think at least a) and b) is the result of merges between development
> > >of different people, sorry for that.
> >
> > Got a patch?
>
> Yes, I am just testing some scenarios with it, will send it after that.
Ok, the attached patch seems to fix a) and b). c) above is bogus, as
explained in a comment in the patch. I also noticed that the TLI check
didn't mark the last source as failed.
Not a real issue and its independent from this patch but I found that
when promoting from streaming rep the code first fails over to archive
recovery and only then to recovering from pg_xlog. Is that intended?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 70cfabc..e33bd7a 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3209,12 +3209,6 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
EndRecPtr = xlogreader->EndRecPtr;
if (record == NULL)
{
- /* not all failures fill errormsg; report those that do */
- if (errormsg && errormsg[0] != '\0')
- ereport(emode_for_corrupt_record(emode,
- RecPtr ? RecPtr : EndRecPtr),
- (errmsg_internal("%s", errormsg) /* already translated */));
-
lastSourceFailed = true;
if (readFile >= 0)
@@ -3222,7 +3216,23 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
close(readFile);
readFile = -1;
}
- break;
+
+ /*
+ * We only end up here without a message when XLogPageRead() failed
+ * - in that case we already logged something.
+ * In StandbyMode that only happens if we have been triggered, so
+ * we shouldn't loop anymore in that case.
+ */
+ if (errormsg == NULL)
+ break;
+
+
+ ereport(emode_for_corrupt_record(emode,
+ RecPtr ? RecPtr : EndRecPtr),
+ (errmsg_internal("%s", errormsg) /* already translated */));
+
+ /* don't make a timeline check */
+ continue;
}
/*
@@ -3234,6 +3244,8 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
XLogSegNo segno;
int32 offset;
+ lastSourceFailed = true;
+
XLByteToSeg(xlogreader->latestPagePtr, segno);
offset = xlogreader->latestPagePtr % XLogSegSize;
XLogFileName(fname, xlogreader->readPageTLI, segno);
@@ -3243,7 +3255,7 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
xlogreader->latestPageTLI,
fname,
offset)));
- return false;
+ continue;
}
} while (StandbyMode && record == NULL);
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index ff871a3..75164f6 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -222,11 +222,7 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
readOff = ReadPageInternal(state, targetPagePtr, SizeOfXLogRecord);
if (readOff < 0)
- {
- if (state->errormsg_buf[0] != '\0')
- *errormsg = state->errormsg_buf;
- return NULL;
- }
+ goto err;
/*
* ReadPageInternal always returns at least the page header, so we can
@@ -246,8 +242,7 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
{
report_invalid_record(state, "invalid record offset at %X/%X",
(uint32) (RecPtr >> 32), (uint32) RecPtr);
- *errormsg = state->errormsg_buf;
- return NULL;
+ goto err;
}
if ((((XLogPageHeader) state->readBuf)->xlp_info & XLP_FIRST_IS_CONTRECORD) &&
@@ -255,8 +250,7 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
{
report_invalid_record(state, "contrecord is requested by %X/%X",
(uint32) (RecPtr >> 32), (uint32) RecPtr);
- *errormsg = state->errormsg_buf;
- return NULL;
+ goto err;
}
/* ReadPageInternal has verified the page header */
@@ -270,11 +264,7 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
targetPagePtr,
Min(targetRecOff + SizeOfXLogRecord, XLOG_BLCKSZ));
if (readOff < 0)
- {
- if (state->errormsg_buf[0] != '\0')
- *errormsg = state->errormsg_buf;
- return NULL;
- }
+ goto err;
/*
* Read the record length.
@@ -300,11 +290,7 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
{
if (!ValidXLogRecordHeader(state, RecPtr, state->ReadRecPtr, record,
randAccess))
- {
- if (state->errormsg_buf[0] != '\0')
- *errormsg = state->errormsg_buf;
- return NULL;
- }
+ goto err;
gotheader = true;
}
else
@@ -314,8 +300,7 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
{
report_invalid_record(state, "invalid record length at %X/%X",
(uint32) (RecPtr >> 32), (uint32) RecPtr);
- *errormsg = state->errormsg_buf;
- return NULL;
+ goto err;
}
gotheader = false;
}
@@ -330,8 +315,7 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
report_invalid_record(state, "record length %u at %X/%X too long",
total_len,
(uint32) (RecPtr >> 32), (uint32) RecPtr);
- *errormsg = state->errormsg_buf;
- return NULL;
+ goto err;
}
len = XLOG_BLCKSZ - RecPtr % XLOG_BLCKSZ;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
