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

Kyotaro HORIGUCHI-2
Hello.

At Wed, 3 Apr 2019 11:55:00 -0400, Robert Haas <[hidden email]> wrote in <[hidden email]>

> On Wed, Apr 3, 2019 at 1:32 AM Masahiko Sawada <[hidden email]> wrote:
> > Attached the updated version patches including the
> > DISABLE_PAGE_SKIPPING part (0003).
>
> I am confused about nleft_dead_tuples.  It looks like it gets
> incremented whenever we set tupgone = true, regardless of whether we
> are doing index cleanup.  But if we ARE doing index cleanup then the
> dead tuple will not be left.  And if we are not doing index vacuum
> then we still don't need this for anything, because tups_vacuumed is
> counting the same thing.  I may be confused.  But if I'm not, then I
> think this should just be ripped out, and we should only keep
> nleft_dead_itemids.

tups_vacuumed is including heap_page_prune()ed tuples, which
aren't counted as "tupgone".

> As far as VacOptTernaryValue, I think it would be safer to change this
> so that VACOPT_TERNARY_DEFAULT = 0.  That way palloc0 will fill in the
> value that people are likely to want by default, which makes it less
> likely that people will accidentally write future code that doesn't
> clean up indexes.

It's convincing. My compalint was enabled=0 and disabled=1 is
confusing so I'm fine with default=0, disabled=1, enabled=2.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center



Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Masahiko Sawada
On Thu, Apr 4, 2019 at 9:18 AM Kyotaro HORIGUCHI
<[hidden email]> wrote:

>
> Hello.
>
> At Wed, 3 Apr 2019 11:55:00 -0400, Robert Haas <[hidden email]> wrote in <[hidden email]>
> > On Wed, Apr 3, 2019 at 1:32 AM Masahiko Sawada <[hidden email]> wrote:
> > > Attached the updated version patches including the
> > > DISABLE_PAGE_SKIPPING part (0003).
> >
> > I am confused about nleft_dead_tuples.  It looks like it gets
> > incremented whenever we set tupgone = true, regardless of whether we
> > are doing index cleanup.  But if we ARE doing index cleanup then the
> > dead tuple will not be left.  And if we are not doing index vacuum
> > then we still don't need this for anything, because tups_vacuumed is
> > counting the same thing.  I may be confused.  But if I'm not, then I
> > think this should just be ripped out, and we should only keep
> > nleft_dead_itemids.
>
> tups_vacuumed is including heap_page_prune()ed tuples, which
> aren't counted as "tupgone".
Yes. tup_vacuumed counts not only HOT pruned tuples but also tuples
that became dead after heap_page_prune(). When index clenaup is
disabled, the former leaves only itemid whereas the latter leaves
itemid and heap tuple as we don't remove. nleft_dead_tuples counts
only the latter to report precisely. I think nleft_dead_tuples should
be incremented only when index cleanup is disabled, and the that part
comment should be polished.

>
> > As far as VacOptTernaryValue, I think it would be safer to change this
> > so that VACOPT_TERNARY_DEFAULT = 0.  That way palloc0 will fill in the
> > value that people are likely to want by default, which makes it less
> > likely that people will accidentally write future code that doesn't
> > clean up indexes.
>
> It's convincing. My compalint was enabled=0 and disabled=1 is
> confusing so I'm fine with default=0, disabled=1, enabled=2.

Okay, fixed.

Attached the updated version patch.

Regards,

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

v14-0001-Add-INDEX_CLEANUP-option-to-VACUUM-command.patch (31K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Robert Haas
On Wed, Apr 3, 2019 at 10:32 PM Masahiko Sawada <[hidden email]> wrote:
> Attached the updated version patch.

Committed with a little bit of documentation tweaking.

--
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 4/4/19, 12:06 PM, "Robert Haas" <[hidden email]> wrote:
> Committed with a little bit of documentation tweaking.

Thanks!  I noticed a very small typo in the new documentation.

diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index fdd8151220..c652f8b0bc 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -199,7 +199,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
       and the table itself will accumulate dead line pointers that cannot be
       removed until index cleanup is completed.  This option has no effect
       for tables that do not have an index and is ignored if the
-      <literal>FULL</literal> is used.
+      <literal>FULL</literal> option is used.
      </para>
     </listitem>
    </varlistentry>

Nathan

Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Masahiko Sawada
In reply to this post by Robert Haas
On Fri, Apr 5, 2019 at 4:06 AM Robert Haas <[hidden email]> wrote:
>
> On Wed, Apr 3, 2019 at 10:32 PM Masahiko Sawada <[hidden email]> wrote:
> > Attached the updated version patch.
>
> Committed with a little bit of documentation tweaking.
>

Thank you for committing them!

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
In reply to this post by Bossart, Nathan
On Thu, Apr 4, 2019 at 5:37 PM Bossart, Nathan <[hidden email]> wrote:
> Thanks!  I noticed a very small typo in the new documentation.

Committed, thanks.

--
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
In reply to this post by Masahiko Sawada
On Fri, Apr 5, 2019 at 10:17 AM Masahiko Sawada <[hidden email]> wrote:

>
> On Fri, Apr 5, 2019 at 4:06 AM Robert Haas <[hidden email]> wrote:
> >
> > On Wed, Apr 3, 2019 at 10:32 PM Masahiko Sawada <[hidden email]> wrote:
> > > Attached the updated version patch.
> >
> > Committed with a little bit of documentation tweaking.
> >
>
> Thank you for committing them!
>

BTW should we support this option for toast tables as well?

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

Andres Freund
In reply to this post by Robert Haas
Hi,

On 2019-04-04 15:05:55 -0400, Robert Haas wrote:
> On Wed, Apr 3, 2019 at 10:32 PM Masahiko Sawada <[hidden email]> wrote:
> > Attached the updated version patch.
>
> Committed with a little bit of documentation tweaking.

I've closed the commitfest entry. I hope that's accurate?

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Masahiko Sawada
On Sat, Apr 6, 2019 at 10:23 AM Andres Freund <[hidden email]> wrote:

>
> Hi,
>
> On 2019-04-04 15:05:55 -0400, Robert Haas wrote:
> > On Wed, Apr 3, 2019 at 10:32 PM Masahiko Sawada <[hidden email]> wrote:
> > > Attached the updated version patch.
> >
> > Committed with a little bit of documentation tweaking.
>
> I've closed the commitfest entry. I hope that's accurate?
>
Yes, but Fujii-san pointed out that this option doesn't support toast
tables and I think there is not specific reason why not supporting
them. So it might be good to add toast.vacuum_index_cleanup. Attached
patch.

Regards,

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

toast_vacuum_index_cleanup.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Michael Paquier-2
On Sat, Apr 06, 2019 at 11:31:31AM +0900, Masahiko Sawada wrote:
> Yes, but Fujii-san pointed out that this option doesn't support toast
> tables and I think there is not specific reason why not supporting
> them. So it might be good to add toast.vacuum_index_cleanup. Attached
> patch.

Being able to control that option at toast level sounds sensible.  I
have added an open item about that.
--
Michael

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

Re: New vacuum option to do only freezing

Tom Lane-2
In reply to this post by Robert Haas
Robert Haas <[hidden email]> writes:
> On Wed, Apr 3, 2019 at 10:32 PM Masahiko Sawada <[hidden email]> wrote:
>> Attached the updated version patch.

> Committed with a little bit of documentation tweaking.

topminnow just failed an assertion from this patch:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=topminnow&dt=2019-04-14%2011%3A01%3A48

The symptoms are:

TRAP: FailedAssertion("!((params->index_cleanup == VACOPT_TERNARY_ENABLED && nleft_dead_tuples == 0 && nleft_dead_itemids == 0) || params->index_cleanup == VACOPT_TERNARY_DISABLED)", File: "/home/nm/farm/mipsel_deb8_gcc_32/HEAD/pgsql.build/../pgsql/src/backend/access/heap/vacuumlazy.c", Line: 1404)
...
2019-04-14 14:49:16.328 CEST [15282:5] LOG:  server process (PID 18985) was terminated by signal 6: Aborted
2019-04-14 14:49:16.328 CEST [15282:6] DETAIL:  Failed process was running: autovacuum: VACUUM ANALYZE pg_catalog.pg_depend

Just looking at the logic around index_cleanup, I rather think that
that assertion is flat out wrong:

+    /* No dead tuples should be left if index cleanup is enabled */
+    Assert((params->index_cleanup == VACOPT_TERNARY_ENABLED &&
+            nleft_dead_tuples == 0 && nleft_dead_itemids == 0) ||
+           params->index_cleanup == VACOPT_TERNARY_DISABLED);

Either it's wrong, or this is:

+                        /*
+                         * Since this dead tuple will not be vacuumed and
+                         * ignored when index cleanup is disabled we count
+                         * count it for reporting.
+                         */
+                        if (params->index_cleanup == VACOPT_TERNARY_ENABLED)
+                            nleft_dead_tuples++;

The poor quality of that comment suggests that maybe the code is just
as confused.

(I also think that that "ternary option" stuff is unreadably overdesigned
notation, which possibly contributed to getting this wrong.  If even the
author can't keep it straight, it's unreadable.)

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Masahiko Sawada
On Mon, Apr 15, 2019 at 12:47 AM Tom Lane <[hidden email]> wrote:

>
> Robert Haas <[hidden email]> writes:
> > On Wed, Apr 3, 2019 at 10:32 PM Masahiko Sawada <[hidden email]> wrote:
> >> Attached the updated version patch.
>
> > Committed with a little bit of documentation tweaking.
>
> topminnow just failed an assertion from this patch:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=topminnow&dt=2019-04-14%2011%3A01%3A48
>
> The symptoms are:
>
> TRAP: FailedAssertion("!((params->index_cleanup == VACOPT_TERNARY_ENABLED && nleft_dead_tuples == 0 && nleft_dead_itemids == 0) || params->index_cleanup == VACOPT_TERNARY_DISABLED)", File: "/home/nm/farm/mipsel_deb8_gcc_32/HEAD/pgsql.build/../pgsql/src/backend/access/heap/vacuumlazy.c", Line: 1404)
> ...
> 2019-04-14 14:49:16.328 CEST [15282:5] LOG:  server process (PID 18985) was terminated by signal 6: Aborted
> 2019-04-14 14:49:16.328 CEST [15282:6] DETAIL:  Failed process was running: autovacuum: VACUUM ANALYZE pg_catalog.pg_depend
>
> Just looking at the logic around index_cleanup, I rather think that
> that assertion is flat out wrong:
>
> +    /* No dead tuples should be left if index cleanup is enabled */
> +    Assert((params->index_cleanup == VACOPT_TERNARY_ENABLED &&
> +            nleft_dead_tuples == 0 && nleft_dead_itemids == 0) ||
> +           params->index_cleanup == VACOPT_TERNARY_DISABLED);
>
> Either it's wrong, or this is:
>
> +                        /*
> +                         * Since this dead tuple will not be vacuumed and
> +                         * ignored when index cleanup is disabled we count
> +                         * count it for reporting.
> +                         */
> +                        if (params->index_cleanup == VACOPT_TERNARY_ENABLED)
> +                            nleft_dead_tuples++;
>

Ugh, I think the assertion is right but the above condition is
completely wrong. We should increment nleft_dead_tuples when index
cleanup is *not* enabled. For nleft_dead_itemids we require that index
cleanup is disabled as follows.

           {
               /*
                * Here, we have indexes but index cleanup is disabled.
Instead of
                * vacuuming the dead tuples on the heap, we just forget them.
                *
                * Note that vacrelstats->dead_tuples could have tuples which
                * became dead after HOT-pruning but are not marked dead yet.
                * We do not process them because it's a very rare condition, and
                * the next vacuum will process them anyway.
                */
               Assert(params->index_cleanup == VACOPT_TERNARY_DISABLED);
               nleft_dead_itemids += vacrelstats->num_dead_tuples;
           }

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

Masahiko Sawada
On Mon, Apr 15, 2019 at 9:28 AM Masahiko Sawada <[hidden email]> wrote:

>
> On Mon, Apr 15, 2019 at 12:47 AM Tom Lane <[hidden email]> wrote:
> >
> > Robert Haas <[hidden email]> writes:
> > > On Wed, Apr 3, 2019 at 10:32 PM Masahiko Sawada <[hidden email]> wrote:
> > >> Attached the updated version patch.
> >
> > > Committed with a little bit of documentation tweaking.
> >
> > topminnow just failed an assertion from this patch:
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=topminnow&dt=2019-04-14%2011%3A01%3A48
> >
> > The symptoms are:
> >
> > TRAP: FailedAssertion("!((params->index_cleanup == VACOPT_TERNARY_ENABLED && nleft_dead_tuples == 0 && nleft_dead_itemids == 0) || params->index_cleanup == VACOPT_TERNARY_DISABLED)", File: "/home/nm/farm/mipsel_deb8_gcc_32/HEAD/pgsql.build/../pgsql/src/backend/access/heap/vacuumlazy.c", Line: 1404)
> > ...
> > 2019-04-14 14:49:16.328 CEST [15282:5] LOG:  server process (PID 18985) was terminated by signal 6: Aborted
> > 2019-04-14 14:49:16.328 CEST [15282:6] DETAIL:  Failed process was running: autovacuum: VACUUM ANALYZE pg_catalog.pg_depend
> >
> > Just looking at the logic around index_cleanup, I rather think that
> > that assertion is flat out wrong:
> >
> > +    /* No dead tuples should be left if index cleanup is enabled */
> > +    Assert((params->index_cleanup == VACOPT_TERNARY_ENABLED &&
> > +            nleft_dead_tuples == 0 && nleft_dead_itemids == 0) ||
> > +           params->index_cleanup == VACOPT_TERNARY_DISABLED);
> >
> > Either it's wrong, or this is:
> >
> > +                        /*
> > +                         * Since this dead tuple will not be vacuumed and
> > +                         * ignored when index cleanup is disabled we count
> > +                         * count it for reporting.
> > +                         */
> > +                        if (params->index_cleanup == VACOPT_TERNARY_ENABLED)
> > +                            nleft_dead_tuples++;
> >
>
> Ugh, I think the assertion is right but the above condition is
> completely wrong. We should increment nleft_dead_tuples when index
> cleanup is *not* enabled.
Here is a draft patch to fix this issue.

Regards,

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

fix_index_cleanup.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Tom Lane-2
Masahiko Sawada <[hidden email]> writes:
>> Ugh, I think the assertion is right but the above condition is
>> completely wrong. We should increment nleft_dead_tuples when index
>> cleanup is *not* enabled.

> Here is a draft patch to fix this issue.

So the real issue here, I fear, is that we've got no consistent testing
of the whole case block for HEAPTUPLE_DEAD, as you can easily confirm
by checking the code coverage report at
https://coverage.postgresql.org/src/backend/access/heap/vacuumlazy.c.gcov.html

This is perhaps unsurprising, given the code comment that points out that
we can only reach that block if the tuple's state changed since
heap_page_prune() a few lines above.  Still, it means that this patch
hasn't been tested in that scenario, until we were lucky enough for
a slow buildfarm machine like topminnow to hit it.

What's more, because that block is the only way for "tupgone" to be
set, we also don't reach the "if (tupgone)" block at lines 1183ff.
And I think this patch has probably broken that, too.  Surely, if we
are not going to remove the tuple, we should not increment tups_vacuumed?
And maybe we have to freeze it instead?  How is it that we can, or should,
treat this situation as different from a dead-but-not-removable tuple?

I have a very strong feeling that this patch was not fully baked.

BTW, I can reproduce the crash fairly easily (within a few cycles
of the regression tests) by (a) inserting pg_usleep(10000) after
the heap_page_prune call, and (b) making autovacuum much more
aggressive.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Robert Haas
On Mon, Apr 15, 2019 at 1:13 PM Tom Lane <[hidden email]> wrote:

> Masahiko Sawada <[hidden email]> writes:
> >> Ugh, I think the assertion is right but the above condition is
> >> completely wrong. We should increment nleft_dead_tuples when index
> >> cleanup is *not* enabled.
>
> > Here is a draft patch to fix this issue.
>
> So the real issue here, I fear, is that we've got no consistent testing
> of the whole case block for HEAPTUPLE_DEAD, as you can easily confirm
> by checking the code coverage report at
> https://coverage.postgresql.org/src/backend/access/heap/vacuumlazy.c.gcov.html
>
> This is perhaps unsurprising, given the code comment that points out that
> we can only reach that block if the tuple's state changed since
> heap_page_prune() a few lines above.  Still, it means that this patch
> hasn't been tested in that scenario, until we were lucky enough for
> a slow buildfarm machine like topminnow to hit it.
>
> What's more, because that block is the only way for "tupgone" to be
> set, we also don't reach the "if (tupgone)" block at lines 1183ff.
> And I think this patch has probably broken that, too.  Surely, if we
> are not going to remove the tuple, we should not increment tups_vacuumed?
> And maybe we have to freeze it instead?  How is it that we can, or should,
> treat this situation as different from a dead-but-not-removable tuple?
>
> I have a very strong feeling that this patch was not fully baked.

I think you're right, but I don't understand the comment in the
preceding paragraph.  How does this problem prevent tupgone from
getting set?

It looks to me like nleft_dead_tuples should be ripped out.  It's
basically trying to count the same thing as tups_vacuumed, and there
doesn't seem to be any need to count that thing twice.  And then just
below this block:

    /* save stats for use later */
    vacrelstats->tuples_deleted = tups_vacuumed;
    vacrelstats->new_dead_tuples = nkeep;
    vacrelstats->nleft_dead_itemids = nleft_dead_itemids;

We should do something like:

if (params->index_cleanup == VACOPT_TERNARY_DISABLED)
{
    nkeep += tups_vacuumed;
    tups_vacuumed = 0;
}

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

Tom Lane-2
Robert Haas <[hidden email]> writes:
> On Mon, Apr 15, 2019 at 1:13 PM Tom Lane <[hidden email]> wrote:
>> I have a very strong feeling that this patch was not fully baked.

> I think you're right, but I don't understand the comment in the
> preceding paragraph.  How does this problem prevent tupgone from
> getting set?

My point is that I suspect that tupgone *shouldn't* get set.
It's not (going to be) gone.

> It looks to me like nleft_dead_tuples should be ripped out.

That was pretty much what I was thinking too.  It makes more sense
just to treat this case identically to dead-but-not-yet-removable.
I have substantial doubts about nleft_dead_itemids being worth
anything, as well.

> We should do something like:
> if (params->index_cleanup == VACOPT_TERNARY_DISABLED)
> {
>     nkeep += tups_vacuumed;
>     tups_vacuumed = 0;
> }

No.  I'm thinking there should be exactly one test of index_cleanup
in this logic, and what it would be is along the lines of

                    if (HeapTupleIsHotUpdated(&tuple) ||
                        HeapTupleIsHeapOnly(&tuple) ||
+                       params->index_cleanup == VACOPT_TERNARY_DISABLED)
                        nkeep += 1;
                    else

In general, this thing has a strong whiff of "large patch
with a small patch struggling to get out".

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Robert Haas
On Mon, Apr 15, 2019 at 3:47 PM Tom Lane <[hidden email]> wrote:
> No.  I'm thinking there should be exactly one test of index_cleanup
> in this logic, and what it would be is along the lines of
>
>                     if (HeapTupleIsHotUpdated(&tuple) ||
>                         HeapTupleIsHeapOnly(&tuple) ||
> +                       params->index_cleanup == VACOPT_TERNARY_DISABLED)
>                         nkeep += 1;
>                     else

I'm not sure that's correct.  If you do that, it'll end up in the
non-tupgone case, which might try to freeze a tuple that should've
been removed.  Or am I confused?

My idea of the mechanism of action of this patch is that we accumulate
the tuples just as if we were going to vacuum them, but then at the
end of each page we forget them all, sorta like there are no indexes.
In that mental model, I don't really see why this part of this logic
needs any adjustment at all vs. pre-patch.

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

Tom Lane-2
Robert Haas <[hidden email]> writes:
> On Mon, Apr 15, 2019 at 3:47 PM Tom Lane <[hidden email]> wrote:
>> No.  I'm thinking there should be exactly one test of index_cleanup
>> in this logic, and what it would be is along the lines of ...

> I'm not sure that's correct.  If you do that, it'll end up in the
> non-tupgone case, which might try to freeze a tuple that should've
> been removed.  Or am I confused?

If we're failing to remove it, and it's below the desired freeze
horizon, then we'd darn well better freeze it instead, no?

Since we know that the tuple only just became dead, I suspect
that the case would be unreachable in practice.  But the approach
you propose risks violating the invariant that all old tuples
will either be removed or frozen.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Michael Paquier-2
On Mon, Apr 15, 2019 at 09:07:16PM -0400, Tom Lane wrote:
> If we're failing to remove it, and it's below the desired freeze
> horizon, then we'd darn well better freeze it instead, no?
>
> Since we know that the tuple only just became dead, I suspect
> that the case would be unreachable in practice.  But the approach
> you propose risks violating the invariant that all old tuples
> will either be removed or frozen.

Please note that I have added an open item for this investigation (see
"topminnow triggered assertion failure with vacuum_index_cleanup").
--
Michael

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

Re: New vacuum option to do only freezing

Masahiko Sawada
In reply to this post by Tom Lane-2
On Tue, Apr 16, 2019 at 4:47 AM Tom Lane <[hidden email]> wrote:

>
> Robert Haas <[hidden email]> writes:
> > On Mon, Apr 15, 2019 at 1:13 PM Tom Lane <[hidden email]> wrote:
> >> I have a very strong feeling that this patch was not fully baked.
>
> > I think you're right, but I don't understand the comment in the
> > preceding paragraph.  How does this problem prevent tupgone from
> > getting set?
>
> My point is that I suspect that tupgone *shouldn't* get set.
> It's not (going to be) gone.
>
> > It looks to me like nleft_dead_tuples should be ripped out.
>
> That was pretty much what I was thinking too.

tups_vacuumed counts not only (1)dead-but-not-yet-removable tuple but
also HOT-pruned tuples. These HOT-pruned tuples include both (2)the
tuples we removed both its itemid and tuple storage and the tuples
(3)we removed only its tuple storage and marked itemid as dead. So we
cannot add tups_vacuumed to nkeeps as it includes completely removed
tuple like tuples-(2). I added nleft_dead_itemids to count only
tuples-(3) and nleft_dead_tuples to count only tuples-(1) for
reporting. Tuples-(2) are removed even when index cleanup is disabled.

> It makes more sense
> just to treat this case identically to dead-but-not-yet-removable.
> I have substantial doubts about nleft_dead_itemids being worth
> anything, as well.

I think that the adding tuples-(3) to nkeeps would be a good idea. If
we do that, nleft_dead_tuples is no longer necessary. On the other
hand, I think we need nleft_dead_itemids to report how many itemids we
left when index cleanup is disabled.
Regards,

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


123456