On Fri, Jan 20, 2012 at 12:54 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> Thanks for the review!
>
> On Fri, Jan 20, 2012 at 8:15 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> I'm looking at this patch and wondering why we're doing so many
>> press-ups to ensure full_page_writes parameter is on. This will still
>> fail if you use a utility that removes the full page writes, but fail
>> silently.
>>
>> I think it would be beneficial to explicitly check that all WAL
>> records have full page writes actually attached to them until we
>> achieve consistency.
>
> I agree that it's worth adding such a safeguard. That can be a self-contained
> feature, so I'll submit a separate patch for that, to keep each patch small.
Maybe, but you mean do this now as well? Not sure I like silent errors.
>> Surprised to see XLOG_FPW_CHANGE is there again after I objected to it
>> and it was removed. Not sure why? We already track other parameters
>> when they change, so I don't want to introduce a whole new WAL record
>> for each new parameter whose change needs tracking.
>
> I revived that because whenever full_page_writes must be WAL-logged
> or replayed, there is no need to WAL-log or replay the HS parameters.
> The opposite is also true. Logging or replaying all of them every time
> seems to be a bit useless, and to make the code unreadable. ISTM that
> XLOG_FPW_CHANGE can make the code simpler and avoid adding useless
> WAL activity by merging them into one WAL record.
I don't agree, but for the sake of getting on with things I say this
is minor so is no reason to block this.
>> Please make a note for committer that wal version needs bumping.
>
> Okay, will add the note about bumping XLOG_PAGE_MAGIC.
>
>> I think its probably time to start a README.recovery to explain why
>> this works the way it does. Other changes can then start to do that as
>> well, so we can keep this to sane levels of complexity.
>
> In this CF, there are other patches which change recovery codes. So
> I think that it's better to do that after all of them will have been committed.
> No need to hurry up to do that now.
Agreed.
Will proceed to final review and if all OK, commit.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
