Re: WAL consistency check facility

On Fri, Oct 28, 2016 at 2:05 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> And here we go. Here is a review as well as a large brush-up for this
> patch. A couple of things:
> - 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....

I don't understand how you can fail to see the point of that. As you
yourself said, this facility generates a ton of WAL. If you're
focusing on one AM, why would you want to be forced to incur the
overhead for every other AM? A good deal has been written about this
upthread already, and just saying "I don't see the point" seems to be
ignoring the explanations already given.

> - Looking for wal_consistency at replay has no real value. What if on
> a standby the parameter value is inconsistent than the one on the
> master? This logic adds a whole new level of complications and
> potential bugs. So instead my suggestion is to add a marker at WAL
> record level to check if this record should be checked for consistency
> at replay or not.

Agreed.

> This is also quite flexible if you think about it,
> the standby is made independent of the WAL generated on the master and
> just applies, or checks what it sees is fit checking for.

+1.

> The best
> match here is to add a flag for XLogRecord->xl_info and make use of
> one of the low 4 bits and only one is used now for
> XLR_SPECIAL_REL_UPDATE.

Seems reasonable.

> - in maskPage, the new rmgr routine, there is no need for the info and
> blkno arguments. info is not used at all to begin with. blkno is used
> for gin pages to detect meta pages but this can be guessed using the
> opaque pointer. For heap pages and speculative inserts, masking the
> blkno would be fine. That's not worth it.

Passing the blkno doesn't cost anything. If it avoids guessing,
that's entirely worth it.

> - Instead of palloc'ing the old and new pages to compare, it would be
> more performant to keep around two static buffers worth of BLCKSZ and
> just use that. This way there is no need as well to perform any palloc
> calls in the masking functions, limiting the risk of errors (those
> code paths had better avoid errors IMO). It would be also less costly
> to just pass to the masking function a pointer to a buffer of size
> BLCKSZ and just do the masking on it.

We always palloc buffers like this so that they will be aligned. But
we could arrange not to repeat the palloc every time (see, e.g.,
BootstrapXLOG()).

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company