On Fri, Feb 12, 2016 at 4:46 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Feb 3, 2016 at 12:32 PM, Masahiko Sawada <sawada.mshk@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);
>
Thank you for reviewing this patch.
I've divided 000 patch into two patches, and attached latest 4 patches in total.
I changed pg_upgrade plugin logic so that all kind of type suffix has
one convert plugin.
The type suffix which doesn't need to be converted has pg_copy_file()
function as plugin function.
Regards,
--
Masahiko Sawada