Re: WAL consistency check facility

On Fri, Sep 9, 2016 at 4:01 PM, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:
>>> - If WAL consistency check is enabled for a rmgrID, we always include
>>> the backup image in the WAL record.
>>
>> What happens if wal_consistency has different settings on a standby
>> and its master? If for example it is set to 'all' on the standby, and
>> 'none' on the master, or vice-versa, how do things react? An update of
>> this parameter should be WAL-logged, no?
>>
> It is possible to set wal_consistency to 'All' in master and any other
> values in standby. But, the scenario you mentioned will cause error in
> standby since it may not get the required backup image for wal
> consistency check. I think that user should be responsible to set
> this value correctly. We can improve the error message to make the
> user aware of the situation.

Let's be careful here. You should as well consider things from the
angle that some parameter updates are WAL-logged as well, like
wal_level with the WAL record XLOG_PARAMETER_CHANGE.

>>> - In recovery tests (src/test/recovery/t), I've added wal_consistency
>>> parameter in the existing scripts. This feature doesn't change the
>>> expected output. If there is any inconsistency, it can be verified in
>>> corresponding log file.
>>
>> I am afraid that just generating a WARNING message is going to be
>> useless for the buildfarm. If we want to detect errors, we could for
>> example have an additional GUC to trigger an ERROR or a FATAL, taking
>> down the cluster, and allowing things to show in red on a platform.
>>
> Yes, we can include an additional GUC to trigger an ERROR for any inconsistency.

I'd like to hear extra opinions about that, but IMO just having an
ERROR would be fine for the first implementation. Once you've bumped
into an ERROR, you are likely going to fix it first.

>> +           /*
>> +            * Followings are the rmids which can have backup blocks.
>> +            * We'll enable this feature only for these rmids.
>> +            */
>> +           newwalconsistency[RM_HEAP2_ID] = true;
>> +           newwalconsistency[RM_HEAP_ID] = true;
>> +           newwalconsistency[RM_BTREE_ID] = true;
>> +           newwalconsistency[RM_HASH_ID] = true;
>> +           newwalconsistency[RM_GIN_ID] = true;
>> +           newwalconsistency[RM_GIST_ID] = true;
>> +           newwalconsistency[RM_SEQ_ID] = true;
>> +           newwalconsistency[RM_SPGIST_ID] = true;
>> +           newwalconsistency[RM_BRIN_ID] = true;
>> +           newwalconsistency[RM_GENERIC_ID] = true;
>> +           newwalconsistency[RM_XLOG_ID] = true;
>> Here you can just use MemSet with RM_MAX_ID and simplify this code maintenance.
>>
> Not all rmids can have backup blocks. So, for wal_consistency = 'all',
> I've enabled only those rmids which can have backup blocks.

Even if some rmgrs do not support FPWs, I don't think that it is safe
to assume that the existing ones would never support it. Imagine for
example that feature X is implemented. Feature X adds rmgs Y, but rmgr
Y does not use FPWs. At a later point a new feature is added, which
makes rmgr Y using FPWs. We'd increase the number of places to update
with your patch, increasing the likelyness to introduce bugs. It would
be better to use a safe implementation from the maintenance point of
view to be honest (maintenance load of masking functions is somewhat
leveraged by the fact that on-disk format is kept compatible).

>>
>> +       if (pg_strcasecmp(tok, "Heap2") == 0)
>> +       {
>> +           newwalconsistency[RM_HEAP2_ID] = true;
>> +       }
>> Thinking more about it, I guess that we had better change the
>> definition list of rmgrs in rmgr.h and get something closer to
>> RmgrDescData that pg_xlogdump has to avoid all this stanza by
>> completing it with the name of the rmgr. The only special cases that
>> this code path would need to take care of would be then 'none' and
>> 'all'. You could do this refactoring on top of the main patch to
>> simplify it as it is rather big (1.7k lines).
>>
> I'm not sure about this point. wal_consistency doesn't support  all
> the rmids. We should have some way to check this.

I'd rather see this code done in such a way that all the rmgrs can be
handled, this approach being particularly attractive for the fact that
there is no need to change it if new rmgrs are added in the future.
(This was a reason as well why I still think that a simple on/off
switch would be plain enough, users have mostly control of the SQLs
triggering WAL. And if you run tests, you'll likely have the mind to
turn autovacuum to off to avoid it to generate FPWs and pollute the
logs at least at the second run of your tests).

And if you move forward with the approach of making this parameter a
list, I think that it would be better to add a section in the WAL
documentation about resource managers, like what they are, and list
them in this section of the docs. Then your parameter could link to
this documentation part and users would be able to see what kind of
values can be set. This leverages the need to update multiple portions
of the docs if rmgrs are added or removed in the future, as well as it
minimizes the maintenance of this code.
-- 
Michael