On 12/13/2013 07:55 AM, Sawada Masahiko wrote:
On Fri, Dec 13, 2013 at 1:51 PM, Dilip kumar <dilip.ku...@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 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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to