On Fri, Oct 28, 2016 at 11:35 AM, Michael Paquier <michael.paqu...@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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers