Re: WAL consistency check facility

On Thu, Nov 3, 2016 at 11:17 PM, Kuntal Ghosh
<kuntalghosh(dot)2007(at)gmail(dot)com> wrote:
> I've updated the patch for review.

Thank you for the new patch. This will be hopefully the last round of
reviews, we are getting close to something that has an acceptable
shape.

+ </para>
+ </listitem>
+ </varlistentry>
+
+ </listitem>
+ </varlistentry>
Did you try to compile the docs? Because that will break. (Likely my
fault). What needs to be done is removing one </listitem> and one
</varlistentry> markup.

+/*-------------------------------------------------------------------------
+ *
+ * bufmask.h
+ * Buffer masking definitions.
+ *
+ * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/storage/bufmask.h
+ */
We could likely survive here with just a copyright mention as 2016,
PGDG... Same remark for bufmask.c.

--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -25,6 +25,7 @@
#include "commands/vacuum.h"
#include "miscadmin.h"
#include "optimizer/plancat.h"
+#include "storage/bufmask.h"
#include "utils/index_selfuncs.h"
#include "utils/rel.h"
This header declaration is not necessary.

+ /*
+ * Mask the Page LSN. Because, we store the page before updating the LSN.
+ * Hence, LSNs of both pages will always be different.
+ */
+ mask_page_lsn(page_norm);
I don't fully understand this comment if phrased this way. Well, I do
understand it, but people who would read this code for the first time
may have a hard time understanding it. So I would suggest removing it,
but add a comment on top of mask_page_lsn() to mention that in
consistency checks the LSN of the two pages compared will likely be
different because of concurrent operations when the WAL is generated
and the state of the page where WAL is applied.

+ maskopaq = (BTPageOpaque)
+ (((char *) page_norm) + ((PageHeader) page_norm)->pd_special);
+ /*
+ * Mask everything on a DELETED page.
+ */
Let's make the code breath and add a space here.

+/* Aligned Buffers dedicated to consistency checks of size BLCKSZ */
+static char *new_page_masked = NULL;
+static char *old_page_masked = NULL;
palloc'd buffers are aligned, so you could just remove the work
"Aligned" in this comment?

+ /* If we've just restored the block from backup image, skip
consistency check. */
+ if (XLogRecHasBlockImage(record, block_id) &&
+ XLogRecBlockImageApply(record, block_id))
+ continue;
Here you could just check for Apply() to decide if continue should be
called or not, and Assert afterwards on HasBlockImage(). The assertion
would help checking for inconsistency errors.

@@ -7810,6 +7929,7 @@ ReadCheckpointRecord(XLogReaderState
*xlogreader, XLogRecPtr RecPtr,
}

record = ReadRecord(xlogreader, RecPtr, LOG, true);
+ info = record->xl_info & ~XLR_INFO_MASK;

if (record == NULL)
{
@@ -7852,8 +7972,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)
Those changes are not directly related to this patch, but make sure
that record checks are done correctly or this patch would just fail.
It may be better to apply those changes independently first per the
patch on this thread:
https://www.postgresql.org/message-id/CAB7nPqSWVyaZJg7_amRKVqRpEP=_=54e+762+2PF9u3Q3+Z0Nw@mail.gmail.com
My recommendation is to do so.

+ /*
+ * Remember that, if WAL consistency check is enabled for
the current rmid,
+ * we always include backup image with the WAL record.
But, during redo we
+ * restore the backup block only if needs_backup is set.
+ */
This could be rewritten a bit:
"if WAL consistency is enabled for the resource manager of this WAL
record, a full-page image is included in the record for the block
modified. During redo, the full-page is replayed only of
BKPIMAGE_APPLY is set."

- * In RBM_ZERO_* modes, if the page doesn't exist, the relation is extended
- * with all-zeroes pages up to the referenced block number. In
- * RBM_ZERO_AND_LOCK and RBM_ZERO_AND_CLEANUP_LOCK modes, the return value
+ * In RBM_ZERO_* modes, if BKPIMAGE_APPLY flag is not set for the backup block,
+ * the relation is extended with all-zeroes pages up to the
referenced block number.
+ * In RBM_ZERO_AND_LOCK and RBM_ZERO_AND_CLEANUP_LOCK modes, the return value
* is always BLK_NEEDS_REDO
You are forgetting to mention "if the page does not exist" in the new
comment block.

+ /* If it has a full-page image and it should be restored, do it. */
+ if (XLogRecHasBlockImage(record, block_id) &&
XLogRecBlockImageApply(record, block_id))
{
Perhaps on two lines?

The headers of the functions in bufmask.c could be more descriptive,
there should be explanations regarding in which aspect they are useful
to guide the user in using them wisely (linked to my comment upstread
if the badly formulated comments before called mask_page_lsn).

Something regarding check_wal_consistency is making uneasy... But I
can't put my finger on what that is now..

I would still for the removal of blkno in the list of arguments of the
masking functions. This is used just for speculative inserts, where we
could just enforce the page number to 0 because this does not matter,
as Peter has mentioned upthread.

Could it be possible to add in pg_xlogdump.c a mention about a FPW
that has the "apply" flag. That would be important for debugging and
development. You could just have for example "(FPW)" for a page that
won't be applied, and "(FPW) apply" for a page where the apply flag is
active.

Please update gindesc.c for FPWs that have the apply flag, issue found
while checking the callers of XLogRecHasBlockImage().

In DecodeXLogRecord(at)xlogreader(dot)c, please add a boolean flag "apply"
and then please could you do some error checks on it. Only one is
needed: if "apply" is true and has_image is false, xlogreader.c should
complain about an inconsistency!

I haven't performed any tests with the patch, and that's all I have
regarding the code. With that done we should be in good shape
code-speaking I think...
--
Michael