Re: Freeze avoidance of very large table.

On Wed, Dec 2, 2015 at 9:30 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.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