On Mon, Mar 22, 2021 at 6:40 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > I've looked at this 0001 patch and here are some review comments:
> + * Since we might have to prune a second time here, the code is structured to > + * use a local per-page copy of the counters that caller accumulates. We add > + * our per-page counters to the per-VACUUM totals from caller last of all, to > + * avoid double counting. > > Those comments should be a part of 0002 patch? Right -- will fix. > pc.num_tuples is incremented twice. ps->hastup = true is also duplicated. Must have been a mistake when splitting the patch up -- will fix. > --- > In step 7, with the patch, we save the freespace of the page and do > lazy_vacuum_page(). But should it be done in reverse? > How about renaming to vacuum_two_pass_strategy() or something to clear > this function is used to vacuum? Okay. I will rename it to lazy_vacuum_pruned_items(). > vacrelstats->dead_tuples->num_tuples))); > > It seems that the comment needs to be updated. Will fix. > I’ll review the other two patches tomorrow. And I'll respond to your remarks on those (which are already posted now) separately. > I didn't look at the 0002 patch in-depth but the main difference > between those two WAL records is that XLOG_HEAP2_PRUNE has the offset > numbers of unused, redirected, and dead whereas XLOG_HEAP2_VACUUM has > only the offset numbers of unused? That's one difference. Another difference is that there is no latestRemovedXid field. And there is a third difference: we no longer need a super-exclusive lock for heap page vacuuming (not pruning) with this design -- which also means that we cannot defragment the page during heap vacuuming (that's unsafe with only an exclusive lock because it physically relocates tuples with storage that somebody else may have a C pointer to that they expect to stay sane). These differences during original execution of heap page vacuum necessitate inventing a new REDO routine that does things in exactly the same way. To put it another way, heap vacuuming is now very similar to index vacuuming (both are dissimilar to heap pruning). They're very simple, and 100% a matter of freeing space in physical data structures. Clearly that's always something that we can put off if it makes sense to do so. That high level simplicity seems important to me. I always disliked the way the WAL records for vacuumlazy.c worked. Especially the XLOG_HEAP2_CLEANUP_INFO record -- that one is terrible. -- Peter Geoghegan
