Horton

Thread

Commits

  1. Minor fixes for WAL consistency checking.

  1. Re: WAL consistency check facility

    Michael Paquier <michael.paquier@gmail.com> — 2016-11-10T04:55:27Z

    On Wed, Nov 9, 2016 at 11:32 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:
> On Fri, Nov 4, 2016 at 1:32 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> 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.
>
> Thanks a lot for reviewing the patch. Based on your review, I've attached the
> updated patch along with few comments.

Thanks for the new version. pg_xlogdump is really helpful now for debugging.

>> 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...
>
> I've done a fair amount of testing which includes regression tests
> and manual creation of inconsistencies in the page. I've also found the
> reason behind inconsistency in brin revmap page.
> Brin revmap page doesn't have standard page layout. Besides, It doesn't update
> pd_upper and pd_lower in its operations as well. But, during WAL
> insertions, it uses
> REGBUF_STANDARD to register a reference in the WAL record. Hence, when we
> restore image before consistency check, RestoreBlockImage fills the space
> between pd_upper and pd_lower(page hole) with zero. I've posted this as a
> separate thread.
> https://www.postgresql.org/message-id/flat/CAGz5QCJ%3D00UQjScSEFbV%3D0qO5ShTZB9WWz_Fm7%2BWd83zPs9Geg%40mail.gmail.com#CAGz5QCJ=00UQjScSEFbV=0qO5ShTZB9WWz_Fm7+Wd83zPs9Geg@mail.gmail.com

Nice to have spotted the inconsistency. This tool is going to be useful..

I have spent a couple of hours playing with the patch, and worked on
it a bit more with a couple of minor changes:
- In gindesc.c, the if blocks are more readable with brackets.
- Addition of a comment when info is set, to mention that this is done
at the beginning of the routine so as callers of XLogInsert() can pass
the flag for consistency checks.
- apply_image should be reset in ResetDecoder().
- The BRIN problem is here:
2016-11-10 12:24:10 JST [65776]: [23-1] db=,user=,app=,client= FATAL:
Inconsistent page found, rel 1663/16385/30625, forknum 0, blkno 1
2016-11-10 12:24:10 JST [65776]: [24-1] db=,user=,app=,client=
CONTEXT:  xlog redo at 0/9BD31148 for BRIN/UPDATE+INIT: heapBlk 100
pagesPerRange 1 old offnum 11, new offnum 1
2016-11-10 12:24:10 JST [65776]: [25-1] db=,user=,app=,client=
WARNING:  buffer refcount leak: [4540] (rel=base/16385/30625,
blockNum=1, flags=0x93800000, refcount=1 1)
TRAP: FailedAssertion("!(RefCountErrors == 0)", File: "bufmgr.c", Line: 2506)
Now the buffer refcount leak is not normal! The safest thing to do
here is to release the buffer once a copy of it has been taken, and
the leaks goes away when calling FATAL to report the inconsistency.
- When checking for XLR_CHECK_CONSISTENCY, better to add a != 0 to get
a boolean out of it.
- guc_malloc() should be done as late as possible, this simplifies the
code and prevents any memory leaks which is what your patch is doing
when there is an error. (I have finally put my finger on what was
itching me here).
- In btree_mask, the lookup of BTP_DELETED can be deadly simplified.
- I wondered also about putting assign_wal_consistency and
check_wal_consistency in a separate file, say xlogparams.c, concluding
that the current code does nothing bad either even if it adds rmgr.h
in the list of headers in guc.c.
- Some comment blocks are longer than 72~80 characters.
- Typos!

With the patch for BRIN applied, I am able to get installcheck-world
working with wal_consistency = all and a standby doing the consistency
checks behind. Adding wal_consistency = all in PostgresNode.pm, the
recovery tests are passing. This patch is switched as "Ready for
Committer". Thanks for completing this effort begun 3 years ago!
-- 
Michael

  2. Re: WAL consistency check facility

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2016-11-10T15:02:58Z

    On Thu, Nov 10, 2016 at 10:25 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Nov 9, 2016 at 11:32 PM, Kuntal Ghosh
>> Thanks a lot for reviewing the patch. Based on your review, I've attached the

>> I've done a fair amount of testing which includes regression tests
>> and manual creation of inconsistencies in the page. I've also found the
>> reason behind inconsistency in brin revmap page.
>> Brin revmap page doesn't have standard page layout. Besides, It doesn't update
>> pd_upper and pd_lower in its operations as well. But, during WAL
>> insertions, it uses
>> REGBUF_STANDARD to register a reference in the WAL record. Hence, when we
>> restore image before consistency check, RestoreBlockImage fills the space
>> between pd_upper and pd_lower(page hole) with zero. I've posted this as a
>> separate thread.
>> https://www.postgresql.org/message-id/flat/CAGz5QCJ%3D00UQjScSEFbV%3D0qO5ShTZB9WWz_Fm7%2BWd83zPs9Geg%40mail.gmail.com#CAGz5QCJ=00UQjScSEFbV=0qO5ShTZB9WWz_Fm7+Wd83zPs9Geg@mail.gmail.com
>
> Nice to have spotted the inconsistency. This tool is going to be useful..
>
> I have spent a couple of hours playing with the patch, and worked on
> it a bit more with a couple of minor changes:
> - In gindesc.c, the if blocks are more readable with brackets.
> - Addition of a comment when info is set, to mention that this is done
> at the beginning of the routine so as callers of XLogInsert() can pass
> the flag for consistency checks.
> - apply_image should be reset in ResetDecoder().
> - The BRIN problem is here:
> 2016-11-10 12:24:10 JST [65776]: [23-1] db=,user=,app=,client= FATAL:
> Inconsistent page found, rel 1663/16385/30625, forknum 0, blkno 1
> 2016-11-10 12:24:10 JST [65776]: [24-1] db=,user=,app=,client=
> CONTEXT:  xlog redo at 0/9BD31148 for BRIN/UPDATE+INIT: heapBlk 100
> pagesPerRange 1 old offnum 11, new offnum 1
> 2016-11-10 12:24:10 JST [65776]: [25-1] db=,user=,app=,client=
> WARNING:  buffer refcount leak: [4540] (rel=base/16385/30625,
> blockNum=1, flags=0x93800000, refcount=1 1)
> TRAP: FailedAssertion("!(RefCountErrors == 0)", File: "bufmgr.c", Line: 2506)
> Now the buffer refcount leak is not normal! The safest thing to do
> here is to release the buffer once a copy of it has been taken, and
> the leaks goes away when calling FATAL to report the inconsistency.
> - When checking for XLR_CHECK_CONSISTENCY, better to add a != 0 to get
> a boolean out of it.
> - guc_malloc() should be done as late as possible, this simplifies the
> code and prevents any memory leaks which is what your patch is doing
> when there is an error. (I have finally put my finger on what was
> itching me here).
> - In btree_mask, the lookup of BTP_DELETED can be deadly simplified.
> - I wondered also about putting assign_wal_consistency and
> check_wal_consistency in a separate file, say xlogparams.c, concluding
> that the current code does nothing bad either even if it adds rmgr.h
> in the list of headers in guc.c.
> - Some comment blocks are longer than 72~80 characters.
> - Typos!
All the changes make perfect sense to me.

> With the patch for BRIN applied, I am able to get installcheck-world
> working with wal_consistency = all and a standby doing the consistency
> checks behind. Adding wal_consistency = all in PostgresNode.pm, the
> recovery tests are passing. This patch is switched as "Ready for
> Committer". Thanks for completing this effort begun 3 years ago!
Thanks to you for reviewing all the patches in so much detail. Amit, Robert
and Dilip also helped me a lot in developing the feature. Thanks to them
as well.


-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  3. Re: WAL consistency check facility

    Robert Haas <robertmhaas@gmail.com> — 2016-11-10T16:36:35Z

    On Thu, Nov 10, 2016 at 10:02 AM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:
>> With the patch for BRIN applied, I am able to get installcheck-world
>> working with wal_consistency = all and a standby doing the consistency
>> checks behind. Adding wal_consistency = all in PostgresNode.pm, the
>> recovery tests are passing. This patch is switched as "Ready for
>> Committer". Thanks for completing this effort begun 3 years ago!
> Thanks to you for reviewing all the patches in so much detail. Amit, Robert
> and Dilip also helped me a lot in developing the feature. Thanks to them
> as well.

So, who should be credited as co-authors of this patch and in what
order, if and when it gets committed?  If X started this patch and
then Kuntal did a little more work on it, I would credit it as:

X and Kuntal Ghosh

If Kuntal did major work on it, though, then I would think of
something more like:

Kuntal Ghosh, based on an earlier patch from X

If he didn't use any of the old code but just the idea, then I would
do something like this:

Kuntal Ghosh, inspired by a previous patch from X

So, who are all of the people involved in the effort to produce this
patch, and what's the right way to attribute credit?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  4. Re: WAL consistency check facility

    Michael Paquier <michael.paquier@gmail.com> — 2016-11-10T22:06:42Z

    On Fri, Nov 11, 2016 at 1:36 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> So, who are all of the people involved in the effort to produce this
> patch, and what's the right way to attribute credit?

The original idea was from Heikki as he has introduced the idea of
doing such checks if you look at the original thread. I added on top
of it a couple of things like the concept of page masking, and hacked
an early version of the versoin we have now
(https://www.postgresql.org/message-id/CAB7nPqR4vxdKijP+Du82vOcOnGMvutq-gfqiU2dsH4bsM77hYg@mail.gmail.com).
So it seems to me that marking Heikki as an author would be fair as
the original idea is from him. I don't mind being marked only as a
reviewer of the feature, or as someone from which has written an
earlier version of the patch, but I let that up to your judgement.
Kuntai is definitely an author.
-- 
Michael

  5. Re: WAL consistency check facility

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2016-11-11T08:53:00Z

    On Fri, Nov 11, 2016 at 3:36 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Nov 11, 2016 at 1:36 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> So, who are all of the people involved in the effort to produce this
>> patch, and what's the right way to attribute credit?
>
> The original idea was from Heikki as he has introduced the idea of
> doing such checks if you look at the original thread. I added on top
> of it a couple of things like the concept of page masking, and hacked
> an early version of the versoin we have now
> (https://www.postgresql.org/message-id/CAB7nPqR4vxdKijP+Du82vOcOnGMvutq-gfqiU2dsH4bsM77hYg@mail.gmail.com).
> So it seems to me that marking Heikki as an author would be fair as
> the original idea is from him. I don't mind being marked only as a
> reviewer of the feature, or as someone from which has written an
> earlier version of the patch, but I let that up to your judgement.
> Kuntai is definitely an author.
Although lot of changes have been done later, but I've started with the patch
attached in the above thread. Hence, I feel the author of that patch should
also get the credit.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  6. Re: WAL consistency check facility

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2016-11-13T03:06:39Z

    On 11/9/16 11:55 PM, Michael Paquier wrote:
> +     <varlistentry id="guc-wal-consistency" xreflabel="wal_consistency">
> +      <term><varname>wal_consistency</varname> (<type>string</type>)
> +      <indexterm>
> +       <primary><varname>wal_consistency</> configuration parameter</primary>
> +      </indexterm>
> +      </term>
> +      <listitem>
> +       <para>
> +        This parameter is used to check the consistency of WAL records, i.e,
> +        whether the WAL records are inserted and applied correctly. When
> +        <varname>wal_consistency</varname> is enabled for a WAL record, it
> +        stores a full-page image along with the record. When a full-page image
> +        arrives during redo, it compares against the current page to check whether
> +        both are consistent.
> +       </para>

Could we name this something like wal_consistency_checking?

Otherwise it sounds like you use this to select the amount of
consistency in the WAL (similar to, say, wal_level).

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  7. Re: WAL consistency check facility

    Michael Paquier <michael.paquier@gmail.com> — 2016-11-13T07:06:36Z

    On Sun, Nov 13, 2016 at 12:06 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Could we name this something like wal_consistency_checking?
>
> Otherwise it sounds like you use this to select the amount of
> consistency in the WAL (similar to, say, wal_level).

Or wal_check? Or wal_consistency_check?
-- 
Michael

  8. Re: WAL consistency check facility

    Robert Haas <robertmhaas@gmail.com> — 2016-11-15T13:53:01Z

    On Sat, Nov 12, 2016 at 10:06 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 11/9/16 11:55 PM, Michael Paquier wrote:
>> +     <varlistentry id="guc-wal-consistency" xreflabel="wal_consistency">
>> +      <term><varname>wal_consistency</varname> (<type>string</type>)
>> +      <indexterm>
>> +       <primary><varname>wal_consistency</> configuration parameter</primary>
>> +      </indexterm>
>> +      </term>
>> +      <listitem>
>> +       <para>
>> +        This parameter is used to check the consistency of WAL records, i.e,
>> +        whether the WAL records are inserted and applied correctly. When
>> +        <varname>wal_consistency</varname> is enabled for a WAL record, it
>> +        stores a full-page image along with the record. When a full-page image
>> +        arrives during redo, it compares against the current page to check whether
>> +        both are consistent.
>> +       </para>
>
> Could we name this something like wal_consistency_checking?
>
> Otherwise it sounds like you use this to select the amount of
> consistency in the WAL (similar to, say, wal_level).

+1.  I like that name.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  9. Re: WAL consistency check facility

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2016-11-15T15:50:21Z

    On Tue, Nov 15, 2016 at 7:23 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sat, Nov 12, 2016 at 10:06 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> On 11/9/16 11:55 PM, Michael Paquier wrote:
>>> +     <varlistentry id="guc-wal-consistency" xreflabel="wal_consistency">
>>> +      <term><varname>wal_consistency</varname> (<type>string</type>)
>>> +      <indexterm>
>>> +       <primary><varname>wal_consistency</> configuration parameter</primary>
>>> +      </indexterm>
>>> +      </term>
>>> +      <listitem>
>>> +       <para>
>>> +        This parameter is used to check the consistency of WAL records, i.e,
>>> +        whether the WAL records are inserted and applied correctly. When
>>> +        <varname>wal_consistency</varname> is enabled for a WAL record, it
>>> +        stores a full-page image along with the record. When a full-page image
>>> +        arrives during redo, it compares against the current page to check whether
>>> +        both are consistent.
>>> +       </para>
>>
>> Could we name this something like wal_consistency_checking?
>>
>> Otherwise it sounds like you use this to select the amount of
>> consistency in the WAL (similar to, say, wal_level).
>
> +1.  I like that name.
I've modified the guc parameter name as wal_consistency_check (little
hesitant for a participle in suffix :) ). Also, updated the sgml and
variable name accordingly.
FYI, regression test will fail because of an inconsistency in brin
page. I've already submitted a patch for that. Following is the thread
for the same:
https://www.postgresql.org/message-id/flat/CAGz5QCJ%3D00UQjScSEFbV%3D0qO5ShTZB9WWz_Fm7%2BWd83zPs9Geg%40mail.gmail.com#CAGz5QCJ=00UQjScSEFbV=0qO5ShTZB9WWz_Fm7+Wd83zPs9Geg@mail.gmail.com
-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  10. Re: WAL consistency check facility

    Michael Paquier <michael.paquier@gmail.com> — 2016-11-15T17:15:03Z

    On Tue, Nov 15, 2016 at 7:50 AM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:
> I've modified the guc parameter name as wal_consistency_check (little
> hesitant for a participle in suffix :) ). Also, updated the sgml and
> variable name accordingly.

The changes look good to me.
-- 
Michael

  11. Re: WAL consistency check facility

    Michael Paquier <michael.paquier@gmail.com> — 2016-11-29T04:31:51Z

    On Wed, Nov 16, 2016 at 2:15 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Nov 15, 2016 at 7:50 AM, Kuntal Ghosh
> <kuntalghosh.2007@gmail.com> wrote:
>> I've modified the guc parameter name as wal_consistency_check (little
>> hesitant for a participle in suffix :) ). Also, updated the sgml and
>> variable name accordingly.
>
> The changes look good to me.

Moved to CF 2017-01, as no committers have showed up yet :(
-- 
Michael

  12. Re: WAL consistency check facility

    Robert Haas <robertmhaas@gmail.com> — 2016-12-21T17:23:16Z

    On Mon, Nov 28, 2016 at 11:31 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Moved to CF 2017-01, as no committers have showed up yet :(

Seeing no other volunteers, here I am.

On a first read-through of this patch -- I have not studied it in
detail yet -- this looks pretty good to me.  One concern is that this
patch adds a bit of code to XLogInsert(), which is a very hot piece of
code.  Conceivably, that might produce a regression even when this is
disabled; if so, we'd probably need to make it a build-time option.  I
hope that's not necessary, because I think it would be great to
compile this into the server by default, but we better make sure it's
not a problem.  A bulk load into an existing table might be a good
test case.

Aside from that, I think the biggest issue here is that the masking
functions are virtually free of comments, whereas I think they should
have extensive and detailed comments.  For example, in heap_mask, you
have this:

+            page_htup->t_infomask =
+                HEAP_XMIN_COMMITTED | HEAP_XMIN_INVALID |
+                HEAP_XMAX_COMMITTED | HEAP_XMAX_INVALID;

For something like this, you could write "We want to ignore
differences in hint bits, since they can be set by SetHintBits without
emitting WAL.  Force them all to be set so that we don't notice
discrepancies."  Actually, though, I think that you could be a bit
more nuanced here: HEAP_XMIN_COMMITTED + HEAP_XMIN_INVALID =
HEAP_XMIN_FROZEN, so maybe what you should do is clear
HEAP_XMAX_COMMITTED and HEAP_XMAX_INVALID but only clear the others if
one is set but not both.

Anyway, leaving that aside, I think every single change that gets
masked in every single masking routine needs a similar comment,
explaining why that change can happen on the master without also
happening on the standby and hopefully referring to the code that
makes that unlogged change.

I think wal_consistency_checking, as proposed by Peter, is better than
wal_consistency_check, as implemented.

Having StartupXLOG() call pfree() on the masking buffers is a waste of
code.  The process is going to exit anyway.

+                 "Inconsistent page found, rel %u/%u/%u, forknum %u, blkno %u",

Primary error messages aren't capitalized.

+        if (!XLogRecGetBlockTag(record, block_id, &rnode, &forknum, &blkno))
+        {
+            /* Caller specified a bogus block_id. Do nothing. */
+            continue;
+        }

Why would the caller do something so dastardly?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  13. Re: WAL consistency check facility

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2016-12-22T06:14:45Z

    On Wed, Dec 21, 2016 at 10:53 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Nov 28, 2016 at 11:31 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> Moved to CF 2017-01, as no committers have showed up yet :(
>
> Seeing no other volunteers, here I am.
>
Thanks Robert for looking into the patch.

> On a first read-through of this patch -- I have not studied it in
> detail yet -- this looks pretty good to me.  One concern is that this
> patch adds a bit of code to XLogInsert(), which is a very hot piece of
> code.  Conceivably, that might produce a regression even when this is
> disabled; if so, we'd probably need to make it a build-time option.  I
> hope that's not necessary, because I think it would be great to
> compile this into the server by default, but we better make sure it's
> not a problem.  A bulk load into an existing table might be a good
> test case.
>
I'll do this test and post the results.

> +        if (!XLogRecGetBlockTag(record, block_id, &rnode, &forknum, &blkno))
> +        {
> +            /* Caller specified a bogus block_id. Do nothing. */
> +            continue;
> +        }
>
> Why would the caller do something so dastardly?
>
Sorry, it's my bad. I've copied the code from somewhere else, but forgot
to modify the comment. It should be something like
/* block_id is not used. */

I'll modify the above along with other suggested changes.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  14. Re: WAL consistency check facility

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2017-01-05T05:54:24Z

    On Wed, Dec 21, 2016 at 10:53 PM, Robert Haas <robertmhaas@gmail.com> wrote:

> On a first read-through of this patch -- I have not studied it in
> detail yet -- this looks pretty good to me.  One concern is that this
> patch adds a bit of code to XLogInsert(), which is a very hot piece of
> code.  Conceivably, that might produce a regression even when this is
> disabled; if so, we'd probably need to make it a build-time option.  I
> hope that's not necessary, because I think it would be great to
> compile this into the server by default, but we better make sure it's
> not a problem.  A bulk load into an existing table might be a good
> test case.
>
I've done some bulk load testing with 16,32,64 clients. I didn't
notice any regression
in the results.

> Aside from that, I think the biggest issue here is that the masking
> functions are virtually free of comments, whereas I think they should
> have extensive and detailed comments.  For example, in heap_mask, you
> have this:
>
> +            page_htup->t_infomask =
> +                HEAP_XMIN_COMMITTED | HEAP_XMIN_INVALID |
> +                HEAP_XMAX_COMMITTED | HEAP_XMAX_INVALID;
>
> For something like this, you could write "We want to ignore
> differences in hint bits, since they can be set by SetHintBits without
> emitting WAL.  Force them all to be set so that we don't notice
> discrepancies."  Actually, though, I think that you could be a bit
> more nuanced here: HEAP_XMIN_COMMITTED + HEAP_XMIN_INVALID =
> HEAP_XMIN_FROZEN, so maybe what you should do is clear
> HEAP_XMAX_COMMITTED and HEAP_XMAX_INVALID but only clear the others if
> one is set but not both.
>
I've modified it as follows:
+
+                       if (!HeapTupleHeaderXminFrozen(page_htup))
+                               page_htup->t_infomask |= HEAP_XACT_MASK;
+                       else
+                               page_htup->t_infomask |=
HEAP_XMAX_COMMITTED | HEAP_XMAX_INVALID;

> Anyway, leaving that aside, I think every single change that gets
> masked in every single masking routine needs a similar comment,
> explaining why that change can happen on the master without also
> happening on the standby and hopefully referring to the code that
> makes that unlogged change.
>
I've added comments for all the masking routines.

> I think wal_consistency_checking, as proposed by Peter, is better than
> wal_consistency_check, as implemented.
>
Modified to wal_consistency_checking.

> Having StartupXLOG() call pfree() on the masking buffers is a waste of
> code.  The process is going to exit anyway.
>
> +                 "Inconsistent page found, rel %u/%u/%u, forknum %u, blkno %u",
>
Done.

> Primary error messages aren't capitalized.
>
> +        if (!XLogRecGetBlockTag(record, block_id, &rnode, &forknum, &blkno))
> +        {
> +            /* Caller specified a bogus block_id. Do nothing. */
> +            continue;
> +        }
>
> Why would the caller do something so dastardly?
>
Modified to following comment:
+                       /*
+                        * WAL record doesn't contain a block reference
+                        * with the given id. Do nothing.
+                        */

I've attached the patch with the modified changes. PFA.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  15. Re: WAL consistency check facility

    Michael Paquier <michael.paquier@gmail.com> — 2017-01-31T05:35:13Z

    On Thu, Jan 5, 2017 at 2:54 PM, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:
> On Wed, Dec 21, 2016 at 10:53 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
>> On a first read-through of this patch -- I have not studied it in
>> detail yet -- this looks pretty good to me.  One concern is that this
>> patch adds a bit of code to XLogInsert(), which is a very hot piece of
>> code.  Conceivably, that might produce a regression even when this is
>> disabled; if so, we'd probably need to make it a build-time option.  I
>> hope that's not necessary, because I think it would be great to
>> compile this into the server by default, but we better make sure it's
>> not a problem.  A bulk load into an existing table might be a good
>> test case.
>>
> I've done some bulk load testing with 16,32,64 clients. I didn't
> notice any regression
> in the results.
>
>> Aside from that, I think the biggest issue here is that the masking
>> functions are virtually free of comments, whereas I think they should
>> have extensive and detailed comments.  For example, in heap_mask, you
>> have this:
>>
>> +            page_htup->t_infomask =
>> +                HEAP_XMIN_COMMITTED | HEAP_XMIN_INVALID |
>> +                HEAP_XMAX_COMMITTED | HEAP_XMAX_INVALID;
>>
>> For something like this, you could write "We want to ignore
>> differences in hint bits, since they can be set by SetHintBits without
>> emitting WAL.  Force them all to be set so that we don't notice
>> discrepancies."  Actually, though, I think that you could be a bit
>> more nuanced here: HEAP_XMIN_COMMITTED + HEAP_XMIN_INVALID =
>> HEAP_XMIN_FROZEN, so maybe what you should do is clear
>> HEAP_XMAX_COMMITTED and HEAP_XMAX_INVALID but only clear the others if
>> one is set but not both.
>>
> I've modified it as follows:
> +
> +                       if (!HeapTupleHeaderXminFrozen(page_htup))
> +                               page_htup->t_infomask |= HEAP_XACT_MASK;
> +                       else
> +                               page_htup->t_infomask |=
> HEAP_XMAX_COMMITTED | HEAP_XMAX_INVALID;
>
>> Anyway, leaving that aside, I think every single change that gets
>> masked in every single masking routine needs a similar comment,
>> explaining why that change can happen on the master without also
>> happening on the standby and hopefully referring to the code that
>> makes that unlogged change.
>>
> I've added comments for all the masking routines.
>
>> I think wal_consistency_checking, as proposed by Peter, is better than
>> wal_consistency_check, as implemented.
>>
> Modified to wal_consistency_checking.
>
>> Having StartupXLOG() call pfree() on the masking buffers is a waste of
>> code.  The process is going to exit anyway.
>>
>> +                 "Inconsistent page found, rel %u/%u/%u, forknum %u, blkno %u",
>>
> Done.
>
>> Primary error messages aren't capitalized.
>>
>> +        if (!XLogRecGetBlockTag(record, block_id, &rnode, &forknum, &blkno))
>> +        {
>> +            /* Caller specified a bogus block_id. Do nothing. */
>> +            continue;
>> +        }
>>
>> Why would the caller do something so dastardly?
>>
> Modified to following comment:
> +                       /*
> +                        * WAL record doesn't contain a block reference
> +                        * with the given id. Do nothing.
> +                        */
>
> I've attached the patch with the modified changes. PFA.

Moved to CF 2017-03 with same status, "ready for committer".
-- 
Michael

  16. Re: WAL consistency check facility

    Robert Haas <robertmhaas@gmail.com> — 2017-01-31T16:06:15Z

    On Thu, Jan 5, 2017 at 12:54 AM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:
> I've attached the patch with the modified changes. PFA.

Can this patch check contrib/bloom?

+        /*
+         * Mask some line pointer bits, particularly those marked as
+         * used on a master and unused on a standby.
+         */

Comment doesn't explain why we need to do this.

+        /*
+         * For GIN_DELETED page, the page is initialized to empty.
+         * Hence mask everything.
+         */
+        if (opaque->flags & GIN_DELETED)
+            memset(page_norm, MASK_MARKER, BLCKSZ);
+        else
+            mask_unused_space(page_norm);

If the page is initialized to empty, why do we need to mask
anything/everything?  Anyway, it doesn't seem right to mask the
GinPageOpaque structure itself. Or at least it doesn't seem right to
mask the flags word.

+        /*
+         * For gist leaf pages, mask some line pointer bits, particularly
+         * those marked as used on a master and unused on a standby.
+         */

Comment doesn't explain why we need to do this.

+            if (!HeapTupleHeaderXminFrozen(page_htup))
+                page_htup->t_infomask |= HEAP_XACT_MASK;
+            else
+                page_htup->t_infomask |= HEAP_XMAX_COMMITTED |
HEAP_XMAX_INVALID;

Comment doesn't address this logic.  Also, the "else" case shouldn't
exist at all, I think.

+            /*
+             * For a speculative tuple, the content of t_ctid is conflicting
+             * between the backup page and current page. Hence, we set it
+             * to the current block number and current offset.
+             */

Why does it differ?  Is that a bug?

+    /*
+     * Mask everything on a DELETED page since it will be re-initialized
+     * during replay.
+     */
+    if ((maskopaq->btpo_flags & BTP_DELETED) != 0)
+    {
+        /* Mask Page Content */
+        memset(page_norm + SizeOfPageHeaderData, MASK_MARKER,
+               BLCKSZ - SizeOfPageHeaderData);
+
+        /* Mask pd_lower and pd_upper */
+        memset(&((PageHeader) page_norm)->pd_lower, MASK_MARKER,
+               sizeof(uint16));
+        memset(&((PageHeader) page_norm)->pd_upper, MASK_MARKER,
+               sizeof(uint16));

This isn't consistent with the GIN_DELETE case - it is more selective
about what it masks.  Probably that logic should be adapted to look
more like this.

+        /*
+         * Mask some line pointer bits, particularly those marked as
+         * used on a master and unused on a standby.
+         */

Comment (still) doesn't explain why we need to do this.

+    /*
+     * During replay of a btree page split, we don't set the BTP_SPLIT_END
+     * flag of the right sibling and initialize th cycle_id to 0 for the same
+     * page.
+     */

A reference to the name of the redo function might be helpful here
(and in some other places), unless it's just ${AMNAME}_redo.  "th" is
a typo for "the".

+                        appendStringInfoString(buf, " (full page
image, apply)");
+                    else
+                        appendStringInfoString(buf, " (full page image)");

How about "(full page image)" and "(full page image, for WAL verification)"?

Similarly in XLogDumpDisplayRecord, I think we should assume that
"FPW" means what it has always meant, and leave that output alone.
Instead, distinguish the WAL-consistency-checking case when it
happens, e.g. "(FPW for consistency check)".

+checkConsistency(XLogReaderState *record)

How about CheckXLogConsistency()?

+         * If needs_backup is true or wal consistency check is enabled for

...or WAL checking is enabled...

+             * If WAL consistency is enabled for the resource manager of

If WAL consistency checking is enabled...

+ * Note: when a backup block is available in XLOG with BKPIMAGE_APPLY flag

with the BKPIMAGE_APPLY flag

- * 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 the page doesn't exist or BKPIMAGE_APPLY flag
+ * is not set for the backup block, the relation is extended with all-zeroes
+ * pages up to the referenced block number.

OK, I'm puzzled by this.  Surely we don't want the WAL consistency
checking facility to cause the relation to be extended like this.  And
I don't see why it would, because the WAL consistency checking happens
after the page changes have already been applied.  I wonder if this
hunk is in error and should be dropped.

+    PageXLogRecPtrSet(phdr->pd_lsn, PG_UINT64_MAX);
+    phdr->pd_prune_xid = PG_UINT32_MAX;
+    phdr->pd_flags |= PD_PAGE_FULL | PD_HAS_FREE_LINES;
+    phdr->pd_flags |= PD_ALL_VISIBLE;
+#define MASK_MARKER        0xFF
(and many others)

Why do we mask by setting bits rather than clearing bits?  My
intuition would have been to zero things we want to mask, rather than
setting them to one.

+                {
+                    newwalconsistency[i] = true;
+                }

Superfluous braces.

+    /*
+     * Leave if no masking functions defined, this is possible in the case
+     * resource managers generating just full page writes, comparing an
+     * image to itself has no meaning in those cases.
+     */
+    if (RmgrTable[rmid].rm_mask == NULL)
+        return;

...and also...

+            /*
+             * This feature is enabled only for the resource managers where
+             * a masking function is defined.
+             */
+            for (i = 0; i <= RM_MAX_ID; i++)
+            {
+                if (RmgrTable[i].rm_mask != NULL)

Why do we assume that the feature is only enabled for RMs that have a
mask function?  Why not instead assume that if there's no masking
function, no masking is required?

+        /* Definitely not an individual resource manager. Check for 'all'. */
+        if (pg_strcasecmp(tok, "all") == 0)

It seems like it might be cleaner to check for "all" first, and then
check for individual RMs afterward.

+    /*
+     * Parameter should contain either 'all' or a combination of resource
+     * managers.
+     */
+    if (isAll && isRmgrId)
+    {
+        GUC_check_errdetail("Invalid value combination");
+        return false;
+    }

That error message is not very clear, and I don't see why we even need
to check this.  If someone sets wal_consistency_checking = hash, all,
let's just set it for all and the fact that hash is also set won't
matter to anything.

+    void        (*rm_mask) (char *page, BlockNumber blkno);

Could the page be passed as type "Page" rather than a "char *" to make
things more convenient for the masking functions?  If not, could those
functions at least do something like "Page page = (Page) pagebytes;"
rather than "Page page_norm = (Page) page;"?

+        /*
+         * Read the contents from the current buffer and store it in a
+         * temporary page.
+         */
+        buf = XLogReadBufferExtended(rnode, forknum, blkno,
+                                          RBM_NORMAL);
+        if (!BufferIsValid(buf))
+            continue;
+
+        new_page = BufferGetPage(buf);
+
+        /*
+         * Read the contents from the backup copy, stored in WAL record
+         * and store it in a temporary page. There is not need to allocate
+         * a new page here, a local buffer is fine to hold its contents and
+         * a mask can be directly applied on it.
+         */
+        if (!RestoreBlockImage(record, block_id, old_page_masked))
+            elog(ERROR, "failed to restore block image");
+
+        /*
+         * Take a copy of the new page where WAL has been applied to have
+         * a comparison base before masking it...
+         */
+        memcpy(new_page_masked, new_page, BLCKSZ);
+
+        /* No need for this page anymore now that a copy is in */
+        ReleaseBuffer(buf);

The order of operations is strange here.  We read the "new" page,
holding the pin (but no lock?).  Then we restore the block image into
old_page_masked.  Now we copy the new page and release the pin.  It
would be better, ISTM, to rearrange that so that we finish with the
new page and release the pin before dealing with the old page.  Also,
I think we need to actually lock the buffer before copying it.  Maybe
that's not strictly necessary since this is all happening on the
standby, but it seems like a bad idea to set the precedent that you
can read a page without taking the content lock.

I think the "new" and "old" page terminology is kinda weird too.
Maybe we should call them the "replay image" and the "master image" or
something like that.   A few more comments wouldn't hurt either.

+     * Also mask the all-visible flag.
+     *
+     * XXX: It is unfortunate that we have to do this. If the flag is set
+     * incorrectly, that's serious, and we would like to catch it. If the flag
+     * is cleared incorrectly, that's serious too. But redo of HEAP_CLEAN
+     * records don't currently set the flag, even though it is set in the
+     * master, so we must silence failures that that causes.
+     */
+    phdr->pd_flags |= PD_ALL_VISIBLE;

I'm puzzled by the reference to HEAP_CLEAN.  The thing that might set
the all-visible bit is XLOG_HEAP2_VISIBLE, not XLOG_HEAP2_CLEAN.
Unless I'm missing something, there's no situation in which
XLOG_HEAP2_CLEAN might be associated with setting PD_ALL_VISIBLE.
Also, XLOG_HEAP2_VISIBLE records do SOMETIMES set the bit, just not
always.  And there's a good reason for that, which is explained in
this comment:

         * We don't bump the LSN of the heap page when setting the visibility
         * map bit (unless checksums or wal_hint_bits is enabled, in which
         * case we must), because that would generate an unworkable volume of
         * full-page writes.  This exposes us to torn page hazards, but since
         * we're not inspecting the existing page contents in any way, we
         * don't care.
         *
         * However, all operations that clear the visibility map bit *do* bump
         * the LSN, and those operations will only be replayed if the XLOG LSN
         * follows the page LSN.  Thus, if the page LSN has advanced past our
         * XLOG record's LSN, we mustn't mark the page all-visible, because
         * the subsequent update won't be replayed to clear the flag.

So I think this comment needs to be rewritten with a bit more nuance.

+extern void mask_unused_space(Page page);
+#endif

Missing newline.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  17. Re: WAL consistency check facility

    Michael Paquier <michael.paquier@gmail.com> — 2017-02-01T02:31:12Z

    On Wed, Feb 1, 2017 at 1:06 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Jan 5, 2017 at 12:54 AM, Kuntal Ghosh
> <kuntalghosh.2007@gmail.com> wrote:
>> I've attached the patch with the modified changes. PFA.
>
> Can this patch check contrib/bloom?

Only full pages are applied at redo by the generic WAL facility. So
you would finish by comparing a page with itself in generic_redo.

> +            /*
> +             * For a speculative tuple, the content of t_ctid is conflicting
> +             * between the backup page and current page. Hence, we set it
> +             * to the current block number and current offset.
> +             */
>
> Why does it differ?  Is that a bug?

This has been discussed twice in this thread, once by me, once by Alvaro:
https://www.postgresql.org/message-id/CAM3SWZQC8nUgp8SjKDY3d74VLpdf9puHc7-n3zf4xcr_bghPzg%40mail.gmail.com
https://www.postgresql.org/message-id/CAB7nPqQTLGvn_XePjS07kZMMw46kS6S7LfsTocK%2BgLpTN0bcZw%40mail.gmail.com

> +    /*
> +     * Leave if no masking functions defined, this is possible in the case
> +     * resource managers generating just full page writes, comparing an
> +     * image to itself has no meaning in those cases.
> +     */
> +    if (RmgrTable[rmid].rm_mask == NULL)
> +        return;
>
> ...and also...
>
> +            /*
> +             * This feature is enabled only for the resource managers where
> +             * a masking function is defined.
> +             */
> +            for (i = 0; i <= RM_MAX_ID; i++)
> +            {
> +                if (RmgrTable[i].rm_mask != NULL)
>
> Why do we assume that the feature is only enabled for RMs that have a
> mask function?  Why not instead assume that if there's no masking
> function, no masking is required?

Not all RMs work on full pages. Tracking in smgr.h the list of RMs
that are no-ops when masking things is easier than having empty
routines declared all over the code base. Don't you think so>

> +    void        (*rm_mask) (char *page, BlockNumber blkno);
>
> Could the page be passed as type "Page" rather than a "char *" to make
> things more convenient for the masking functions?  If not, could those
> functions at least do something like "Page page = (Page) pagebytes;"
> rather than "Page page_norm = (Page) page;"?

xlog_internal.h is used as well by frontends, this makes the header
dependencies cleaner. (I have looked at using Page when hacking this
stuff, but the header dependencies are not worth it, I don't recall
all the details though this was a couple of months back).
-- 
Michael

  18. Re: WAL consistency check facility

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2017-02-01T05:57:37Z

    On Tue, Jan 31, 2017 at 9:36 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Jan 5, 2017 at 12:54 AM, Kuntal Ghosh
> <kuntalghosh.2007@gmail.com> wrote:
>> I've attached the patch with the modified changes. PFA.
Thanks Robert for taking your time for the review.  I'll update the
patch with the changes suggested by you.


-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  19. Re: WAL consistency check facility

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2017-02-01T06:31:45Z

    On Wed, Feb 1, 2017 at 8:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Feb 1, 2017 at 1:06 AM, Robert Haas <robertmhaas@gmail.com> wrote:

>> +    /*
>> +     * Leave if no masking functions defined, this is possible in the case
>> +     * resource managers generating just full page writes, comparing an
>> +     * image to itself has no meaning in those cases.
>> +     */
>> +    if (RmgrTable[rmid].rm_mask == NULL)
>> +        return;
>>
>> ...and also...
>>
>> +            /*
>> +             * This feature is enabled only for the resource managers where
>> +             * a masking function is defined.
>> +             */
>> +            for (i = 0; i <= RM_MAX_ID; i++)
>> +            {
>> +                if (RmgrTable[i].rm_mask != NULL)
>>
>> Why do we assume that the feature is only enabled for RMs that have a
>> mask function?  Why not instead assume that if there's no masking
>> function, no masking is required?
>
> Not all RMs work on full pages. Tracking in smgr.h the list of RMs
> that are no-ops when masking things is easier than having empty
> routines declared all over the code base. Don't you think so>
>
Robert's suggestion surely makes the approach more general. But,  the
existing approach makes it easier to decide the RM's for which we
support the consistency check facility. Surely, we can use a list to
track the RMs which should (/not) be masked. But, I think we always
have to mask the lsn of the pages at least. Isn't it?

>> +    void        (*rm_mask) (char *page, BlockNumber blkno);
>>
>> Could the page be passed as type "Page" rather than a "char *" to make
>> things more convenient for the masking functions?  If not, could those
>> functions at least do something like "Page page = (Page) pagebytes;"
>> rather than "Page page_norm = (Page) page;"?
>
> xlog_internal.h is used as well by frontends, this makes the header
> dependencies cleaner. (I have looked at using Page when hacking this
> stuff, but the header dependencies are not worth it, I don't recall
> all the details though this was a couple of months back).
+ 1



-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  20. Re: WAL consistency check facility

    Robert Haas <robertmhaas@gmail.com> — 2017-02-02T02:25:24Z

    On Tue, Jan 31, 2017 at 9:31 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Feb 1, 2017 at 1:06 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Thu, Jan 5, 2017 at 12:54 AM, Kuntal Ghosh
>> <kuntalghosh.2007@gmail.com> wrote:
>>> I've attached the patch with the modified changes. PFA.
>>
>> Can this patch check contrib/bloom?
>
> Only full pages are applied at redo by the generic WAL facility. So
> you would finish by comparing a page with itself in generic_redo.

generic_redo is more complicated than just restoring an FPI.  I admit
that generic_redo *probably* has no bugs, but I don't see why it would
hurt to allow it to be checked.  It's not IMPOSSIBLE that restoring
the page delta could go wrong somehow.

>> +            /*
>> +             * For a speculative tuple, the content of t_ctid is conflicting
>> +             * between the backup page and current page. Hence, we set it
>> +             * to the current block number and current offset.
>> +             */
>>
>> Why does it differ?  Is that a bug?
>
> This has been discussed twice in this thread, once by me, once by Alvaro:
> https://www.postgresql.org/message-id/CAM3SWZQC8nUgp8SjKDY3d74VLpdf9puHc7-n3zf4xcr_bghPzg%40mail.gmail.com
> https://www.postgresql.org/message-id/CAB7nPqQTLGvn_XePjS07kZMMw46kS6S7LfsTocK%2BgLpTN0bcZw%40mail.gmail.com

Sorry, I missed/forgot about that.  I think this is evidence that the
comment isn't really good enough.  It says "hey, this might differ"
... with no real explanation of why it might differ or why that's OK.
If there were an explanation there, I wouldn't have flagged it.

>> +    /*
>> +     * Leave if no masking functions defined, this is possible in the case
>> +     * resource managers generating just full page writes, comparing an
>> +     * image to itself has no meaning in those cases.
>> +     */
>> +    if (RmgrTable[rmid].rm_mask == NULL)
>> +        return;
>>
>> ...and also...
>>
>> +            /*
>> +             * This feature is enabled only for the resource managers where
>> +             * a masking function is defined.
>> +             */
>> +            for (i = 0; i <= RM_MAX_ID; i++)
>> +            {
>> +                if (RmgrTable[i].rm_mask != NULL)
>>
>> Why do we assume that the feature is only enabled for RMs that have a
>> mask function?  Why not instead assume that if there's no masking
>> function, no masking is required?
>
> Not all RMs work on full pages. Tracking in smgr.h the list of RMs
> that are no-ops when masking things is easier than having empty
> routines declared all over the code base. Don't you think so>

Sure, but I don't think that's what the code is doing.  If an RM is a
no-op when masking things, it must define an empty function.  If it
defines no function, checking is disabled.  I think we want to be able
to check any RM.  No?

>> +    void        (*rm_mask) (char *page, BlockNumber blkno);
>>
>> Could the page be passed as type "Page" rather than a "char *" to make
>> things more convenient for the masking functions?  If not, could those
>> functions at least do something like "Page page = (Page) pagebytes;"
>> rather than "Page page_norm = (Page) page;"?
>
> xlog_internal.h is used as well by frontends, this makes the header
> dependencies cleaner. (I have looked at using Page when hacking this
> stuff, but the header dependencies are not worth it, I don't recall
> all the details though this was a couple of months back).

OK.  I still think page_norm is a lame variable name.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  21. Re: WAL consistency check facility

    Amit Kapila <amit.kapila16@gmail.com> — 2017-02-07T11:32:47Z

    On Tue, Jan 31, 2017 at 9:36 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> +            if (!HeapTupleHeaderXminFrozen(page_htup))
> +                page_htup->t_infomask |= HEAP_XACT_MASK;
> +            else
> +                page_htup->t_infomask |= HEAP_XMAX_COMMITTED |
> HEAP_XMAX_INVALID;
>
> Comment doesn't address this logic.  Also, the "else" case shouldn't
> exist at all, I think.
>

In the *if* check, it just checks frozen status of xmin, so I think
you need to mask xmax related bits in else check.  Can you explain
what makes you think that the else case shouldn't exist?


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  22. Re: WAL consistency check facility

    Robert Haas <robertmhaas@gmail.com> — 2017-02-07T12:48:05Z

    On Tue, Feb 7, 2017 at 6:32 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Jan 31, 2017 at 9:36 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> +            if (!HeapTupleHeaderXminFrozen(page_htup))
>> +                page_htup->t_infomask |= HEAP_XACT_MASK;
>> +            else
>> +                page_htup->t_infomask |= HEAP_XMAX_COMMITTED |
>> HEAP_XMAX_INVALID;
>>
>> Comment doesn't address this logic.  Also, the "else" case shouldn't
>> exist at all, I think.
>>
>
> In the *if* check, it just checks frozen status of xmin, so I think
> you need to mask xmax related bits in else check.  Can you explain
> what makes you think that the else case shouldn't exist?

Oh, you're right.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  23. Re: WAL consistency check facility

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2017-02-08T06:25:10Z

    On Tue, Jan 31, 2017 at 9:36 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Jan 5, 2017 at 12:54 AM, Kuntal Ghosh
> <kuntalghosh.2007@gmail.com> wrote:
>> I've attached the patch with the modified changes. PFA.
>
> Can this patch check contrib/bloom?
>
Added support for generic resource manager type.

> +        /*
> +         * Mask some line pointer bits, particularly those marked as
> +         * used on a master and unused on a standby.
> +         */
>
> Comment doesn't explain why we need to do this.
>
Added comments.

> +        /*
> +         * For GIN_DELETED page, the page is initialized to empty.
> +         * Hence mask everything.
> +         */
> +        if (opaque->flags & GIN_DELETED)
> +            memset(page_norm, MASK_MARKER, BLCKSZ);
> +        else
> +            mask_unused_space(page_norm);
>
> If the page is initialized to empty, why do we need to mask
> anything/everything?  Anyway, it doesn't seem right to mask the
> GinPageOpaque structure itself. Or at least it doesn't seem right to
> mask the flags word.
>
Modified accordingly.

>
> +            if (!HeapTupleHeaderXminFrozen(page_htup))
> +                page_htup->t_infomask |= HEAP_XACT_MASK;
> +            else
> +                page_htup->t_infomask |= HEAP_XMAX_COMMITTED |
> HEAP_XMAX_INVALID;
>
> Comment doesn't address this logic.  Also, the "else" case shouldn't
> exist at all, I think.
Added comments. I think that "Else" part is needed for xmax.

>
> +            /*
> +             * For a speculative tuple, the content of t_ctid is conflicting
> +             * between the backup page and current page. Hence, we set it
> +             * to the current block number and current offset.
> +             */
>
> Why does it differ?  Is that a bug?
>
Added comments.

>
> +    /*
> +     * During replay of a btree page split, we don't set the BTP_SPLIT_END
> +     * flag of the right sibling and initialize th cycle_id to 0 for the same
> +     * page.
> +     */
>
> A reference to the name of the redo function might be helpful here
> (and in some other places), unless it's just ${AMNAME}_redo.  "th" is
> a typo for "the".
Corrected.

> +                        appendStringInfoString(buf, " (full page
> image, apply)");
> +                    else
> +                        appendStringInfoString(buf, " (full page image)");
>
> How about "(full page image)" and "(full page image, for WAL verification)"?
>
> Similarly in XLogDumpDisplayRecord, I think we should assume that
> "FPW" means what it has always meant, and leave that output alone.
> Instead, distinguish the WAL-consistency-checking case when it
> happens, e.g. "(FPW for consistency check)".
>
Corrected.

> +checkConsistency(XLogReaderState *record)
>
> How about CheckXLogConsistency()?
>
> +         * If needs_backup is true or wal consistency check is enabled for
>
> ...or WAL checking is enabled...
>
> +             * If WAL consistency is enabled for the resource manager of
>
> If WAL consistency checking is enabled...
>
> + * Note: when a backup block is available in XLOG with BKPIMAGE_APPLY flag
>
> with the BKPIMAGE_APPLY flag
Modified accordingly.

>
> - * 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 the page doesn't exist or BKPIMAGE_APPLY flag
> + * is not set for the backup block, the relation is extended with all-zeroes
> + * pages up to the referenced block number.
>
> OK, I'm puzzled by this.  Surely we don't want the WAL consistency
> checking facility to cause the relation to be extended like this.  And
> I don't see why it would, because the WAL consistency checking happens
> after the page changes have already been applied.  I wonder if this
> hunk is in error and should be dropped.
>
Dropped comments.

> +    PageXLogRecPtrSet(phdr->pd_lsn, PG_UINT64_MAX);
> +    phdr->pd_prune_xid = PG_UINT32_MAX;
> +    phdr->pd_flags |= PD_PAGE_FULL | PD_HAS_FREE_LINES;
> +    phdr->pd_flags |= PD_ALL_VISIBLE;
> +#define MASK_MARKER        0xFF
> (and many others)
>
> Why do we mask by setting bits rather than clearing bits?  My
> intuition would have been to zero things we want to mask, rather than
> setting them to one.
>
Modified accordingly so that we can zero things during masking instead
of setting them to one.

> +                {
> +                    newwalconsistency[i] = true;
> +                }
>
> Superfluous braces.
>
Corrected.

> +    /*
> +     * Leave if no masking functions defined, this is possible in the case
> +     * resource managers generating just full page writes, comparing an
> +     * image to itself has no meaning in those cases.
> +     */
> +    if (RmgrTable[rmid].rm_mask == NULL)
> +        return;
>
> ...and also...
>
> +            /*
> +             * This feature is enabled only for the resource managers where
> +             * a masking function is defined.
> +             */
> +            for (i = 0; i <= RM_MAX_ID; i++)
> +            {
> +                if (RmgrTable[i].rm_mask != NULL)
>
> Why do we assume that the feature is only enabled for RMs that have a
> mask function?  Why not instead assume that if there's no masking
> function, no masking is required?
I've introduced a function in rmgr.c, named
consistencyCheck_is_enabled, which returns true if
wal_consistency_checking is enabled for a resource manager. It does
not have any dependency on the masking function. If a masking function
is defined, then we mask the page before consistency check. However,
I'm not sure whether rmgr.c is the right place to define the function
consistencyCheck_is_enabled.

>
> +        /* Definitely not an individual resource manager. Check for 'all'. */
> +        if (pg_strcasecmp(tok, "all") == 0)
>
> It seems like it might be cleaner to check for "all" first, and then
> check for individual RMs afterward.
>
Done.

> +    /*
> +     * Parameter should contain either 'all' or a combination of resource
> +     * managers.
> +     */
> +    if (isAll && isRmgrId)
> +    {
> +        GUC_check_errdetail("Invalid value combination");
> +        return false;
> +    }
>
> That error message is not very clear, and I don't see why we even need
> to check this.  If someone sets wal_consistency_checking = hash, all,
> let's just set it for all and the fact that hash is also set won't
> matter to anything.
>
Modified accordingly.

> +    void        (*rm_mask) (char *page, BlockNumber blkno);
>
> Could the page be passed as type "Page" rather than a "char *" to make
> things more convenient for the masking functions?  If not, could those
> functions at least do something like "Page page = (Page) pagebytes;"
> rather than "Page page_norm = (Page) page;"?
>
Modified it as "Page tempPage = (Page) page;"

> +        /*
> +         * Read the contents from the current buffer and store it in a
> +         * temporary page.
> +         */
> +        buf = XLogReadBufferExtended(rnode, forknum, blkno,
> +                                          RBM_NORMAL);
> +        if (!BufferIsValid(buf))
> +            continue;
> +
> +        new_page = BufferGetPage(buf);
> +
> +        /*
> +         * Read the contents from the backup copy, stored in WAL record
> +         * and store it in a temporary page. There is not need to allocate
> +         * a new page here, a local buffer is fine to hold its contents and
> +         * a mask can be directly applied on it.
> +         */
> +        if (!RestoreBlockImage(record, block_id, old_page_masked))
> +            elog(ERROR, "failed to restore block image");
> +
> +        /*
> +         * Take a copy of the new page where WAL has been applied to have
> +         * a comparison base before masking it...
> +         */
> +        memcpy(new_page_masked, new_page, BLCKSZ);
> +
> +        /* No need for this page anymore now that a copy is in */
> +        ReleaseBuffer(buf);
>
> The order of operations is strange here.  We read the "new" page,
> holding the pin (but no lock?).  Then we restore the block image into
> old_page_masked.  Now we copy the new page and release the pin.  It
> would be better, ISTM, to rearrange that so that we finish with the
> new page and release the pin before dealing with the old page.  Also,
> I think we need to actually lock the buffer before copying it.  Maybe
> that's not strictly necessary since this is all happening on the
> standby, but it seems like a bad idea to set the precedent that you
> can read a page without taking the content lock.
>
Modified accordingly.

> I think the "new" and "old" page terminology is kinda weird too.
> Maybe we should call them the "replay image" and the "master image" or
> something like that.   A few more comments wouldn't hurt either.
>
Done.

> +     * Also mask the all-visible flag.
> +     *
> +     * XXX: It is unfortunate that we have to do this. If the flag is set
> +     * incorrectly, that's serious, and we would like to catch it. If the flag
> +     * is cleared incorrectly, that's serious too. But redo of HEAP_CLEAN
> +     * records don't currently set the flag, even though it is set in the
> +     * master, so we must silence failures that that causes.
> +     */
> +    phdr->pd_flags |= PD_ALL_VISIBLE;
>
> I'm puzzled by the reference to HEAP_CLEAN.  The thing that might set
> the all-visible bit is XLOG_HEAP2_VISIBLE, not XLOG_HEAP2_CLEAN.
> Unless I'm missing something, there's no situation in which
> XLOG_HEAP2_CLEAN might be associated with setting PD_ALL_VISIBLE.
> Also, XLOG_HEAP2_VISIBLE records do SOMETIMES set the bit, just not
> always.  And there's a good reason for that, which is explained in
> this comment:
>
>          * We don't bump the LSN of the heap page when setting the visibility
>          * map bit (unless checksums or wal_hint_bits is enabled, in which
>          * case we must), because that would generate an unworkable volume of
>          * full-page writes.  This exposes us to torn page hazards, but since
>          * we're not inspecting the existing page contents in any way, we
>          * don't care.
>          *
>          * However, all operations that clear the visibility map bit *do* bump
>          * the LSN, and those operations will only be replayed if the XLOG LSN
>          * follows the page LSN.  Thus, if the page LSN has advanced past our
>          * XLOG record's LSN, we mustn't mark the page all-visible, because
>          * the subsequent update won't be replayed to clear the flag.
>
> So I think this comment needs to be rewritten with a bit more nuance.
>
Corrected.

> +extern void mask_unused_space(Page page);
> +#endif
>
> Missing newline.
>
Done.

Thank you Robert for the review. Please find the updated patch in the
attachment.

Thanks to Amit Kapila and Dilip Kumar for their suggestions in offline
discussions.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  24. Re: WAL consistency check facility

    Robert Haas <robertmhaas@gmail.com> — 2017-02-08T20:56:00Z

    On Wed, Feb 8, 2017 at 1:25 AM, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:
> Thank you Robert for the review. Please find the updated patch in the
> attachment.

I have committed this patch after fairly extensive revisions:

* Rewrote the documentation to give some idea what the underlying
mechanism of operation of the feature is, so that users who choose to
enable this will hopefully have some understanding of what they've
turned on.
* Renamed "char *page" arguments to "char *pagedata" and "Page page",
because tempPage doesn't seem to be to be any better a name than
page_norm.
* Moved bufmask.c to src/backend/access/common, because there's no
code in src/backend/storage/buffer that knows anything about the
format of pages; that is the job of AMs, hence src/backend/access.
* Improved some comments in bufmask.c
* Removed consistencyCheck_is_enabled in favor of determining which
RMs support masking by the presence of absence of an rm_mask function.
* Removed assertion in checkXLogConsistency that consistency checking
is enabled for the RM; that's trivially false if
wal_consistency_checking is not the same on the master and the
standby.  (Note that quite apart from the issue of whether this
function should exist at all, adding it to a header file after the
closing #endif guard is certainly not right.)
* Changed checkXLogConsistency to use RBM_NORMAL_NO_LOG instead of
RBM_NORMAL.  I'm not sure if there are any cases where this makes a
difference, but it seems safer.
* Changed checkXLogConsistency to skip pages whose LSN is newer than
that of the record.  Without this, if you shut down recovery and
restart it, it complains of inconsistent pages and dies.  (I'm not
sure this is the only scenario that needs to be covered; it would be
smart to do more testing of restarting the standby.)
* Made wal_consistency_checking a developer option instead of a WAL
option.  Even though it CAN be used in production, we don't
particularly want to encourage that; enabling WAL consistency checking
has a big performance cost and makes your system more fragile not less
-- a WAL consistency failure causes your standby to die a hard death.
(Maybe there should be a way to suppress consistency checking on the
standby -- but I think not just by requiring wal_consistency_checking
on both ends.  Or maybe we should just downgrade the FATAL to WARNING;
blowing up the standby irrevocably seems like poor behavior.)
* Coding style improvement in check_wal_consistency_checking.
* Removed commas in messages added to pg_xlogdump; those didn't look
good to me, on further review.
* Comment improvements in xlog_internal.h and xlogreader.h

I also bumped XLOG_PAGE_MAGIC (which is normally done by the
committer, not the patch author, so I wasn't expecting that to be in
the patch as submitted).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  25. Re: WAL consistency check facility

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2017-02-09T05:48:47Z

    On Thu, Feb 9, 2017 at 2:26 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Feb 8, 2017 at 1:25 AM, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:
>> Thank you Robert for the review. Please find the updated patch in the
>> attachment.
>
> I have committed this patch after fairly extensive revisions:
>
Thank you, Robert, for the above corrections and commit. Thanks to
Michael Paquier, Peter Eisentraut, Amit Kapila, Álvaro Herrera, and
Simon Riggs for taking their time to complete the patch. It was a
great learning experience for me.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  26. Re: WAL consistency check facility

    Michael Paquier <michael.paquier@gmail.com> — 2017-02-10T01:17:06Z

    On Thu, Feb 9, 2017 at 5:56 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Feb 8, 2017 at 1:25 AM, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:
>> Thank you Robert for the review. Please find the updated patch in the
>> attachment.
>
> I have committed this patch after fairly extensive revisions:

Cool. I had finally a look at what has been committed in a507b869.
Running regression tests with all RMGRs enabled, a single installcheck
generates 7GB of WAL. Woah.

> * Rewrote the documentation to give some idea what the underlying
> mechanism of operation of the feature is, so that users who choose to
> enable this will hopefully have some understanding of what they've
> turned on.

Thanks, those look good to me.

> * Renamed "char *page" arguments to "char *pagedata" and "Page page",
> because tempPage doesn't seem to be to be any better a name than
> page_norm.

> * Removed assertion in checkXLogConsistency that consistency checking
> is enabled for the RM; that's trivially false if
> wal_consistency_checking is not the same on the master and the
> standby.  (Note that quite apart from the issue of whether this
> function should exist at all, adding it to a header file after the
> closing #endif guard is certainly not right.)

I recall doing those two things the same way as in the commit. Not
sure at which point they have been re-introduced.

> * Changed checkXLogConsistency to skip pages whose LSN is newer than
> that of the record.  Without this, if you shut down recovery and
> restart it, it complains of inconsistent pages and dies.  (I'm not
> sure this is the only scenario that needs to be covered; it would be
> smart to do more testing of restarting the standby.)

Good point.

> -- a WAL consistency failure causes your standby to die a hard death.
> (Maybe there should be a way to suppress consistency checking on the
> standby -- but I think not just by requiring wal_consistency_checking
> on both ends.  Or maybe we should just downgrade the FATAL to WARNING;
> blowing up the standby irrevocably seems like poor behavior.)

Having a FATAL is useful for buildfarm members, that would show up in
red. Having a switch to generate a warning would be useful for live
deployments I agree. Now I think that we need as well two things:
- A recovery test to run regression tests with a standby behind.
- Extend the TAP tests so as it is possible to fill in postgresql.conf
with custom variables.
- have the buildfarm client run recovery tests!
I am fine to write those patches.

> I also bumped XLOG_PAGE_MAGIC (which is normally done by the
> committer, not the patch author, so I wasn't expecting that to be in
> the patch as submitted).

Here are a couple of things I have noticed while looking at the code.

+ * Portions Copyright (c) 2016, PostgreSQL Global Development Group
s/2016/2017/ in bufmask.c and bufmask.h.

+       if (ItemIdIsNormal(iid))
+       {
+
+           HeapTupleHeader page_htup = (HeapTupleHeader) page_item;
Unnecessary newline here.

+        * Read the contents from the backup copy, stored in WAL record and
+        * store it in a temporary page. There is not need to allocate a new
+        * page here, a local buffer is fine to hold its contents and a mask
+        * can be directly applied on it.
s/not need/no need/.

In checkXLogConsistency(), FPWs that have the flag BKPIMAGE_APPLY set
will still be checked, resulting in a FPW being compared to itself. I
think that those had better be bypassed.

Please find attached a patch with those fixes.
-- 
Michael

  27. Re: WAL consistency check facility

    Robert Haas <robertmhaas@gmail.com> — 2017-02-14T17:43:58Z

    On Thu, Feb 9, 2017 at 8:17 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Please find attached a patch with those fixes.

Committed, but I changed the copyright dates to 2016-2017 rather than
just 2017 since surely some of the code was originally written before
2017.  Even that might not really be going back far enough, but it
doesn't matter too much.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  28. Re: WAL consistency check facility

    Michael Paquier <michael.paquier@gmail.com> — 2017-02-14T22:16:37Z

    On Wed, Feb 15, 2017 at 2:43 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Feb 9, 2017 at 8:17 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> Please find attached a patch with those fixes.
>
> Committed, but I changed the copyright dates to 2016-2017 rather than
> just 2017 since surely some of the code was originally written before
> 2017.  Even that might not really be going back far enough, but it
> doesn't matter too much.

Just for curiosity: does the moment when the code has been written or
committed counts? It's no big deal seeing how liberal the Postgres
license is, but this makes me wonder...
-- 
Michael

  29. Re: WAL consistency check facility

    Robert Haas <robertmhaas@gmail.com> — 2017-02-14T23:22:12Z

    On Tue, Feb 14, 2017 at 5:16 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Feb 15, 2017 at 2:43 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Thu, Feb 9, 2017 at 8:17 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> Please find attached a patch with those fixes.
>>
>> Committed, but I changed the copyright dates to 2016-2017 rather than
>> just 2017 since surely some of the code was originally written before
>> 2017.  Even that might not really be going back far enough, but it
>> doesn't matter too much.
>
> Just for curiosity: does the moment when the code has been written or
> committed counts? It's no big deal seeing how liberal the Postgres
> license is, but this makes me wonder...

IANAL, but I think if you ask one, he or she will tell you that what
matters is the date the work was created.  In the case of code, that
means when the code was written.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  30. Re: WAL consistency check facility

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-02-15T00:12:53Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Feb 14, 2017 at 5:16 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> Just for curiosity: does the moment when the code has been written or
>> committed counts? It's no big deal seeing how liberal the Postgres
>> license is, but this makes me wonder...

> IANAL, but I think if you ask one, he or she will tell you that what
> matters is the date the work was created.  In the case of code, that
> means when the code was written.

FWIW, my own habit when creating new PG files is generally to write

 * Portions Copyright (c) 1996-2017, PostgreSQL Global Development Group
 * Portions Copyright (c) 1994, Regents of the University of California

even if it's "all new" code.  The main reason being that it's hardly ever
the case that you didn't copy-and-paste some amount of stuff out of a
pre-existing file, and trying to sort out how much of what originated
exactly when is an unrewarding exercise.  Even if it is basically all
new code, this feels like giving an appropriate amount of credit to
Those Who Went Before Us.

			regards, tom lane

  31. Re: WAL consistency check facility

    Robert Haas <robertmhaas@gmail.com> — 2017-02-15T02:08:59Z

    On Tue, Feb 14, 2017 at 7:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Feb 14, 2017 at 5:16 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> Just for curiosity: does the moment when the code has been written or
>>> committed counts? It's no big deal seeing how liberal the Postgres
>>> license is, but this makes me wonder...
>
>> IANAL, but I think if you ask one, he or she will tell you that what
>> matters is the date the work was created.  In the case of code, that
>> means when the code was written.
>
> FWIW, my own habit when creating new PG files is generally to write
>
>  * Portions Copyright (c) 1996-2017, PostgreSQL Global Development Group
>  * Portions Copyright (c) 1994, Regents of the University of California
>
> even if it's "all new" code.  The main reason being that it's hardly ever
> the case that you didn't copy-and-paste some amount of stuff out of a
> pre-existing file, and trying to sort out how much of what originated
> exactly when is an unrewarding exercise.  Even if it is basically all
> new code, this feels like giving an appropriate amount of credit to
> Those Who Went Before Us.

Right.  I tend to do the same, and wonder if we shouldn't make that a
general practice.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  32. Re: WAL consistency check facility

    Michael Paquier <michael.paquier@gmail.com> — 2017-02-15T02:16:09Z

    On Wed, Feb 15, 2017 at 11:08 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Feb 14, 2017 at 7:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> FWIW, my own habit when creating new PG files is generally to write
>>
>>  * Portions Copyright (c) 1996-2017, PostgreSQL Global Development Group
>>  * Portions Copyright (c) 1994, Regents of the University of California
>>
>> even if it's "all new" code.  The main reason being that it's hardly ever
>> the case that you didn't copy-and-paste some amount of stuff out of a
>> pre-existing file, and trying to sort out how much of what originated
>> exactly when is an unrewarding exercise.  Even if it is basically all
>> new code, this feels like giving an appropriate amount of credit to
>> Those Who Went Before Us.
>
> Right.  I tend to do the same, and wonder if we shouldn't make that a
> general practice.

This looks sensible to me. No-brainer rules that make sense are less
things to worry about.
-- 
Michael