Re: New vacuum option to do only freezing

On Wed, Feb 27, 2019 at 10:02 AM Bossart, Nathan <bossartn(at)amazon(dot)com> wrote:
>
> 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(dot)mshk(at)gmail(dot)com> wrote:
> > On Fri, Feb 1, 2019 at 11:43 PM Bossart, Nathan <bossartn(at)amazon(dot)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.

Thank you for reviewing the patch!

>
> + 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?

Agreed. Will add a comment.

>
> + 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.

I think that tups_vacuumed shows total number of vacuumed tuples and
is already shown in the log message. The 'nleft' counts the total
number of recorded dead tuple but not counts tuples are removed during
HOT-pruning. Is this a valuable for users in non-DISABLE_INDEX_CLEANUP
case?

>
> + /*
> + * 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.

Hmm, I think it's true that we leave 'nleft' dead tuples because it
includes both tuples marked as dead and tuples not marked yet. So I
think the log message works. Maybe the comment leads misreading. Will
fix it.

>
> + /*
> + * 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?
>

Yeah, I think you're right. Will fix.

> + 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/
>

Will fix.

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

Will fix.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center