Re: WAL consistency check facility

On Thu, Nov 3, 2016 at 3:24 PM, Kuntal Ghosh <kuntalghosh(dot)2007(at)gmail(dot)com> wrote:
> On Thu, Nov 3, 2016 at 2:35 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> - Another suggestion was to remove wal_consistency from PostgresNode.pm
>>> because small buildfarm machines may suffer on it. Although I've no
>>> experience in this matter, I would like to be certain that nothings breaks
>>> in recovery tests after some modifications.
>>
>> An extra idea that I have here would be to extend the TAP tests to
>> accept an environment variable that would be used to specify extra
>> options when starting Postgres instances. Buildfarm machines could use
>> it.
>>
> It can be added as a separate feature.
>
>>
>> - /* If it's a full-page image, restore it. */
>> - if (XLogRecHasBlockImage(record, block_id))
>> + /* If full-page image should be restored, do it. */
>> + if (XLogRecBlockImageApply(record, block_id))
>> Hm. It seems to me that this modification is incorrect. If the page
>> does not need to be applied, aka if it needs to be used for
>> consistency checks, what should be done is more something like the
>> following in XLogReadBufferForRedoExtended:
>> if (Apply(record, block_id))
>> return;
>> if (HasImage)
>> {
>> //do what needs to be done with an image
>> }
>> else
>> {
>> //do default things
>> }
>>
> XLogReadBufferForRedoExtended should return a redo action
> (block restored, done, block needs redo or block not found). So, we
> can't just return
> from the function if BLKIMAGE_APPLY flag is not set. It still has to
> check whether a
> redo is required or not.

Wouldn't the definition of a new redo action make sense then? Say
SKIPPED. None of the existing actions match the non-apply case.
--
Michael