New vacuum option to do only freezing

classic Classic list List threaded Threaded
113 messages Options
123456
Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Robert Haas
On Mon, Jan 14, 2019 at 9:24 PM Masahiko Sawada <[hidden email]> wrote:
> I think that because the tuples that got dead after heap_page_prune()
> looked are recorded but not removed without lazy_vacuum_page() we need
> to process them in lazy_vacuum_page(). For decision about whether to
> truncate we should not change it, so I will fix it. It should be an
> another option to control whether to truncate if we want.

I think that heap_page_prune() is going to truncate dead tuples to
dead line pointers. At that point the tuple is gone.  The only
remaining step is to mark the dead line pointer as unused, which can't
be done without scanning the indexes.  So I don't understand what
lazy_vacuum_page() would need to do in this case.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Masahiko Sawada
On Wed, Jan 16, 2019 at 5:24 AM Robert Haas <[hidden email]> wrote:

>
> On Mon, Jan 14, 2019 at 9:24 PM Masahiko Sawada <[hidden email]> wrote:
> > I think that because the tuples that got dead after heap_page_prune()
> > looked are recorded but not removed without lazy_vacuum_page() we need
> > to process them in lazy_vacuum_page(). For decision about whether to
> > truncate we should not change it, so I will fix it. It should be an
> > another option to control whether to truncate if we want.
>
> I think that heap_page_prune() is going to truncate dead tuples to
> dead line pointers. At that point the tuple is gone.  The only
> remaining step is to mark the dead line pointer as unused, which can't
> be done without scanning the indexes.  So I don't understand what
> lazy_vacuum_page() would need to do in this case.
>

vacuumlazy.c:L1022
            switch (HeapTupleSatisfiesVacuum(&tuple, OldestXmin, buf))
            {
                case HEAPTUPLE_DEAD:

                    /*
                     * Ordinarily, DEAD tuples would have been removed by
                     * heap_page_prune(), but it's possible that the tuple
                     * state changed since heap_page_prune() looked.  In
                     * particular an INSERT_IN_PROGRESS tuple could have
                     * changed to DEAD if the inserter aborted.  So this
                     * cannot be considered an error condition.
                     *
                     (snip)
                     */
                    if (HeapTupleIsHotUpdated(&tuple) ||
                        HeapTupleIsHeapOnly(&tuple))
                        nkeep += 1;
                    else
                        tupgone = true; /* we can delete the tuple */
                    all_visible = false;
                    break;

As the above comment says, it's possible that the state of an
INSERT_IN_PROGRESS tuple could be changed to 'dead' after
heap_page_prune(). Since such tuple is not truncated at this point we
record it and set it as UNUSED in lazy_vacuum_page(). I think that the
DISABLE_INDEX_CLEANUP case is the same; we need to process them after
recorded. Am I missing something?

Regards,

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

Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Robert Haas
On Wed, Jan 16, 2019 at 3:30 AM Masahiko Sawada <[hidden email]> wrote:
> As the above comment says, it's possible that the state of an
> INSERT_IN_PROGRESS tuple could be changed to 'dead' after
> heap_page_prune(). Since such tuple is not truncated at this point we
> record it and set it as UNUSED in lazy_vacuum_page(). I think that the
> DISABLE_INDEX_CLEANUP case is the same; we need to process them after
> recorded. Am I missing something?

I believe you are.  Think about it this way.  After the first pass
over the heap has been completed but before we've done anything to the
indexes, let alone started the second pass over the heap, somebody
could kill the vacuum process.  Somebody could in fact yank the plug
out of the wall, stopping the entire server in its tracks.  If they do
that, then lazy_vacuum_page() will never get executed.  Yet, the heap
can't be in any kind of corrupted state at this point, right?  We know
that the system is resilient against crashes, and killing a vacuum or
even the whole server midway through does not leave the system in any
kind of bad state.  If it's fine for lazy_vacuum_page() to never be
reached in that case, it must also be fine for it never to be reached
if we ask for vacuum to stop cleanly before lazy_vacuum_page().

In the case of the particular comment to which you are referring, that
comment is part of lazy_scan_heap(), not lazy_vacuum_page(), so I
don't see how it bears on the question of whether we need to call
lazy_vacuum_page().  It's true that, at any point in time, an
in-progress transaction could abort.  And if it does then some
insert-in-progress tuples could become dead.  But if that happens,
then the next vacuum will remove them, just as it will remove any
tuples that become dead for that reason when vacuum isn't running in
the first place.  You can't use that as a justification for needing a
second heap pass, because if it were, then you'd also need a THIRD
heap pass in case a transaction aborts after the second heap pass has
visited the pages, and a fourth heap pass in case a transaction aborts
after the third heap pass has visited the pages, etc. etc. forever.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Masahiko Sawada
On Thu, Jan 17, 2019 at 1:33 AM Robert Haas <[hidden email]> wrote:

>
> On Wed, Jan 16, 2019 at 3:30 AM Masahiko Sawada <[hidden email]> wrote:
> > As the above comment says, it's possible that the state of an
> > INSERT_IN_PROGRESS tuple could be changed to 'dead' after
> > heap_page_prune(). Since such tuple is not truncated at this point we
> > record it and set it as UNUSED in lazy_vacuum_page(). I think that the
> > DISABLE_INDEX_CLEANUP case is the same; we need to process them after
> > recorded. Am I missing something?
>
> I believe you are.  Think about it this way.  After the first pass
> over the heap has been completed but before we've done anything to the
> indexes, let alone started the second pass over the heap, somebody
> could kill the vacuum process.  Somebody could in fact yank the plug
> out of the wall, stopping the entire server in its tracks.  If they do
> that, then lazy_vacuum_page() will never get executed.  Yet, the heap
> can't be in any kind of corrupted state at this point, right?  We know
> that the system is resilient against crashes, and killing a vacuum or
> even the whole server midway through does not leave the system in any
> kind of bad state.  If it's fine for lazy_vacuum_page() to never be
> reached in that case, it must also be fine for it never to be reached
> if we ask for vacuum to stop cleanly before lazy_vacuum_page().
>

Yes, that's right. It doesn't necessarily need to be reached lazy_vacuum_page().

> In the case of the particular comment to which you are referring, that
> comment is part of lazy_scan_heap(), not lazy_vacuum_page(), so I
> don't see how it bears on the question of whether we need to call
> lazy_vacuum_page().  It's true that, at any point in time, an
> in-progress transaction could abort.  And if it does then some
> insert-in-progress tuples could become dead.  But if that happens,
> then the next vacuum will remove them, just as it will remove any
> tuples that become dead for that reason when vacuum isn't running in
> the first place.  You can't use that as a justification for needing a
> second heap pass, because if it were, then you'd also need a THIRD
> heap pass in case a transaction aborts after the second heap pass has
> visited the pages, and a fourth heap pass in case a transaction aborts
> after the third heap pass has visited the pages, etc. etc. forever.
>

The reason why I processed the tuples that became dead after the first
heap pass is that I was not sure the reason why we ignore such tuples
in the second heap pass despite of there already have been the code
doing so which has been used for a long time. I thought we can do that
in the same manner even in DISABLE_INDEX_CLEANUP case. Also, since I
thought that lazy_vacuum_page() is the best place to set them as DEAD
I modified it (In the previous patch I introduced another function
setting them as DEAD aside from lazy_vacuum_page(). But since these
were almost same I merged them).

However, as you mentioned it's true that these tuples will be removed
by the next vacuum even if we ignore. Also these tuples would not be
many and without this change the patch would get more simple. So if
the risk of this change is greater than the benefit, we should not do
that.

Regards,

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

Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Robert Haas
On Thu, Jan 17, 2019 at 1:57 AM Masahiko Sawada <[hidden email]> wrote:
> The reason why I processed the tuples that became dead after the first
> heap pass is that I was not sure the reason why we ignore such tuples
> in the second heap pass despite of there already have been the code
> doing so which has been used for a long time. I thought we can do that
> in the same manner even in DISABLE_INDEX_CLEANUP case. Also, since I
> thought that lazy_vacuum_page() is the best place to set them as DEAD
> I modified it (In the previous patch I introduced another function
> setting them as DEAD aside from lazy_vacuum_page(). But since these
> were almost same I merged them).

The race you're concerned about is extremely narrow.  We HOT-prune the
page, and then immediately afterward -- probably a few milliseconds
later -- we loop over the tuples still on the page and check the
status of each one.  The only time we get a different answer is when a
transaction aborts in those few milliseconds.  We don't worry about
handling those because it's a very rare condition.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Masahiko Sawada
On Sat, Jan 19, 2019 at 5:08 AM Robert Haas <[hidden email]> wrote:

>
> On Thu, Jan 17, 2019 at 1:57 AM Masahiko Sawada <[hidden email]> wrote:
> > The reason why I processed the tuples that became dead after the first
> > heap pass is that I was not sure the reason why we ignore such tuples
> > in the second heap pass despite of there already have been the code
> > doing so which has been used for a long time. I thought we can do that
> > in the same manner even in DISABLE_INDEX_CLEANUP case. Also, since I
> > thought that lazy_vacuum_page() is the best place to set them as DEAD
> > I modified it (In the previous patch I introduced another function
> > setting them as DEAD aside from lazy_vacuum_page(). But since these
> > were almost same I merged them).
>
> The race you're concerned about is extremely narrow.  We HOT-prune the
> page, and then immediately afterward -- probably a few milliseconds
> later -- we loop over the tuples still on the page and check the
> status of each one.  The only time we get a different answer is when a
> transaction aborts in those few milliseconds.  We don't worry about
> handling those because it's a very rare condition.
>
Understood and Agreed. I've attached the new version patch
incorporated the review comments.

Regards,

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

v5-0001-Add-DISABLE_INDEX_CLEANUP-option-to-VACUUM-comman.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Bossart, Nathan
On 1/21/19, 2:23 AM, "Masahiko Sawada" <[hidden email]> wrote:
> Understood and Agreed. I've attached the new version patch
> incorporated the review comments.

I took a look at the latest version of the patch.

+    <varlistentry>
+    <term><literal>DISABLE_INDEX_CLEANUP</literal></term>
+    <listitem>
+     <para>
+      <command>VACUUM</command> removes dead tuples and prunes HOT-updated
+      tuples chain for live tuples on table. If the table has any dead tuple
+      it removes them from both table and indexes for re-use. With this
+      option <command>VACUUM</command> doesn't completely remove dead tuples
+      and disables removing dead tuples from indexes.  This is suitable for
+      avoiding transaction ID wraparound but not sufficient for avoiding
+      index bloat. This option is ignored if the table doesn't have index.
+      Also, this cannot be used in conjunction with <literal>FULL</literal>
+      option.
+     </para>
+    </listitem>
+   </varlistentry>

IMHO we could document this feature at a slightly higher level without
leaving out any really important user-facing behavior.  Here's a quick
attempt to show what I am thinking:

        With this option, VACUUM skips all index cleanup behavior and
        only marks tuples as "dead" without reclaiming the storage.
        While this can help reclaim transaction IDs faster to avoid
        transaction ID wraparound (see Section 24.1.5), it will not
        reduce bloat.  Note that this option is ignored for tables
        that have no indexes.  Also, this option cannot be used in
        conjunction with the FULL option, since FULL requires
        rewriting the table.

+ /* Notify user that DISABLE_INDEX_CLEANUP option is ignored */
+ if (!vacrelstats->hasindex && (options & VACOPT_DISABLE_INDEX_CLEANUP))
+ ereport(NOTICE,
+ (errmsg("DISABLE_INDEX_CLEANUP is ignored because table \"%s\" does not have index",
+ RelationGetRelationName(onerel))));

It might make more sense to emit this in lazy_scan_heap() where we
determine the value of skip_index_vacuum.

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

I would suggest adding this option to vacuumdb in this patch set as
well.

Nathan

Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Michael Paquier-2
On Fri, Feb 01, 2019 at 10:43:37PM +0000, Bossart, Nathan wrote:
> I would suggest adding this option to vacuumdb in this patch set as
> well.

Doing it would be simple enough, for now I have moved it to next CF.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Alvaro Herrera-9
In reply to this post by Bossart, Nathan
On 2019-Feb-01, Bossart, Nathan wrote:

> IMHO we could document this feature at a slightly higher level without
> leaving out any really important user-facing behavior.  Here's a quick
> attempt to show what I am thinking:
>
>         With this option, VACUUM skips all index cleanup behavior and
>         only marks tuples as "dead" without reclaiming the storage.
>         While this can help reclaim transaction IDs faster to avoid
>         transaction ID wraparound (see Section 24.1.5), it will not
>         reduce bloat.

Hmm ... don't we compact out the storage for dead tuples?  If we do (and
I think we should) then this wording is not entirely correct.

>         Note that this option is ignored for tables
>         that have no indexes.  Also, this option cannot be used in
>         conjunction with the FULL option, since FULL requires
>         rewriting the table.

I would remove the "Also," in there, since it seems to me to give the
wrong impression about those two things being similar, but they're not:
one is convenient behavior, the other is a limitation.

> + /* Notify user that DISABLE_INDEX_CLEANUP option is ignored */
> + if (!vacrelstats->hasindex && (options & VACOPT_DISABLE_INDEX_CLEANUP))
> + ereport(NOTICE,
> + (errmsg("DISABLE_INDEX_CLEANUP is ignored because table \"%s\" does not have index",
> + RelationGetRelationName(onerel))));
>
> It might make more sense to emit this in lazy_scan_heap() where we
> determine the value of skip_index_vacuum.

Why do we need a NOTICE here?  I think this case should be silent.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Masahiko Sawada
On Sat, Feb 2, 2019 at 7:00 PM Alvaro Herrera <[hidden email]> wrote:

>
> On 2019-Feb-01, Bossart, Nathan wrote:
>
> > IMHO we could document this feature at a slightly higher level without
> > leaving out any really important user-facing behavior.  Here's a quick
> > attempt to show what I am thinking:
> >
> >         With this option, VACUUM skips all index cleanup behavior and
> >         only marks tuples as "dead" without reclaiming the storage.
> >         While this can help reclaim transaction IDs faster to avoid
> >         transaction ID wraparound (see Section 24.1.5), it will not
> >         reduce bloat.
>
> Hmm ... don't we compact out the storage for dead tuples?  If we do (and
> I think we should) then this wording is not entirely correct.
Yeah, we remove tuple and leave the dead ItemId. So we actually
reclaim the almost tuple storage.

>
> >         Note that this option is ignored for tables
> >         that have no indexes.  Also, this option cannot be used in
> >         conjunction with the FULL option, since FULL requires
> >         rewriting the table.
>
> I would remove the "Also," in there, since it seems to me to give the
> wrong impression about those two things being similar, but they're not:
> one is convenient behavior, the other is a limitation.

Agreed.

>
> > +     /* Notify user that DISABLE_INDEX_CLEANUP option is ignored */
> > +     if (!vacrelstats->hasindex && (options & VACOPT_DISABLE_INDEX_CLEANUP))
> > +             ereport(NOTICE,
> > +                             (errmsg("DISABLE_INDEX_CLEANUP is ignored because table \"%s\" does not have index",
> > +                                             RelationGetRelationName(onerel))));
> >
> > It might make more sense to emit this in lazy_scan_heap() where we
> > determine the value of skip_index_vacuum.
>
> Why do we need a NOTICE here?  I think this case should be silent.
>
Okay, removed it.

On Fri, Feb 1, 2019 at 11:43 PM Bossart, Nathan <[hidden email]> 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

Attached the updated patch and the patch for vacuumdb.

Regards,

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

v6-0001-Add-DISABLE_INDEX_CLEANUP-option-to-VACUUM-comman.patch (16K) Download Attachment
v6-0002-Add-diable-index-cleanup-option-to-vacuumdb.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Bossart, Nathan
On 2/3/19, 1:48 PM, "Masahiko Sawada" <[hidden email]> wrote:

> On Sat, Feb 2, 2019 at 7:00 PM Alvaro Herrera <[hidden email]> wrote:
>>
>> On 2019-Feb-01, Bossart, Nathan wrote:
>>
>> > IMHO we could document this feature at a slightly higher level without
>> > leaving out any really important user-facing behavior.  Here's a quick
>> > attempt to show what I am thinking:
>> >
>> >         With this option, VACUUM skips all index cleanup behavior and
>> >         only marks tuples as "dead" without reclaiming the storage.
>> >         While this can help reclaim transaction IDs faster to avoid
>> >         transaction ID wraparound (see Section 24.1.5), it will not
>> >         reduce bloat.
>>
>> Hmm ... don't we compact out the storage for dead tuples?  If we do (and
>> I think we should) then this wording is not entirely correct.
>
> Yeah, we remove tuple and leave the dead ItemId. So we actually
> reclaim the almost tuple storage.

Ah, yes.  I was wrong here.  Thank you for clarifying.

> Attached the updated patch and the patch for vacuumdb.

Thanks!  I am hoping to take a deeper look at this patch in the next
couple of days.

Nathan

Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

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

On 2/3/19, 1:48 PM, "Masahiko Sawada" <[hidden email]> wrote:

> On Fri, Feb 1, 2019 at 11:43 PM Bossart, Nathan <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Masahiko Sawada
On Wed, Feb 27, 2019 at 10:02 AM Bossart, Nathan <[hidden email]> 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" <[hidden email]> wrote:
> > On Fri, Feb 1, 2019 at 11:43 PM Bossart, Nathan <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Bossart, Nathan
On 2/27/19, 2:08 AM, "Masahiko Sawada" <[hidden email]> wrote:

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

I think it is valuable.  When DISABLE_INDEX_CLEANUP is not used or it
is used for a relation with no indexes, it makes it clear that no
tuples were left marked as dead.  Also, it looks like all of the other
information here is provided regardless of the options used.  IMO it
is good to list all of the stats so that users have the full picture
of what VACUUM did.

Nathan

Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Masahiko Sawada
On Thu, Feb 28, 2019 at 2:46 AM Bossart, Nathan <[hidden email]> wrote:

>
> On 2/27/19, 2:08 AM, "Masahiko Sawada" <[hidden email]> wrote:
> >> +       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?
>
> I think it is valuable.  When DISABLE_INDEX_CLEANUP is not used or it
> is used for a relation with no indexes, it makes it clear that no
> tuples were left marked as dead.  Also, it looks like all of the other
> information here is provided regardless of the options used.  IMO it
> is good to list all of the stats so that users have the full picture
> of what VACUUM did.
>
I see your point. That seems good to me.

Attached the updated version patch. I've incorporated all review
comments I got and have changed the number of tuples being reported as
'removed tuples'. With this option, tuples completely being removed is
only tuples marked as unused during HOT-pruning, other dead tuples are
left. So we count those tuples during HOT-pruning and reports it as
removed tuples.

Regards,

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

v7-0001-Add-DISABLE_INDEX_CLEANUP-option-to-VACUUM-comman.patch (22K) Download Attachment
v7-0002-Add-diable-index-cleanup-option-to-vacuumdb.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Robert Haas
On Thu, Feb 28, 2019 at 3:12 AM Masahiko Sawada <[hidden email]> wrote:
> Attached the updated version patch.

Regarding the user interface for this patch, please have a look at the
concerns I mention in

https://www.postgresql.org/message-id/CA+TgmoZORX_UUv67rjaSX_aswkdZWV8kWfKfrWxyLdCqFqj+Yw@...

I provided a suggestion there as to how to resolve the issue but
naturally others may feel differently.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Bossart, Nathan
On 2/28/19, 12:13 AM, "Masahiko Sawada" <[hidden email]> wrote:
> Attached the updated version patch. I've incorporated all review
> comments I got and have changed the number of tuples being reported as
> 'removed tuples'. With this option, tuples completely being removed is
> only tuples marked as unused during HOT-pruning, other dead tuples are
> left. So we count those tuples during HOT-pruning and reports it as
> removed tuples.

Thanks for the new patches.  Beyond the reloptions discussion, most of
my feedback is wording suggestions.

+      <command>VACUUM</command> removes dead tuples and prunes HOT-updated
+      tuples chain for live tuples on table. If the table has any dead tuple
+      it removes them from both table and indexes for re-use. With this
+      option <command>VACUUM</command> doesn't completely remove dead tuples
+      and disables removing dead tuples from indexes.  This is suitable for
+      avoiding transaction ID wraparound (see
+      <xref linkend="vacuum-for-wraparound"/>) but not sufficient for avoiding
+      index bloat. This option is ignored if the table doesn't have index.
+      This cannot be used in conjunction with <literal>FULL</literal>
+      option.

There are a couple of small changes I would make.  How does something
like this sound?

    VACUUM removes dead tuples and prunes HOT-updated tuple chains for
    live tuples on the table.  If the table has any dead tuples, it
    removes them from both the table and its indexes and marks the
    corresponding line pointers as available for re-use.  With this
    option, VACUUM still removes dead tuples from the table, but it
    does not process any indexes, and the line pointers are marked as
    dead instead of available for re-use.  This is suitable for
    avoiding transaction ID wraparound (see Section 24.1.5) but not
    sufficient for avoiding index bloat.  This option is ignored if
    the table does not have any indexes.  This cannot be used in
    conjunction with the FULL option.

- * Returns the number of tuples deleted from the page and sets
- * latestRemovedXid.
+ * Returns the number of tuples deleted from the page and set latestRemoveXid
+ * and increment nunused.

I would say something like: "Returns the number of tuples deleted from
the page, sets latestRemovedXid, and updates nunused."

+ /*
+ * hasindex = true means two-pass strategy; false means one-pass. But we
+ * always use the one-pass strategy when index vacuum is disabled.
+ */

I think the added sentence should make it more clear that hasindex
will still be true when DISABLE_INDEX_CLEANUP is used.  Maybe
something like:

    /*
     * hasindex = true means two-pass strategy; false means one-pass
     *
     * If DISABLE_INDEX_CLEANUP is used, hasindex may still be true,
     * but we'll always use the one-pass strategy.
     */

                tups_vacuumed += heap_page_prune(onerel, buf, OldestXmin, false,
-                                                                                &vacrelstats->latestRemovedXid);
+                                                                                &vacrelstats->latestRemovedXid,
+                                                                                &tups_pruned);

Why do we need a separate tups_pruned argument in heap_page_prune()?
Could we add the result of heap_page_prune() to tups_pruned instead,
then report the total number of removed tuples as tups_vacuumed +
tups_pruned elsewhere?

+ * If there are no indexes or we skip index vacuum then we can vacuum
+ * the page right now instead of doing a second scan.

How about:

    If there are no indexes or index cleanup is disabled, we can
    vacuum the page right now instead of doing a second scan.

+ /*
+ * Here, we have indexes but index vacuum is disabled. We don't
+ * vacuum dead tuples on heap but forget them as we skip index
+ * vacuum. The vacrelstats->dead_tuples could have tuples which
+ * became dead after checked at HOT-pruning time but aren't marked
+ * as dead yet. We don't process them because it's a very rare
+ * condition and the next vacuum will process them.
+ */

I would suggest a few small changes:

    /*
     * Here, we have indexes but index vacuum is disabled.  Instead of
     * vacuuming the dead tuples on the heap, we just forget them.
     *
     * Note that vacrelstats->dead_tuples could include tuples which
     * became dead after HOT-pruning but are not marked dead yet.  We
     * do not process them because this is a very rare condition, and
     * the next vacuum will process them anyway.
     */

-       /* If no indexes, make log report that lazy_vacuum_heap would've made */
+       /*
+        * If no index or disables index vacuum, make log report that lazy_vacuum_heap
+        * would've made. If index vacuum is disabled, we didn't remove all dead
+        * tuples but did for tuples removed by HOT-pruning.
+        */
        if (vacuumed_pages)
                ereport(elevel,
                                (errmsg("\"%s\": removed %.0f row versions in %u pages",
                                                RelationGetRelationName(onerel),
-                                               tups_vacuumed, vacuumed_pages)));
+                                               skip_index_vacuum ? tups_pruned : tups_vacuumed,
+                                               vacuumed_pages)));

How about:

    /*
     * If no index or index vacuum is disabled, make log report that
     * lazy_vacuum_heap would've made.  If index vacuum is disabled,
     * we don't include the tuples that we marked dead, but we do
     * include tuples removed by HOT-pruning.
     */

Another interesting thing I noticed is that this "removed X row
versions" message is only emitted if vacuumed_pages is greater than 0.
However, if we only did HOT pruning, tups_vacuumed will be greater
than 0 while vacuumed_pages will still be 0, so some information will
be left out.  I think this is already the case, though, so this could
probably be handled in a separate thread.

Nathan

Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Masahiko Sawada
On Tue, Mar 5, 2019 at 8:27 AM Bossart, Nathan <[hidden email]> wrote:

>
> On 2/28/19, 12:13 AM, "Masahiko Sawada" <[hidden email]> wrote:
> > Attached the updated version patch. I've incorporated all review
> > comments I got and have changed the number of tuples being reported as
> > 'removed tuples'. With this option, tuples completely being removed is
> > only tuples marked as unused during HOT-pruning, other dead tuples are
> > left. So we count those tuples during HOT-pruning and reports it as
> > removed tuples.
>
> Thanks for the new patches.  Beyond the reloptions discussion, most of
> my feedback is wording suggestions.

Thank you for the comment!

>
> +      <command>VACUUM</command> removes dead tuples and prunes HOT-updated
> +      tuples chain for live tuples on table. If the table has any dead tuple
> +      it removes them from both table and indexes for re-use. With this
> +      option <command>VACUUM</command> doesn't completely remove dead tuples
> +      and disables removing dead tuples from indexes.  This is suitable for
> +      avoiding transaction ID wraparound (see
> +      <xref linkend="vacuum-for-wraparound"/>) but not sufficient for avoiding
> +      index bloat. This option is ignored if the table doesn't have index.
> +      This cannot be used in conjunction with <literal>FULL</literal>
> +      option.
>
> There are a couple of small changes I would make.  How does something
> like this sound?
>
>     VACUUM removes dead tuples and prunes HOT-updated tuple chains for
>     live tuples on the table.  If the table has any dead tuples, it
>     removes them from both the table and its indexes and marks the
>     corresponding line pointers as available for re-use.  With this
>     option, VACUUM still removes dead tuples from the table, but it
>     does not process any indexes, and the line pointers are marked as
>     dead instead of available for re-use.  This is suitable for
>     avoiding transaction ID wraparound (see Section 24.1.5) but not
>     sufficient for avoiding index bloat.  This option is ignored if
>     the table does not have any indexes.  This cannot be used in
>     conjunction with the FULL option.

Hmm, that's good idea but I'm not sure that user knows the word 'line
pointer' because it is used only at pageinspect document. I wonder if
the word 'item identifier' would rather be appropriate here because
this is used at at describing page layout(in storage.sgml). Thought?

>
> - * Returns the number of tuples deleted from the page and sets
> - * latestRemovedXid.
> + * Returns the number of tuples deleted from the page and set latestRemoveXid
> + * and increment nunused.
>
> I would say something like: "Returns the number of tuples deleted from
> the page, sets latestRemovedXid, and updates nunused."

Fixed.

>
> +       /*
> +        * hasindex = true means two-pass strategy; false means one-pass. But we
> +        * always use the one-pass strategy when index vacuum is disabled.
> +        */
>
> I think the added sentence should make it more clear that hasindex
> will still be true when DISABLE_INDEX_CLEANUP is used.  Maybe
> something like:
>
>     /*
>      * hasindex = true means two-pass strategy; false means one-pass
>      *
>      * If DISABLE_INDEX_CLEANUP is used, hasindex may still be true,
>      * but we'll always use the one-pass strategy.
>      */
>

Fixed.

>                 tups_vacuumed += heap_page_prune(onerel, buf, OldestXmin, false,
> -                                                                                &vacrelstats->latestRemovedXid);
> +                                                                                &vacrelstats->latestRemovedXid,
> +                                                                                &tups_pruned);
>
> Why do we need a separate tups_pruned argument in heap_page_prune()?
> Could we add the result of heap_page_prune() to tups_pruned instead,
> then report the total number of removed tuples as tups_vacuumed +
> tups_pruned elsewhere?

Hmm, I thought that we should report only the number of tuples
completely removed but we already count the tulples marked as
redirected as tups_vacuumed. Let me summarize the fate of dead tuples.
I think we can roughly classify dead tuples as follows.

1. root tuple of HOT chain that became dead
2. root tuple of HOT chain that became redirected
3. other tupels of HOT chain that became unused
4. tuples that became dead after HOT pruning

The tuples of #1 through #3 either have only ItemIDs or have been
completely removed but tuples of #4 has its tuple storage because they
are not processed when HOT-pruning.

Currently tups_vacuumed counts all of them, nleft (=
vacrelstats->num_dead_tuples) counts #1 + #4. I think that the number
of removed tuples being reported would be #1 + #2 + #3. Or should we
use  #2 + #3 instead?

>
> +                * If there are no indexes or we skip index vacuum then we can vacuum
> +                * the page right now instead of doing a second scan.
>
> How about:
>
>     If there are no indexes or index cleanup is disabled, we can
>     vacuum the page right now instead of doing a second scan.

Fixed.

>
> +                               /*
> +                                * Here, we have indexes but index vacuum is disabled. We don't
> +                                * vacuum dead tuples on heap but forget them as we skip index
> +                                * vacuum. The vacrelstats->dead_tuples could have tuples which
> +                                * became dead after checked at HOT-pruning time but aren't marked
> +                                * as dead yet. We don't process them because it's a very rare
> +                                * condition and the next vacuum will process them.
> +                                */
>
> I would suggest a few small changes:
>
>     /*
>      * Here, we have indexes but index vacuum is disabled.  Instead of
>      * vacuuming the dead tuples on the heap, we just forget them.
>      *
>      * Note that vacrelstats->dead_tuples could include tuples which
>      * became dead after HOT-pruning but are not marked dead yet.  We
>      * do not process them because this is a very rare condition, and
>      * the next vacuum will process them anyway.
>      */

Fixed.

>
> -       /* If no indexes, make log report that lazy_vacuum_heap would've made */
> +       /*
> +        * If no index or disables index vacuum, make log report that lazy_vacuum_heap
> +        * would've made. If index vacuum is disabled, we didn't remove all dead
> +        * tuples but did for tuples removed by HOT-pruning.
> +        */
>         if (vacuumed_pages)
>                 ereport(elevel,
>                                 (errmsg("\"%s\": removed %.0f row versions in %u pages",
>                                                 RelationGetRelationName(onerel),
> -                                               tups_vacuumed, vacuumed_pages)));
> +                                               skip_index_vacuum ? tups_pruned : tups_vacuumed,
> +                                               vacuumed_pages)));
>
> How about:
>
>     /*
>      * If no index or index vacuum is disabled, make log report that
>      * lazy_vacuum_heap would've made.  If index vacuum is disabled,
>      * we don't include the tuples that we marked dead, but we do
>      * include tuples removed by HOT-pruning.
>      */

Fixed.

>
> Another interesting thing I noticed is that this "removed X row
> versions" message is only emitted if vacuumed_pages is greater than 0.
> However, if we only did HOT pruning, tups_vacuumed will be greater
> than 0 while vacuumed_pages will still be 0, so some information will
> be left out.  I think this is already the case, though, so this could
> probably be handled in a separate thread.
>

Hmm, since this log message is corresponding to the one that
lazy_vacuum_heap makes and total number of removed tuples are always
reported, it seems consistent to me. Do you have another point?


Regards,

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

Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Kyotaro HORIGUCHI-2
In reply to this post by Bossart, Nathan
Hello, I have some other comments.

At Mon, 4 Mar 2019 23:27:10 +0000, "Bossart, Nathan" <[hidden email]> wrote in <[hidden email]>

> On 2/28/19, 12:13 AM, "Masahiko Sawada" <[hidden email]> wrote:
> > Attached the updated version patch. I've incorporated all review
> > comments I got and have changed the number of tuples being reported as
> > 'removed tuples'. With this option, tuples completely being removed is
> > only tuples marked as unused during HOT-pruning, other dead tuples are
> > left. So we count those tuples during HOT-pruning and reports it as
> > removed tuples.
>
> Thanks for the new patches.  Beyond the reloptions discussion, most of
> my feedback is wording suggestions.
>
> +      <command>VACUUM</command> removes dead tuples and prunes HOT-updated
> +      tuples chain for live tuples on table. If the table has any dead tuple
> +      it removes them from both table and indexes for re-use. With this
> +      option <command>VACUUM</command> doesn't completely remove dead tuples
> +      and disables removing dead tuples from indexes.  This is suitable for
> +      avoiding transaction ID wraparound (see
> +      <xref linkend="vacuum-for-wraparound"/>) but not sufficient for avoiding
> +      index bloat. This option is ignored if the table doesn't have index.
> +      This cannot be used in conjunction with <literal>FULL</literal>
> +      option.
>
> There are a couple of small changes I would make.  How does something
> like this sound?
>
>     VACUUM removes dead tuples and prunes HOT-updated tuple chains for
>     live tuples on the table.  If the table has any dead tuples, it
>     removes them from both the table and its indexes and marks the
>     corresponding line pointers as available for re-use.  With this
>     option, VACUUM still removes dead tuples from the table, but it
>     does not process any indexes, and the line pointers are marked as
>     dead instead of available for re-use.  This is suitable for
>     avoiding transaction ID wraparound (see Section 24.1.5) but not
>     sufficient for avoiding index bloat.  This option is ignored if
>     the table does not have any indexes.  This cannot be used in
>     conjunction with the FULL option.
>
> - * Returns the number of tuples deleted from the page and sets
> - * latestRemovedXid.
> + * Returns the number of tuples deleted from the page and set latestRemoveXid
> + * and increment nunused.
>
> I would say something like: "Returns the number of tuples deleted from
> the page, sets latestRemovedXid, and updates nunused."
>
> + /*
> + * hasindex = true means two-pass strategy; false means one-pass. But we
> + * always use the one-pass strategy when index vacuum is disabled.
> + */
>
> I think the added sentence should make it more clear that hasindex
> will still be true when DISABLE_INDEX_CLEANUP is used.  Maybe
> something like:
>
>     /*
>      * hasindex = true means two-pass strategy; false means one-pass
>      *
>      * If DISABLE_INDEX_CLEANUP is used, hasindex may still be true,
>      * but we'll always use the one-pass strategy.
>      */
>
>                 tups_vacuumed += heap_page_prune(onerel, buf, OldestXmin, false,
> -                                                                                &vacrelstats->latestRemovedXid);
> +                                                                                &vacrelstats->latestRemovedXid,
> +                                                                                &tups_pruned);
>
> Why do we need a separate tups_pruned argument in heap_page_prune()?
> Could we add the result of heap_page_prune() to tups_pruned instead,
> then report the total number of removed tuples as tups_vacuumed +
> tups_pruned elsewhere?
>
> + * If there are no indexes or we skip index vacuum then we can vacuum
> + * the page right now instead of doing a second scan.
>
> How about:
>
>     If there are no indexes or index cleanup is disabled, we can
>     vacuum the page right now instead of doing a second scan.
>
> + /*
> + * Here, we have indexes but index vacuum is disabled. We don't
> + * vacuum dead tuples on heap but forget them as we skip index
> + * vacuum. The vacrelstats->dead_tuples could have tuples which
> + * became dead after checked at HOT-pruning time but aren't marked
> + * as dead yet. We don't process them because it's a very rare
> + * condition and the next vacuum will process them.
> + */
>
> I would suggest a few small changes:
>
>     /*
>      * Here, we have indexes but index vacuum is disabled.  Instead of
>      * vacuuming the dead tuples on the heap, we just forget them.
>      *
>      * Note that vacrelstats->dead_tuples could include tuples which
>      * became dead after HOT-pruning but are not marked dead yet.  We
>      * do not process them because this is a very rare condition, and
>      * the next vacuum will process them anyway.
>      */
>
> -       /* If no indexes, make log report that lazy_vacuum_heap would've made */
> +       /*
> +        * If no index or disables index vacuum, make log report that lazy_vacuum_heap
> +        * would've made. If index vacuum is disabled, we didn't remove all dead
> +        * tuples but did for tuples removed by HOT-pruning.
> +        */
>         if (vacuumed_pages)
>                 ereport(elevel,
>                                 (errmsg("\"%s\": removed %.0f row versions in %u pages",
>                                                 RelationGetRelationName(onerel),
> -                                               tups_vacuumed, vacuumed_pages)));
> +                                               skip_index_vacuum ? tups_pruned : tups_vacuumed,
> +                                               vacuumed_pages)));
>
> How about:
>
>     /*
>      * If no index or index vacuum is disabled, make log report that
>      * lazy_vacuum_heap would've made.  If index vacuum is disabled,
>      * we don't include the tuples that we marked dead, but we do
>      * include tuples removed by HOT-pruning.
>      */
>
> Another interesting thing I noticed is that this "removed X row
> versions" message is only emitted if vacuumed_pages is greater than 0.
> However, if we only did HOT pruning, tups_vacuumed will be greater
> than 0 while vacuumed_pages will still be 0, so some information will
> be left out.  I think this is already the case, though, so this could
> probably be handled in a separate thread.


+            nleft;            /* item pointers we left */

The name seems to be something other, and the comment doesn't
makes sense at least.. for me.. Looking below,

+                                    "%.0f tuples are left as dead.\n",
+                                    nleft),
+                     nleft);

How about "nleft_dead; /* iterm pointers left as dead */"?



In this block:

-        if (nindexes == 0 &&
+        if ((nindexes == 0 || skip_index_vacuum) &&
             vacrelstats->num_dead_tuples > 0)
         {

Is it right that vacuumed_pages is incremented and FSM is updated
while the page is not actually vacuumed?


         tups_vacuumed += heap_page_prune(onerel, buf, OldestXmin, false,
-                                         &vacrelstats->latestRemovedXid);
+                                         &vacrelstats->latestRemovedXid,
+                                         &tups_pruned);

tups_pruned looks as "HOT pruned tuples". It is named "unused" in
the function's parameters. (But I think it is useless. Please see
the details below.)


I tested it with a simple data set.

(autovacuum = off)
drop table if exists t;
create table t with (fillfactor=50) as select a, a % 3 as b from generate_series(1, 9) a;
create index on t(a);
update t set a = -a where b = 0;
update t set b = b + 1 where b = 1;

We now have 9 tuples, 15 versions and 3 out of 6 "old" tuples are
to be "left dead" by DISABLE_INDEX_CLEANUP vacuum. It means,
three tuples ends with "left dead", three tuples are removed and
12 tuples will survive the vacuum below.

vacuum (verbose, freeze ,disable_index_cleanup, disable_page_skipping) t;

> INFO:  "t": removed 0 row versions in 1 pages
> INFO:  "t": found 0 removable, 9 nonremovable row versions in 1 out of 1 pages
> DETAIL:  0 dead row versions cannot be removed yet, oldest xmin: 925
> There were 0 unused item pointers.
> Skipped 0 pages due to buffer pins, 0 frozen pages.
> 0 pages are entirely empty.
> 3 tuples are left as dead.

Three tuple versions have vanished. Actually they were removed
but not shown in the message.

heap_prune_chain() doesn't count a live root entry of a chain as
"unused (line pointer)" since it is marked as "redierected". As
the result the vanished tuples are counted in tups_vacuumed, not
in tups_pruned. Maybe the name tups_vacuumed is confusing.  After
removing tups_pruned code it works correctly.

> INFO:  "t": removed 6 row versions in 1 pages
> INFO:  "t": found 6 removable, 9 nonremovable row versions in 1 out of 1 pages
> DETAIL:  0 dead row versions cannot be removed yet, oldest xmin: 932
> There were 0 unused item pointers.
> Skipped 0 pages due to buffer pins, 0 frozen pages.
> 0 pages are entirely empty.
> 3 tuples are left as dead.

I see two choices of the second line above.

1> "t": found 6 removable, 9 nonremovable row versions in 1 out of 1 pages

  "removable" includes "left dead" tuples.

2> "t": found 3 removable, 12 nonremovable row versions in 1 out of 1 pages

  "removable" excludes "left dead" tuples.

If you prefer the latter, removable and nonremoveable need to be
corrected using nleft.

> INFO:  "t": found 3 removable, 12 nonremovable row versions in 1 out of 1 pages
> DETAIL:  0 dead row versions cannot be removed yet, oldest xmin: 942
> There were 0 unused item pointers.
> Skipped 0 pages due to buffer pins, 0 frozen pages.
> 0 pages are entirely empty.
> 3 tuples are left as dead.
> CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Bossart, Nathan
On 3/5/19, 1:22 AM, "Masahiko Sawada" <[hidden email]> wrote:

> On Tue, Mar 5, 2019 at 8:27 AM Bossart, Nathan <[hidden email]> wrote:
>> +      <command>VACUUM</command> removes dead tuples and prunes HOT-updated
>> +      tuples chain for live tuples on table. If the table has any dead tuple
>> +      it removes them from both table and indexes for re-use. With this
>> +      option <command>VACUUM</command> doesn't completely remove dead tuples
>> +      and disables removing dead tuples from indexes.  This is suitable for
>> +      avoiding transaction ID wraparound (see
>> +      <xref linkend="vacuum-for-wraparound"/>) but not sufficient for avoiding
>> +      index bloat. This option is ignored if the table doesn't have index.
>> +      This cannot be used in conjunction with <literal>FULL</literal>
>> +      option.
>>
>> There are a couple of small changes I would make.  How does something
>> like this sound?
>>
>>     VACUUM removes dead tuples and prunes HOT-updated tuple chains for
>>     live tuples on the table.  If the table has any dead tuples, it
>>     removes them from both the table and its indexes and marks the
>>     corresponding line pointers as available for re-use.  With this
>>     option, VACUUM still removes dead tuples from the table, but it
>>     does not process any indexes, and the line pointers are marked as
>>     dead instead of available for re-use.  This is suitable for
>>     avoiding transaction ID wraparound (see Section 24.1.5) but not
>>     sufficient for avoiding index bloat.  This option is ignored if
>>     the table does not have any indexes.  This cannot be used in
>>     conjunction with the FULL option.
>
> Hmm, that's good idea but I'm not sure that user knows the word 'line
> pointer' because it is used only at pageinspect document. I wonder if
> the word 'item identifier' would rather be appropriate here because
> this is used at at describing page layout(in storage.sgml). Thought?

That seems reasonable to me.  It seems like ItemIdData is referred to
as "item pointer," "line pointer," or "item identifier" depending on
where you're looking, but ItemPointerData is also referred to as "item
pointer."  I think using "item identifier" is appropriate here for
clarity and consistency with storage.sgml.

>>                 tups_vacuumed += heap_page_prune(onerel, buf, OldestXmin, false,
>> -                                                                                &vacrelstats->latestRemovedXid);
>> +                                                                                &vacrelstats->latestRemovedXid,
>> +                                                                                &tups_pruned);
>>
>> Why do we need a separate tups_pruned argument in heap_page_prune()?
>> Could we add the result of heap_page_prune() to tups_pruned instead,
>> then report the total number of removed tuples as tups_vacuumed +
>> tups_pruned elsewhere?
>
> Hmm, I thought that we should report only the number of tuples
> completely removed but we already count the tulples marked as
> redirected as tups_vacuumed. Let me summarize the fate of dead tuples.
> I think we can roughly classify dead tuples as follows.
>
> 1. root tuple of HOT chain that became dead
> 2. root tuple of HOT chain that became redirected
> 3. other tupels of HOT chain that became unused
> 4. tuples that became dead after HOT pruning
>
> The tuples of #1 through #3 either have only ItemIDs or have been
> completely removed but tuples of #4 has its tuple storage because they
> are not processed when HOT-pruning.
>
> Currently tups_vacuumed counts all of them, nleft (=
> vacrelstats->num_dead_tuples) counts #1 + #4. I think that the number
> of removed tuples being reported would be #1 + #2 + #3. Or should we
> use  #2 + #3 instead?

I think I'm actually more in favor of what was in v6.  IIRC that
version of the patch didn't modify how we tracked the "removed" tuples
at all, but it just added the "X item identifiers left marked dead"
metric.  Since even the tuples we are leaving marked dead lose
storage, that seems accurate enough to me.

>> Another interesting thing I noticed is that this "removed X row
>> versions" message is only emitted if vacuumed_pages is greater than 0.
>> However, if we only did HOT pruning, tups_vacuumed will be greater
>> than 0 while vacuumed_pages will still be 0, so some information will
>> be left out.  I think this is already the case, though, so this could
>> probably be handled in a separate thread.
>>
>
> Hmm, since this log message is corresponding to the one that
> lazy_vacuum_heap makes and total number of removed tuples are always
> reported, it seems consistent to me. Do you have another point?

Here's an example:

        postgres=# CREATE TABLE test (a INT, b INT);
        CREATE TABLE
        postgres=# CREATE INDEX ON test (a);
        CREATE INDEX
        postgres=# INSERT INTO test VALUES (1, 2);
        INSERT 0 1

After only HOT updates, the "removed X row versions in Y pages"
message is not emitted:

        postgres=# UPDATE test SET b = 3;
        UPDATE 1
        postgres=# UPDATE test SET b = 4;
        UPDATE 1
        postgres=# VACUUM (FREEZE, VERBOSE) test;
        INFO:  aggressively vacuuming "public.test"
        INFO:  index "test_a_idx" now contains 1 row versions in 2 pages
        DETAIL:  0 index row versions were removed.
        0 index pages have been deleted, 0 are currently reusable.
        CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s.
        INFO:  "test": found 2 removable, 1 nonremovable row versions in 1 out of 1 pages
        DETAIL:  0 dead row versions cannot be removed yet, oldest xmin: 494
        There were 1 unused item pointers.
        Skipped 0 pages due to buffer pins, 0 frozen pages.
        0 pages are entirely empty.
        CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s.
        VACUUM

After non-HOT updates, the "removed" message is emitted:

        postgres=# UPDATE test SET a = 5;
        UPDATE 1
        postgres=# VACUUM (FREEZE, VERBOSE) test;
        INFO:  aggressively vacuuming "public.test"
        INFO:  scanned index "test_a_idx" to remove 1 row versions
        DETAIL:  CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s
        INFO:  "test": removed 1 row versions in 1 pages
        DETAIL:  CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s
        INFO:  index "test_a_idx" now contains 1 row versions in 2 pages
        DETAIL:  1 index row versions were removed.
        0 index pages have been deleted, 0 are currently reusable.
        CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s.
        INFO:  "test": found 1 removable, 1 nonremovable row versions in 1 out of 1 pages
        DETAIL:  0 dead row versions cannot be removed yet, oldest xmin: 495
        There were 1 unused item pointers.
        Skipped 0 pages due to buffer pins, 0 frozen pages.
        0 pages are entirely empty.
        CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s.
        VACUUM

Nathan

123456