|From:||Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>|
|To:||Robert Haas <robertmhaas(at)gmail(dot)com>|
|Cc:||Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Bruce Momjian <bruce(at)momjian(dot)us>, Jeff Janes <jeff(dot)janes(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Josh Berkus <josh(at)agliodbs(dot)com>, Masao Fujii <masao(dot)fujii(at)gmail(dot)com>, Petr Jelinek <petr(at)2ndquadrant(dot)com>, Greg S <stark(at)mit(dot)edu>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Freeze avoidance of very large table.|
|Views:||Raw Message | Whole Thread | Download mbox|
On Fri, Feb 12, 2016 at 4:46 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Feb 3, 2016 at 12:32 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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
> - 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) &&
> 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
> 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.
|Next Message||Michael Paquier||2016-02-14 05:59:26||Re: extend pgbench expressions with functions|
|Previous Message||Pavel Stehule||2016-02-14 03:28:53||Re: proposal: function parse_ident|