Re: New vacuum option to do only freezing

Sorry for the delay.  I finally got a chance to look through the
latest patches.

On 2/3/19, 1:48 PM, "Masahiko Sawada" <sawada.mshk@gmail.com> wrote:
> On Fri, Feb 1, 2019 at 11:43 PM Bossart, Nathan <bossartn@amazon.com> wrote:
>>
>> +               if (skip_index_vacuum)
>> +                       ereport(elevel,
>> +                                       (errmsg("\"%s\": marked %.0f row versions as dead in %u pages",
>> +                                                       RelationGetRelationName(onerel),
>> +                                                       tups_vacuumed, vacuumed_pages)));
>>
>> IIUC tups_vacuumed will include tuples removed during HOT pruning, so
>> it could be inaccurate to say all of these tuples have only been
>> marked "dead."  Perhaps we could keep a separate count of tuples
>> removed via HOT pruning in case we are using DISABLE_INDEX_CLEANUP.
>> There might be similar problems with the values stored in vacrelstats
>> that are used at the end of heap_vacuum_rel() (e.g. tuples_deleted).
>
> Yeah, tups_vacuumed include such tuples so the message is inaccurate.
> So I think that we should not change the message but we can report the
> dead item pointers we left in errdetail. That's more accurate and
> would help user.
>
> postgres(1:1130)=# vacuum (verbose, disable_index_cleanup) test;
> INFO:  vacuuming "public.test"
> INFO:  "test": removed 49 row versions in 1 pages
> INFO:  "test": found 49 removable, 51 nonremovable row versions in 1
> out of 1 pages
> DETAIL:  0 dead row versions cannot be removed yet, oldest xmin: 509
> There were 0 unused item pointers.
> Skipped 0 pages due to buffer pins, 0 frozen pages.
> 0 pages are entirely empty.
> 49 tuples are left as dead.
> CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s.
> VACUUM

This seems reasonable to me.

The current version of the patches builds cleanly, passes 'make
check-world', and seems to work well in my own manual tests.  I have a
number of small suggestions, but I think this will be ready-for-
committer soon.

+               Assert(!skip_index_vacuum);

There are two places in lazy_scan_heap() that I see this without any
comment.  Can we add a short comment explaining why this should be
true at these points?

+       if (skip_index_vacuum)
+               appendStringInfo(&buf, ngettext("%.0f tuple is left as dead.\n",
+                                                                               "%.0f tuples are left as dead.\n",
+                                                                               nleft),
+                                                nleft);

I think we could emit this metric for all cases, not only when
DISABLE_INDEX_CLEANUP is used.

+                       /*
+                        * Remove tuples from heap if the table has no index.  If the table
+                        * has index but index vacuum is disabled, we don't vacuum and forget
+                        * them. The vacrelstats->dead_tuples could have tuples which became
+                        * dead after checked at HOT-pruning time which are handled by
+                        * lazy_vacuum_page() but we don't worry about handling those because
+                        * it's a very rare condition and these would not be a large number.
+                        */

Based on this, it sounds like nleft could be inaccurate.  Do you think
it is worth adjusting the log message to reflect that, or is this
discrepancy something we should just live with?  I think adding
something like "at least N tuples left marked dead" is arguably a bit
misleading, since the only time the number is actually higher is when
this "very rare condition" occurs.

+       /*
+        * Skip index vacuum if it's requested for table with indexes. In this
+        * case we use the one-pass strategy and don't remove tuple storage.
+        */
+       skip_index_vacuum =
+               (options & VACOPT_DISABLE_INDEX_CLEANUP) != 0 && vacrelstats->hasindex;

AFAICT we don't actually need to adjust this based on
vacrelstats->hasindex because we are already checking for indexes
everywhere we check for this option.  What do you think about leaving
that part out?

+                       if (vacopts->disable_index_cleanup)
+                       {
+                               /* DISABLE_PAGE_SKIPPING is supported since 12 */
+                               Assert(serverVersion >= 120000);
+                               appendPQExpBuffer(sql, "%sDISABLE_INDEX_CLEANUP", sep);
+                               sep = comma;
+                       }

s/DISABLE_PAGE_SKIPPING/DISABLE_INDEX_CLEANUP/

+       printf(_("      --disable-index-cleanup     disable index vacuuming and index clenaup\n"));

s/clenaup/cleanup/

Nathan