Re: vacuum verbose: show pages marked allvisible/frozen/hintbits - Mailing list pgsql-hackers

On Sun, 26 Jan 2020 at 23:13, Justin Pryzby <pryzby@telsasoft.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