On Wed, Feb 3, 2016 at 12:32 PM, Masahiko Sawada <sawada.m...@gmail.com> wrote: > I've divided the main patch into two patches; add frozen bit patch and > pg_upgrade support patch. > 000 patch is almost same as previous code. (includes small fix) > 001 patch provides rewriting visibility map as a pageConverter routine. > 002 patch is for enhancement debug message in visibilitymap.c
I'd like to suggest splitting 000 into two patches. The first one would change the format of the visibility map, and the second one would change VACUUM to optimize scans based on the new format. I think that would make it easier to get this reviewed and committed. I think this patch churns a bunch of things that don't really need to be churned. For example, consider this hunk: /* * If we didn't pin the visibility map page and the page has become all - * visible while we were busy locking the buffer, we'll have to unlock and - * re-lock, to avoid holding the buffer lock across an I/O. That's a bit - * unfortunate, but hopefully shouldn't happen often. + * visible or all frozen while we were busy locking the buffer, we'll + * have to unlock and re-lock, to avoid holding the buffer lock across an + * I/O. That's a bit unfortunate, but hopefully shouldn't happen often. */ Since the page can't become all-frozen without also becoming all-visible, the original text is still 100% accurate, and the change doesn't seem to add any useful clarity. Let's think about which things really need to be changed and not just mechanically change everything. - Assert(PageIsAllVisible(heapPage)); + /* + * Caller is expected to set PD_ALL_VISIBLE or + * PD_ALL_FROZEN first. + */ + Assert(((flags | VISIBILITYMAP_ALL_VISIBLE) && PageIsAllVisible(heapPage)) || + ((flags | VISIBILITYMAP_ALL_FROZEN) && PageIsAllFrozen(heapPage))); I think this would be more clear as two separate assertions. Your 000 patch has a little bit of whitespace damage: [rhaas pgsql]$ git diff --check src/backend/commands/vacuumlazy.c:1951: indent with spaces. + bool *all_visible, bool *all_frozen) src/include/access/heapam_xlog.h:393: indent with spaces. + Buffer vm_buffer, TransactionId cutoff_xid, uint8 flags); -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers