Applying XLR_INFO_MASK correctly when looking at WAL record information
Hi all,
I have just been trapped by the fact that in some code paths we look
at the RMGR information of a WAL record (record->xl_info) but the
filter dedicated to it, ~XLR_INFO_MASK, is not applied. This is
harmful now, but this could lead to problems if in the future new
record-level flags, similar to XLR_SPECIAL_REL_UPDATE, are added.
Disclaimer: I am looking at a patch where a record-level flag makes
sense, still this looks like a separate problem.
What about the patch attached to make things more consistent?
Thanks,
--
Michael
Attachments:
wal-xlinfo.patchtext/x-diff; charset=US-ASCII; name=wal-xlinfo.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c9bb46b..1087d3b 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -903,8 +903,9 @@ XLogInsertRecord(XLogRecData *rdata, XLogRecPtr fpw_lsn)
pg_crc32c rdata_crc;
bool inserted;
XLogRecord *rechdr = (XLogRecord *) rdata->data;
+ uint8 info = rechdr->xl_info & ~XLR_INFO_MASK;
bool isLogSwitch = (rechdr->xl_rmid == RM_XLOG_ID &&
- rechdr->xl_info == XLOG_SWITCH);
+ info == XLOG_SWITCH);
XLogRecPtr StartPos;
XLogRecPtr EndPos;
@@ -7785,6 +7786,7 @@ ReadCheckpointRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr,
int whichChkpt, bool report)
{
XLogRecord *record;
+ uint8 info;
if (!XRecOffIsValid(RecPtr))
{
@@ -7810,6 +7812,7 @@ ReadCheckpointRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr,
}
record = ReadRecord(xlogreader, RecPtr, LOG, true);
+ info = record->xl_info & ~XLR_INFO_MASK;
if (record == NULL)
{
@@ -7852,8 +7855,8 @@ ReadCheckpointRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr,
}
return NULL;
}
- if (record->xl_info != XLOG_CHECKPOINT_SHUTDOWN &&
- record->xl_info != XLOG_CHECKPOINT_ONLINE)
+ if (info != XLOG_CHECKPOINT_SHUTDOWN &&
+ info != XLOG_CHECKPOINT_ONLINE)
{
switch (whichChkpt)
{
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index f2da505..56d4c66 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -462,7 +462,8 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
/*
* Special processing if it's an XLOG SWITCH record
*/
- if (record->xl_rmid == RM_XLOG_ID && record->xl_info == XLOG_SWITCH)
+ if (record->xl_rmid == RM_XLOG_ID &&
+ (record->xl_info & ~XLR_INFO_MASK) == XLOG_SWITCH)
{
/* Pretend it extends to end of segment */
state->EndRecPtr += XLogSegSize - 1;
Hi,
What about the patch attached to make things more consistent?
I have reviewed this patch. It does serve the purpose and looks sane
to me. I am marking it as ready for committer.
With Regards,
Ashutosh Sharma
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Michael Paquier <michael.paquier@gmail.com> writes:
I have just been trapped by the fact that in some code paths we look
at the RMGR information of a WAL record (record->xl_info) but the
filter dedicated to it, ~XLR_INFO_MASK, is not applied. This is
harmful now, but this could lead to problems if in the future new
record-level flags, similar to XLR_SPECIAL_REL_UPDATE, are added.
Disclaimer: I am looking at a patch where a record-level flag makes
sense, still this looks like a separate problem.
What about the patch attached to make things more consistent?
Grepping found a couple of additional places that needed the same
fix. Pushed with those additions.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Nov 5, 2016 at 2:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
I have just been trapped by the fact that in some code paths we look
at the RMGR information of a WAL record (record->xl_info) but the
filter dedicated to it, ~XLR_INFO_MASK, is not applied. This is
harmful now, but this could lead to problems if in the future new
record-level flags, similar to XLR_SPECIAL_REL_UPDATE, are added.
Disclaimer: I am looking at a patch where a record-level flag makes
sense, still this looks like a separate problem.What about the patch attached to make things more consistent?
Grepping found a couple of additional places that needed the same
fix.
Ah, where wasShutdown is assigned. I thought I grepped it as well,
thanks for double-checking.
Pushed with those additions.
Thanks.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers