Re: [HACKERS] WAL consistency check facility

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