Re: Logging WAL when updating hintbit

On 12/13/2013 07:55 AM, Sawada Masahiko wrote:
> On Fri, Dec 13, 2013 at 1:51 PM, Dilip kumar <dilip.kumar@huawei.com> wrote:
>> On 04 December 2013, Sawada Masahiko Wrote
>>
>>> I attached the patch which have modified based on Robert suggestion,
>>> and fixed typo.
>>
>> I have reviewed the modified patch and I have some comments..
>>
>> 1. Patch need to be rebased (failed applying on head)
>>
>> 2. crc field should be at end in ControlFileData struct, because for crc calculation, it directly take the offset of
crcfield in ControlFileData.
 
>>
>>          /* CRC of all above ... MUST BE LAST! */
>>          pg_crc32        crc;
>> +
>> +       /* Enable logging WAL when updating hint bits */
>> +       bool            wal_log_hintbits;
>> } ControlFileData;
>> 3. wal_log_hintbits field should be printed in PrintControlValues function.

Actually, no. It's reset anyway like wal_level and some other settings, 
so it's not an interesting value to print out.

Thanks for the review!

> I have modified the patch base on your comment, and I attached the v7 patch.

Thanks, committed with some minor changes:

The amount of extra WAL-logging with wal_log_hintbits=on is almost, but 
not exactly the same as with checksums enabled. With checksums enabled, 
visibilitymap_set() creates a full-page image of the heap page, but with 
wal_log_hintbits=on, it does not. For pg_rewind's purposes, that's 
correct, but I feel that it would be better if the decision on whether 
to WAL-log or not was exactly the same in both cases. One reason is that 
you could then use wal_log_hintbits=on to evaluate how big the impact on 
WAL volume would be if you had checksums enabled. I committed it that way.

OTOH, for pg_rewind's purposes, there's actually no need to take a full 
page image when a hint bit is set. A small WAL-record indicating that 
the page was modified would be enough. It's particularly strange to 
insist that hint bit updates create full-page images, when you have 
full_page_writes=off so that normal updates don't create them.

We're a bit schizophrenic with full page writes also when data checksums 
are enabled, though. If I'm reading the code correctly, you can turn 
full_page_writes=off even when data checksums are enabled, which exposes 
you to torn page problems. Which might be OK under some special 
circumstances. But you'll still get full-page images of hint bits!

I think it's a bit excessive to require wal_level > minimal if you set 
wal_log_hintbits=on. It's a bit silly to ask for wal_log_hintbits=on 
without archiving, but I also don't see a big reason to throw an error. 
It would be annoying, if you want to e.g temporarily set 
wal_level=minimal when you do a big data load, and re-initialize the 
standby afterwards. Now you'd also need to remember to turn 
wal_log_hintbits=off temporarily, and remember to turn it back on 
afterwards. So I just removed that check.

I'm not totally satisfied with the name of the GUC, wal_log_hintbits. 
The term "hint bits" doesn't mean anything to most people. I couldn't 
come up with anything better, but if someone has a better suggestion, we 
can still change it.

- Heikki