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(dot)kumar(at)huawei(dot)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 crc field 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