On Fri, Oct 28, 2016 at 11:35 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> And here we go. Here is a review as well as a large brush-up for this
> patch. A couple of things:
Thanks for reviewing the patch in detail.
> - Speaking of which using BKPIMAGE_IS_REQUIRED_FOR_REDO stored in the
> block definition is sort of weird because we want to know if
> consistency should be checked at a higher level.
A full page image can be included in the WAL record because of the following
reasons:
1. It needs to be restored during replay.
2. WAL consistency should be checked for the record.
3. Both of above.
In your patch, you've included a full page image whenever wal_consistency
is true. So, XLogReadBufferForRedoExtended always restores the image
and returns BLK_RESTORED, which is unacceptable. We can't change
the default WAL replay behaviour. A full image should only be restored if it is
necessary to do so. Although, I agree that BKPIMAGE_IS_REQUIRED_FOR_REDO
doesn't look a clean way to implement this feature.
> - wal_consistency is using a list of RMGRs, at the cost of being
> PGC_POSTMASTER. I'd suggest making it PGC_SUSER, and use a boolean (I
> have been thinking hard about that, and still I don't see the point).
> It is rather easy to for example default it to false, and enable it to
> true to check if a certain code path is correctly exercised or not for
> WAL consistency. Note that this simplification reduces the patch size
> by 100~150 lines. I know, I know, I'd expect some complains about
> that....
As Robert also told, if I'm focusing on a single AM, I really don't
want to store
full images and perform consistency check for other AMs.
> On top of that, I have done a fair amount of testing, creating
> manually some inconsistencies in the REDO routines to trigger failures
> on standbys. And that was sort of fun to break things intentionally.
I know the feeling. :)
--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com
