On Fri, Nov 4, 2016 at 1:32 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Thank you for the new patch. This will be hopefully the last round of
> reviews, we are getting close to something that has an acceptable
> shape.
Thanks a lot for reviewing the patch. Based on your review, I've attached the
updated patch along with few comments.
> In DecodeXLogRecord@xlogreader.c, please add a boolean flag "apply"
> and then please could you do some error checks on it. Only one is
> needed: if "apply" is true and has_image is false, xlogreader.c should
> complain about an inconsistency!
Added a flag named apply_image in DecodedBkpBlock and used it to
check whether image apply is required or not.
> I would still for the removal of blkno in the list of arguments of the
> masking functions. This is used just for speculative inserts, where we
> could just enforce the page number to 0 because this does not matter,
> as Peter has mentioned upthread.
It just doesn't feel right to me to enforce the number manually when
I can use the blkno without any harm.
> I haven't performed any tests with the patch, and that's all I have
> regarding the code. With that done we should be in good shape
> code-speaking I think...
I've done a fair amount of testing which includes regression tests
and manual creation of inconsistencies in the page. I've also found the
reason behind inconsistency in brin revmap page.
Brin revmap page doesn't have standard page layout. Besides, It doesn't update
pd_upper and pd_lower in its operations as well. But, during WAL
insertions, it uses
REGBUF_STANDARD to register a reference in the WAL record. Hence, when we
restore image before consistency check, RestoreBlockImage fills the space
between pd_upper and pd_lower(page hole) with zero. I've posted this as a
separate thread.
https://www.postgresql.org/message-id/flat/CAGz5QCJ%3D00UQjScSEFbV%3D0qO5ShTZB9WWz_Fm7%2BWd83zPs9Geg%40mail.gmail.com#CAGz5QCJ=00UQjScSEFbV=0qO5ShTZB9WWz_Fm7+Wd83zPs9Geg@mail.gmail.com
--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com