Re: Freeze avoidance of very large table.

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.


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.

 In visibilitymap_set, the following lines does something different from what to do.  Only right side of the inequality
getsshifted 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))


analyze.c:
In do_analyze_rel, the successive if (!inh) in the followingsteps looks a bit odd. This would be emphasized by the
firstifblock 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); 
 

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


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

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))


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.

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

pg_upgrade.c:
BITS_PER_HEAPBLOCK is defined in two c files with the samedefinition. This might be better to be merged into some
headerfile.


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

regards,


At Wed, 2 Dec 2015 00:10:09 +0530, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoC72S2ShoeAmCxWYUyGSNOaTn4fMHJ-ZKNX-MPcsQpaOw@mail.gmail.com>
> On Tue, Dec 1, 2015 at 3:04 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
> > On Mon, Nov 30, 2015 at 9:18 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > After running pg_upgrade, if I manually vacuum a table a start getting warnings:
> >
> > WARNING:  page is not marked all-visible (and all-frozen) but
> > visibility map bit(s) is set in relation "foo" page 32756
> > WARNING:  page is not marked all-visible (and all-frozen) but
> > visibility map bit(s) is set in relation "foo" page 32756
...
> > The warnings are right where the blocks would start using the 2nd page
> > of the _vm, so I think the problem is there.  And looking at the code,
> > I think that "cur += SizeOfPageHeaderData;" in the inner loop cannot
> > be correct.  We can't skip a header in the current (old) block each
> > time we reach the end of the new block.  The thing we are skipping in
> > the current block is half the time not a header, but the data at the
> > halfway point through the block.
> >
> 
> Thank you for reviewing.
> 
> You're right, it's not necessary.
> Attached latest v29 patch which removes the mention in pg_upgrade documentation.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center