Re: vacuum verbose: show pages marked allvisible/frozen/hintbits

On Sun, 26 Jan 2020 at 23:13, Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> I'm forking this thread since it's separate topic, and since keeping in a
> single branch hasn't made maintaining the patches any easier.
> https://www.postgresql.org/message-id/CAMkU%3D1xAyWnwnLGORBOD%3Dpyv%3DccEkDi%3DwKeyhwF%3DgtB7QxLBwQ%40mail.gmail.com
> On Sun, Dec 29, 2019 at 01:15:24PM -0500, Jeff Janes wrote:
> > Also, I'd appreciate a report on how many hint-bits were set, and how many
> > pages were marked all-visible and/or frozen. When I do a manual vacuum, it
> > is more often for those purposes than it is for removing removable rows
> > (which autovac generally does a good enough job of).
>
> The first patch seems simple enough but the 2nd could use critical review.

Here is comments on 0001 patch from a quick review:

- BlockNumber pages_removed;
+ BlockNumber pages_removed; /* Due to truncation */
+ BlockNumber pages_allvisible;
+ BlockNumber pages_frozen;

Other codes in vacuumlazy.c uses ‘all_frozen', so how about
pages_allfrozen instead of pages_frozen?

---
@@ -1549,8 +1558,12 @@ lazy_scan_heap(Relation onerel, VacuumParams
*params, LVRelStats *vacrelstats,
{
uint8 flags = VISIBILITYMAP_ALL_VISIBLE;

- if (all_frozen)
+ if (all_frozen) {
flags |= VISIBILITYMAP_ALL_FROZEN;
+ vacrelstats->pages_frozen++;
+ }

@@ -1979,10 +2000,14 @@ lazy_vacuum_page(Relation onerel, BlockNumber
blkno, Buffer buffer,
uint8 flags = 0;

/* Set the VM all-frozen bit to flag, if needed */
- if ((vm_status & VISIBILITYMAP_ALL_VISIBLE) == 0)
+ if ((vm_status & VISIBILITYMAP_ALL_VISIBLE) == 0) {
flags |= VISIBILITYMAP_ALL_VISIBLE;
- if ((vm_status & VISIBILITYMAP_ALL_FROZEN) == 0 && all_frozen)
+ vacrelstats->pages_allvisible++;
+ }
+ if ((vm_status & VISIBILITYMAP_ALL_FROZEN) == 0 && all_frozen) {
flags |= VISIBILITYMAP_ALL_FROZEN;
+ vacrelstats->pages_frozen++;
+ }

The above changes need to follow PostgreSQL code format (a newline is
required after if statement).

---
/*
* If the all-visible page is all-frozen but not marked as such yet,
* mark it as all-frozen. Note that all_frozen is only valid if
* all_visible is true, so we must check both.
*/
else if (all_visible_according_to_vm && all_visible && all_frozen &&
!VM_ALL_FROZEN(onerel, blkno, &vmbuffer))
{
/*
* We can pass InvalidTransactionId as the cutoff XID here,
* because setting the all-frozen bit doesn't cause recovery
* conflicts.
*/
visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
vmbuffer, InvalidTransactionId,
VISIBILITYMAP_ALL_FROZEN);
}

We should also count up vacrelstats->pages_frozen here.

For 0002 patch, how users will be able to make any meaning out of how
many hint bits were updated by vacuumu?

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services