Re: Freeze avoidance of very large table.

On Tue, Jul 7, 2015 at 5:19 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Jul 2, 2015 at 9:00 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
wrote:
> >
> >
> > Thank you for bug report, and comments.
> >
> > Fixed version is attached, and source code comment is also updated.
> > Please review it.
> >
>
> I am looking into this patch and would like to share my findings with
> you:
>

Few more comments:

@@ -47,6 +47,8 @@ CATALOG(pg_class,1259) BKI_BOOTSTRAP BKI_ROWTYPE_OID(83)
BKI_SCHEMA_MACRO
float4 reltuples; /* # of tuples (not always up-to-date) */
int32 relallvisible; /* # of all-visible blocks (not always
* up-to-date) */
+ int32 relallfrozen; /* # of all-frozen blocks (not always
+ up-to-date) */

You have added relallfrozen similar to relallvisible, but how you
are planning to use it, is there any usecase for it?

lazy_scan_heap()
..
- /* Current block is all-visible */
+ /*
+ * Current block is all-visible.
+ * If visibility map represents that it's all frozen, we can
+ * skip to vacuum page unconditionally.
+ */
+ if (visibilitymap_test(onerel, blkno, &vmbuffer,
VISIBILITYMAP_ALL_FROZEN))
+ {
+ vacrelstats->vmskipped_pages++;
+ continue;
+ }
+

a. please explain in comment why it is safe if someone clear the
frozen bit concurrently
b. won't skipping pages intermittently due to set frozen bit break the
readahead mechanism? In this regard, if possible, I think we should
do some tests to see the benefit of this patch. I understand that in
general, it will be good to skip pages, however it seems better to check
that with some different kind of tests.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com