Re: Freeze avoidance of very large table.

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