Re: Freeze avoidance of very large table.

On Wed, Dec 2, 2015 at 9:30 AM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello,
>
>> You're right, it's not necessary.
>> Attached latest v29 patch which removes the mention in pg_upgrade documentation.
>
> The changes looks to be correct but I haven't tested.
> And I have some additional random comments.
>

Thank you for revewing!
Fixed these following points, and attached latest patch.

> visibilitymap.c:
>
> In visibilitymap_set, the followint lines.
>
> map = PageGetContents(page);
> ...
> if (flags != (map[mapByte] & (flags << mapBit)))
>
> map is (char*), PageGetContents returns (char*) but flags is
> uint8. I think that defining map as (uint8*) would be safer.

I agree with you.
Fixed.

>
> In visibilitymap_set, the following lines does something
> different from what to do. Only right side of the inequality
> gets shifted and what should be used in right side is not flags
> but VISIBILITYMAP_VALID_BITS.
>
> - if (!(map[mapByte] & (1 << mapBit)))
> + if (flags != (map[mapByte] & (flags << mapBit)))
>
> Something like the following will do the right thing.
>
> + if (flags != (map[mapByte]>>mapBit & VISIBILITYMAP_VALID_BITS))
>

You're right.
Fixed.

> analyze.c:
>
> In do_analyze_rel, the successive if (!inh) in the following
> steps looks a bit odd. This would be emphasized by the first if
> block you added:) These blocks should be enclosed by if (!inh)
> {} block.
>
>
> > /* Calculate the number of all-visible and all-frozen bit */
> > if (!inh)
> > relallvisible = visibilitymap_count(onerel, &relallfrozen);
> > if (!inh)
> > vac_update_relstats(onerel,
> > if (!inh && !(options & VACOPT_VACUUM))
> > {
> > for (ind = 0; ind < nindexes; ind++)
> ...
> > }
> > if (!inh)
> > pgstat_report_analyze(onerel, totalrows, totaldeadrows, relallfrozen);

Fixed.

>
> vacuum.c:
>
> >- * relpages and relallvisible, we try to maintain certain lazily-updated
> >- * DDL flags such as relhasindex, by clearing them if no longer correct.
> >- * It's safe to do this in VACUUM, which can't run in parallel with
> >- * CREATE INDEX/RULE/TRIGGER and can't be part of a transaction block.
> >- * However, it's *not* safe to do it in an ANALYZE that's within an
>
> >+ * relpages, relallvisible, we try to maintain certain lazily-updated
>
> Why did you just drop the 'and' after relpages? And this seems
> the only change of this file except the additinally missing
> letter just below:p
>
> >+ * DDL flags such as relhasindex, by clearing them if no onger correct.
> >+ * It's safe to do this in VACUUM, which can't run in
> >+ * parallel with CREATE INDEX/RULE/TRIGGER and can't be part of a transaction
> >+ * block. However, it's *not* safe to do it in an ANALYZE that's within an

Fixed.

>
> nodeIndexonlyscan.c:
>
> A duplicate letters. And the line exceeds right margin.
>
> > - * Note on Memory Ordering Effects: visibilitymap_test does not lock
> -> + * Note on Memory Ordering Effects: visibilitymap_get_stattus does not lock
> + * Note on Memory Ordering Effects: visibilitymap_get_status does not lock

Fixed.

>
> The edited line exceeds right margin by removing a newline.
>
> - if (!visibilitymap_test(scandesc->heapRelation,
> - ItemPointerGetBlockNumber(tid),
> - &node->ioss_VMBuffer))
> + if (!VM_ALL_VISIBLE(scandesc->heapRelation, ItemPointerGetBlockNumber(tid),
> + &node->ioss_VMBuffer))
>

Fixed.

> costsize.c:
>
> Duplicate words and it is the only change.
>
> > - * pages for which the visibility map shows all tuples are visible.
> -> + * pages for which the visibility map map shows all tuples are visible.
> + * pages for which the visibility map shows all tuples are visible.

Fixed.

> pgstat.c:
>
> The new parameter frozenpages of pgstat_report_vacuum() is
> defined as int32, but its callers give BlockNumber(=uint32). I
> recommend to define the frozenpages as BlockNumber.
> PgStat_MsgVacuum has a corresponding member defined as int32.

I agree with you.
Fixed.

> pg_upgrade.c:
>
> BITS_PER_HEAPBLOCK is defined in two c files with the same
> definition. This might be better to be merged into some header
> file.

Fixed.
I moved these definition to visibilitymap.h.

>
> heapam_xlog.h, hio.h, execnodes.h:
>
> Have we decided to rename vm to pim? Anyway it is inconsistent
> with that of corresponding definition of the function body
> remains as 'vm_buffer'. (I'm not confident on that, though.)
>
> >- Buffer vm_buffer, TransactionId cutoff_xid);
> >+ Buffer pim_buffer, TransactionId cutoff_xid, uint8 flags);
>

Fixed.

Regards,

--
Masahiko Sawada