Re: [HACKERS] WAL consistency check facility

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