New vacuum option to do only freezing

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
118 messages Options
123456
Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Robert Haas
On Mon, Apr 15, 2019 at 9:07 PM Tom Lane <[hidden email]> wrote:
> > 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?

I don't know that that's safe.  IIRC, the freeze code doesn't cope
nicely with being given a tuple that actually ought to have been
deleted.  It'll just freeze it anyway, which is obviously bad.

Unless this has been changed since I last looked at it.

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

Alvaro Herrera-9
On 2019-Apr-16, Robert Haas wrote:

> On Mon, Apr 15, 2019 at 9:07 PM Tom Lane <[hidden email]> wrote:
> > > 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?
>
> I don't know that that's safe.  IIRC, the freeze code doesn't cope
> nicely with being given a tuple that actually ought to have been
> deleted.  It'll just freeze it anyway, which is obviously bad.

Umm, but if we fail to freeze it, we'll leave a tuple around that's
below the relfrozenxid for the table, causing later pg_commit to be
truncated and error messages saying that the tuple cannot be read, no?

I remember that for a similar case in multixact-land, what we do is
generate a fresh multixact that carries the members that are still alive
(ie. those that cause the multixact to be kept rather than remove it),
and relabel the tuple with that one.  So the old multixact can be
removed safely.  Obviously we cannot do that for XIDs, but I do wonder
what can possibly cause a tuple to be unfreezable yet the XID to be
below the freeze horizon.  Surely if the transaction is that old, we
should have complained about it, and generated a freeze horizon that was
even older?

> Unless this has been changed since I last looked at it.

I don't think so.

--
Á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
In reply to this post by Robert Haas
On Tue, Apr 16, 2019 at 11:26 PM Robert Haas <[hidden email]> wrote:

>
> On Mon, Apr 15, 2019 at 9:07 PM Tom Lane <[hidden email]> wrote:
> > > 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?
>
> I don't know that that's safe.  IIRC, the freeze code doesn't cope
> nicely with being given a tuple that actually ought to have been
> deleted.  It'll just freeze it anyway, which is obviously bad.

Hmm, I think that we already choose to leave HEAPTUPLE_DEAD tuples and
might freeze them if HeapTupleIsHotUpdated() ||
HeapTupleIsHeapOnly(L1083 at vacuumlazy.c) is true, which actually
have to be deleted. What difference between these tuples and the
tuples that we intentionally leave when index cleanup is disabled?
Maybe I'm missing something and confused.




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

Tom Lane-2
In reply to this post by Alvaro Herrera-9
Alvaro Herrera <[hidden email]> writes:
> On 2019-Apr-16, Robert Haas wrote:
>> On Mon, Apr 15, 2019 at 9:07 PM Tom Lane <[hidden email]> 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?

>> I don't know that that's safe.  IIRC, the freeze code doesn't cope
>> nicely with being given a tuple that actually ought to have been
>> deleted.  It'll just freeze it anyway, which is obviously bad.

> Umm, but if we fail to freeze it, we'll leave a tuple around that's
> below the relfrozenxid for the table, causing later pg_commit to be
> truncated and error messages saying that the tuple cannot be read, no?

Yeah.  If you think that it's unsafe to freeze the tuple, then this
entire patch is ill-conceived and needs to be reverted.  I don't
know how much more plainly I can put it: index_cleanup cannot be a
license to ignore the freeze horizon.  (Indeed, I do not quite see
what the point of the feature is otherwise.  Why would you run a
vacuum with this option at all, if not to increase the table's
relfrozenxid?  But you can *not* advance relfrozenxid if you left
old XIDs behind.)

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Andres Freund
In reply to this post by Alvaro Herrera-9
Hi,

On 2019-04-16 10:54:34 -0400, Alvaro Herrera wrote:

> On 2019-Apr-16, Robert Haas wrote:
> > On Mon, Apr 15, 2019 at 9:07 PM Tom Lane <[hidden email]> wrote:
> > > > 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?
> >
> > I don't know that that's safe.  IIRC, the freeze code doesn't cope
> > nicely with being given a tuple that actually ought to have been
> > deleted.  It'll just freeze it anyway, which is obviously bad.
>
> Umm, but if we fail to freeze it, we'll leave a tuple around that's
> below the relfrozenxid for the table, causing later pg_commit to be
> truncated and error messages saying that the tuple cannot be read, no?

Is the below-relfrozenxid case actually reachable? Isn't the theory of
that whole codeblock that we ought to only get there if a transaction
concurrently commits?

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

And in case there was a concurrent transaction at the time of the
heap_page_prune(), it got to be above the OldestXmin passed to
HeapTupleSatisfiesVacuum() to - as it's the same OldestXmin value.  And
as FreezeLimit should always be older than than OldestXmin, we should
never get into a situation where heap_page_prune() couldn't prune
something that we would have been forced to remove?


> > I don't know that that's safe.  IIRC, the freeze code doesn't cope
> > nicely with being given a tuple that actually ought to have been
> > deleted.  It'll just freeze it anyway, which is obviously bad.
> >
> > Unless this has been changed since I last looked at it.
>
> I don't think so.

I think it has changed a bit - these days heap_prepare_freeze_tuple()
will detect that case, and error out:

                        /*
                         * If we freeze xmax, make absolutely sure that it's not an XID
                         * that is important.  (Note, a lock-only xmax can be removed
                         * independent of committedness, since a committed lock holder has
                         * released the lock).
                         */
                        if (!HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask) &&
                                TransactionIdDidCommit(xid))
                                ereport(ERROR,
                                                (errcode(ERRCODE_DATA_CORRUPTED),
                                                 errmsg_internal("cannot freeze committed xmax %u",
                                                                                 xid)));
and the equivalent multixact case:

                                if (TransactionIdDidCommit(xid))
                                        ereport(ERROR,
                                                        (errcode(ERRCODE_DATA_CORRUPTED),
                                                         errmsg_internal("cannot freeze committed update xid %u", xid)));

We even complain if xmin is uncommitted and would need to be frozen:

                if (TransactionIdPrecedes(xid, cutoff_xid))
                {
                        if (!TransactionIdDidCommit(xid))
                                ereport(ERROR,
                                                (errcode(ERRCODE_DATA_CORRUPTED),
                                                 errmsg_internal("uncommitted xmin %u from before xid cutoff %u needs to be frozen",
                                                                                 xid, cutoff_xid)));


I IIRC added that after one of the multixact issues lead to precisely
that, heap_prepare_freeze_tuple() leading to a valid xmax just being
emptied out, resurfacing dead tuples (and HOT corruption and such).

These messages are obviously intended to be a backstop against
continuing to corrupt further, than actually something a user should
ever see in a working system.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Andres Freund
In reply to this post by Tom Lane-2
Hi,

On 2019-04-16 11:38:01 -0400, Tom Lane wrote:

> Alvaro Herrera <[hidden email]> writes:
> > On 2019-Apr-16, Robert Haas wrote:
> >> On Mon, Apr 15, 2019 at 9:07 PM Tom Lane <[hidden email]> 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?
>
> >> I don't know that that's safe.  IIRC, the freeze code doesn't cope
> >> nicely with being given a tuple that actually ought to have been
> >> deleted.  It'll just freeze it anyway, which is obviously bad.
>
> > Umm, but if we fail to freeze it, we'll leave a tuple around that's
> > below the relfrozenxid for the table, causing later pg_commit to be
> > truncated and error messages saying that the tuple cannot be read, no?
>
> Yeah.  If you think that it's unsafe to freeze the tuple, then this
> entire patch is ill-conceived and needs to be reverted.  I don't
> know how much more plainly I can put it: index_cleanup cannot be a
> license to ignore the freeze horizon.  (Indeed, I do not quite see
> what the point of the feature is otherwise.  Why would you run a
> vacuum with this option at all, if not to increase the table's
> relfrozenxid?  But you can *not* advance relfrozenxid if you left
> old XIDs behind.)

As I just wrote - I don't think this codepath can ever deal with tuples
that old.

Greetings,

Andres Freund


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 Mon, Apr 15, 2019 at 9:07 PM Tom Lane <[hidden email]> 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?

> I don't know that that's safe.  IIRC, the freeze code doesn't cope
> nicely with being given a tuple that actually ought to have been
> deleted.  It'll just freeze it anyway, which is obviously bad.

Looking at heap_prepare_freeze_tuple, it looks to me like it'd notice
the problem and throw an error.  The two possible reasons for a tuple
to be dead are xmin aborted and xmax committed, right?  There are
tests in there that will complain if either of those is true and
the xid is below the freeze horizon.

Given that we don't get here except when the tuple has just become dead,
it probably is all right to assume that it can't possibly get selected
for freezing, and let those tests backstop the assumption.

(BTW, I don't understand why that code will throw "found xmin %u from
before relfrozenxid %u" if HeapTupleHeaderXminFrozen is true?  Shouldn't
the whole if-branch at lines 6113ff be skipped if xmin_frozen?)

                        regards, tom lane

PS: I see that mandrill just replicated the topminnow failure that
started this discussion.


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Andres Freund
Hi,

On 2019-04-16 12:01:36 -0400, Tom Lane wrote:
> (BTW, I don't understand why that code will throw "found xmin %u from
> before relfrozenxid %u" if HeapTupleHeaderXminFrozen is true?  Shouldn't
> the whole if-branch at lines 6113ff be skipped if xmin_frozen?)

I *think* that just looks odd, but isn't actively wrong. That's because
TransactionIdIsNormal() won't trigger, as:

#define HeapTupleHeaderGetXmin(tup) \
( \
        HeapTupleHeaderXminFrozen(tup) ? \
                FrozenTransactionId : HeapTupleHeaderGetRawXmin(tup) \
)

which afaict makes
        xmin_frozen = ((xid == FrozenTransactionId) ||
                                   HeapTupleHeaderXminFrozen(tuple));
redundant.

Looks like that was introduced relatively recently, in

commit d2599ecfcc74fea9fad1720a70210a740c716730
Author: Alvaro Herrera <[hidden email]>
Date:   2018-05-04 15:24:44 -0300

    Don't mark pages all-visible spuriously


@@ -6814,6 +6815,8 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
 
    /* Process xmin */
    xid = HeapTupleHeaderGetXmin(tuple);
+   xmin_frozen = ((xid == FrozenTransactionId) ||
+                  HeapTupleHeaderXminFrozen(tuple));
    if (TransactionIdIsNormal(xid))
    {
        if (TransactionIdPrecedes(xid, relfrozenxid))
@@ -6832,9 +6835,8 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
 
            frz->t_infomask |= HEAP_XMIN_FROZEN;
            changed = true;
+           xmin_frozen = true;
        }
-       else
-           totally_frozen = false;
    }

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Tom Lane-2
So after thinking about this a bit more ...

ISTM that what we have here is a race condition (ie, tuple changed state
since heap_page_prune), and that ideally we want the code to resolve it
as if no race had happened.  That is, either of these behaviors would
be acceptable:

1. Delete the tuple, just as heap_page_prune would've done if it had seen
it DEAD.  (Possibly we could implement that by jumping back and doing
heap_page_prune again, but that seems pretty messy and bug-prone.
In any case, if we're not doing index cleanup then this must reduce to
"replace tuple by a dead line pointer", not remove it altogether.)

2. Act as if the tuple were still live, just as would've happened if the
state didn't change till just after we looked instead of just before.

Option #2 is a lot simpler and safer, and can be implemented as I
suggested earlier, assuming we're all good with the assumption that
heap_prepare_freeze_tuple isn't going to do anything bad.

However ... it strikes me that there's yet another assumption in here
that this patch has broken.  Namely, notice that the reason we normally
don't get here is that what heap_page_prune does with an already-DEAD
tuple is reduce it to a dead line pointer and then count it in its
return value, which gets added to tups_vacuumed.  But then what
lazy_scan_heap's per-tuple loop does is

            /*
             * DEAD item pointers are to be vacuumed normally; but we don't
             * count them in tups_vacuumed, else we'd be double-counting (at
             * least in the common case where heap_page_prune() just freed up
             * a non-HOT tuple).
             */
            if (ItemIdIsDead(itemid))
            {
                lazy_record_dead_tuple(vacrelstats, &(tuple.t_self));
                all_visible = false;
                continue;
            }

When this patch is active, it will *greatly* increase the odds that
we report a misleading tups_vacuumed total, for two different reasons:

* DEAD tuples reduced to dead line pointers during heap_page_prune will be
counted as tups_vacuumed, even though we don't take the further step of
removing the dead line pointer, as always happened before.

* When, after some vacuum cycles with index_cleanup disabled, we finally
do one with index_cleanup enabled, there are going to be a heck of a lot
of old dead line pointers to clean out, which the existing logic won't
count at all.  That was only barely tolerable before, and it seems like
this has pushed it over the bounds of silliness.  People are going to
be wondering why vacuum reports that it removed zillions of index
entries and no tuples.

I'm thinking that we really need to upgrade vacuum's reporting totals
so that it accounts in some more-honest way for pre-existing dead
line pointers.  The patch as it stands has made the reporting even more
confusing, rather than less so.

BTW, the fact that dead line pointers will accumulate without limit
makes me even more dubious of the proposition that this "feature"
will be safe to enable as a reloption in production.  I really think
that we ought to restrict it to be a manual VACUUM option, to be
used only when you're desperate to freeze old tuples.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Tom Lane-2
I wrote:
> I'm thinking that we really need to upgrade vacuum's reporting totals
> so that it accounts in some more-honest way for pre-existing dead
> line pointers.  The patch as it stands has made the reporting even more
> confusing, rather than less so.

Here's a couple of ideas about that:

1. Ignore heap_page_prune's activity altogether, on the grounds that
it's just random chance that any cleanup done there was done during
VACUUM and not some preceding DML operation.  Make tups_vacuumed
be the count of dead line pointers removed.  The advantage of this
way is that tups_vacuumed would become independent of previous
non-VACUUM pruning activity, as well as preceding index-cleanup-disabled
VACUUMs.  But maybe it's hiding too much information.

2. Have heap_page_prune count, and add to tups_vacuumed, only HOT
tuples that it deleted entirely.  The action of replacing a DEAD
root tuple with a dead line pointer doesn't count for anything.
Then also add the count of dead line pointers removed to tups_vacuumed.

Approach #2 is closer to the way we've defined tups_vacuumed up to
now, but I think it'd be more realistic in cases where previous
pruning or index-cleanup-disabled vacuums have left lots of dead
line pointers.

I'm not especially wedded to either of these --- any other ideas?

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Masahiko Sawada
On Wed, Apr 17, 2019 at 5:00 AM Tom Lane <[hidden email]> wrote:

>
> I wrote:
> > I'm thinking that we really need to upgrade vacuum's reporting totals
> > so that it accounts in some more-honest way for pre-existing dead
> > line pointers.  The patch as it stands has made the reporting even more
> > confusing, rather than less so.
>
> Here's a couple of ideas about that:
>
> 1. Ignore heap_page_prune's activity altogether, on the grounds that
> it's just random chance that any cleanup done there was done during
> VACUUM and not some preceding DML operation.  Make tups_vacuumed
> be the count of dead line pointers removed.  The advantage of this
> way is that tups_vacuumed would become independent of previous
> non-VACUUM pruning activity, as well as preceding index-cleanup-disabled
> VACUUMs.  But maybe it's hiding too much information.
>
> 2. Have heap_page_prune count, and add to tups_vacuumed, only HOT
> tuples that it deleted entirely.  The action of replacing a DEAD
> root tuple with a dead line pointer doesn't count for anything.
> Then also add the count of dead line pointers removed to tups_vacuumed.
>
> Approach #2 is closer to the way we've defined tups_vacuumed up to
> now, but I think it'd be more realistic in cases where previous
> pruning or index-cleanup-disabled vacuums have left lots of dead
> line pointers.

On top of the approach #2, how about reporting the number of line
pointers we left so that user can notice that there are many dead line
pointers in the table?

>
> I'm not especially wedded to either of these --- any other ideas?

Or, how about reporting the vacuumed tuples and line pointers we
separately? It might be too detailed but since with this patch we
delete either only tuples or only line pointers, it's more accurate.



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

Tom Lane-2
Masahiko Sawada <[hidden email]> writes:
> On Wed, Apr 17, 2019 at 5:00 AM Tom Lane <[hidden email]> wrote:
>>> I'm thinking that we really need to upgrade vacuum's reporting totals
>>> so that it accounts in some more-honest way for pre-existing dead
>>> line pointers.  The patch as it stands has made the reporting even more
>>> confusing, rather than less so.

> Or, how about reporting the vacuumed tuples and line pointers we
> separately? It might be too detailed but since with this patch we
> delete either only tuples or only line pointers, it's more accurate.

Yeah, if we wanted to expose these complications more directly, we
could think about adding or changing the main counters.  I was wondering
about whether we should have it report "x bytes and y line pointers
freed", rather than counting tuples per se.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Peter Geoghegan-4
On Wed, Apr 17, 2019 at 10:46 AM Tom Lane <[hidden email]> wrote:
> Yeah, if we wanted to expose these complications more directly, we
> could think about adding or changing the main counters.  I was wondering
> about whether we should have it report "x bytes and y line pointers
> freed", rather than counting tuples per se.

I like that idea, but I'm pretty sure that there are very few users
that are aware of these distinctions at all. It would be a good idea
to clearly document them.

--
Peter Geoghegan


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Masahiko Sawada
On Thu, Apr 18, 2019 at 4:20 AM Peter Geoghegan <[hidden email]> wrote:
>
> On Wed, Apr 17, 2019 at 10:46 AM Tom Lane <[hidden email]> wrote:
> > Yeah, if we wanted to expose these complications more directly, we
> > could think about adding or changing the main counters.  I was wondering
> > about whether we should have it report "x bytes and y line pointers
> > freed", rather than counting tuples per se.
>

It looks good idea to me.

> I like that idea, but I'm pretty sure that there are very few users
> that are aware of these distinctions at all. It would be a good idea
> to clearly document them.

I completely agreed. I'm sure that only a few user can do the action
of enabling index cleanup when the report says there are many dead
line pointers in the table.

It brought me an another idea of reporting something like "x bytes
freed, y bytes can be freed after index cleanup". That is, we report
how much bytes including tuples and line pointers we freed and how
much bytes of line pointers can be freed after index cleanup. While
index cleanup is enabled, the latter value should always be 0. If the
latter value gets large user can be aware of necessity of doing index
cleanup.

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 Thu, Apr 18, 2019 at 3:12 PM Masahiko Sawada <[hidden email]> wrote:

>
> On Thu, Apr 18, 2019 at 4:20 AM Peter Geoghegan <[hidden email]> wrote:
> >
> > On Wed, Apr 17, 2019 at 10:46 AM Tom Lane <[hidden email]> wrote:
> > > Yeah, if we wanted to expose these complications more directly, we
> > > could think about adding or changing the main counters.  I was wondering
> > > about whether we should have it report "x bytes and y line pointers
> > > freed", rather than counting tuples per se.
> >
>
> It looks good idea to me.
>
> > I like that idea, but I'm pretty sure that there are very few users
> > that are aware of these distinctions at all. It would be a good idea
> > to clearly document them.
>
> I completely agreed. I'm sure that only a few user can do the action
> of enabling index cleanup when the report says there are many dead
> line pointers in the table.
>
> It brought me an another idea of reporting something like "x bytes
> freed, y bytes can be freed after index cleanup". That is, we report
> how much bytes including tuples and line pointers we freed and how
> much bytes of line pointers can be freed after index cleanup. While
> index cleanup is enabled, the latter value should always be 0. If the
> latter value gets large user can be aware of necessity of doing index
> cleanup.
>
Attached the draft version of patch based on the discussion so far.
This patch fixes two issues: the assertion error topminnow reported
and the contents of the vacuum logs.

For the first issue, I've changed lazy_scan_heap so that it counts the
tuples that became dead after HOT pruning in nkeep when index cleanup
is disabled. As per discussions so far, it would be no problem to try
to freeze tuples that ought to have been deleted.

For the second issue, I've changed lazy vacuum so that it reports both
the number of kilobytes we freed and the number of kilobytes can be
freed after index cleanup. The former value includes the size of not
only heap tuples but also line pointers. That is, when a normal tuple
has been marked as either dead or redirected we count only the size of
heap tuple, and when it has been marked as unused we count the size of
both heap tuple and line pointer. Similarly when either a redirect
line pointer or a dead line pointer become unused we count only the
size of line pointer. The latter value we report could be non-zero
only when index cleanup is disabled; it counts the number of bytes of
dead line pointers we left. The advantage of this change is that user
can be aware of both how many bytes we freed and how many bytes we
left due to skipping index cleanup. User can be aware of the necessity
of doing index cleanup by seeing the latter value.

Also with this patch, we count only tuples that has been marked as
unused as deleted tuples. The action of replacing a dead root tuple
with a dead line pointer doesn't count for anything.  It would be
close to the meaning of "deleted tuples" and less confusion. We do
that when actually marking rather than when recording because we could
record and forget dead tuples.

Here is the sample of the report by VACUUM VERBOSE. I used the
following script and the vacuum report is changed by the patch.

* Script
DROP TABLE IF EXISTS test;
CREATE TABLE test (c int primary key, d int);
INSERT INTO test SELECT * FROM  generate_series(1,10000);
DELETE FROM test WHERE c < 2000;
VACUUM (INDEX_CLEANUP FALSE, VERBOSE) test;
VACUUM (INDEX_CLEANUP TRUE, VERBOSE) test;

* HEAD (when index cleanup is disabled)
INFO:  vacuuming "public.test"
INFO:  "test": found 1999 removable, 8001 nonremovable row versions in
45 out of 45 pages
DETAIL:  0 dead row versions cannot be removed yet, oldest xmin: 504
There were 0 unused item pointers.
Skipped 0 pages due to buffer pins, 0 frozen pages.
0 pages are entirely empty.
0 tuples and 1999 item identifiers are left as dead.
CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s.
VACUUM

* Patched (when index cleanup is disabled)
INFO:  vacuuming "public.test"
INFO:  "test": 55 kB freed, 7996 bytes can be freed after index
cleanup, table size: 360 kB
DETAIL:  found 8001 nonremovable row versions in 45 out of 45 pages.
0 dead row versions cannot be removed yet, oldest xmin: 1660
There were 0 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

* HEAD (when index cleanup is enabled)
INFO:  vacuuming "public.test"
INFO:  scanned index "test_pkey" to remove 1999 row versions
DETAIL:  CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s
INFO:  "test": removed 1999 row versions in 9 pages
DETAIL:  CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s
INFO:  index "test_pkey" now contains 8001 row versions in 30 pages
DETAIL:  1999 index row versions were removed.
5 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 0 removable, 91 nonremovable row versions in 10
out of 45 pages
DETAIL:  0 dead row versions cannot be removed yet, oldest xmin: 504
There were 0 unused item pointers.
Skipped 0 pages due to buffer pins, 0 frozen pages.
0 pages are entirely empty.
0 tuples and 0 item identifiers are left as dead.
CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s.
VACUUM

* Patched (when index cleanup is enabled)
INFO:  vacuuming "public.test"
INFO:  scanned index "test_pkey" to remove 1999 row versions
DETAIL:  CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s
INFO:  "test": removed 1999 row versions in 9 pages
DETAIL:  CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s
INFO:  index "test_pkey" now contains 8001 row versions in 30 pages
DETAIL:  1999 index row versions were removed.
5 index pages have been deleted, 0 are currently reusable.
CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s.
INFO:  "test": 7996 bytes freed, table size: 360 kB
DETAIL:  found 91 nonremovable row versions in 10 out of 45 pages.
0 dead row versions cannot be removed yet, oldest xmin: 1660
There were 0 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

The patch doesn't address the concern that Tom had, which is it might
not be safe in production to accumulate the dead line pointers without
limit when the reloption is set to false. I think this is a separate
issue from the above two issues so I'd like to discuss that after
these are solved.


Regards,

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

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

Re: New vacuum option to do only freezing

Robert Haas
On Tue, Apr 23, 2019 at 7:09 AM Masahiko Sawada <[hidden email]> wrote:
> For the second issue, I've changed lazy vacuum so that it reports both
> the number of kilobytes we freed and the number of kilobytes can be
> freed after index cleanup.

I am not very convinced that this reporting is in any useful to users.
Despite N kilobytes of tuples having been freed, the pages themselves
are still allocated and the actual ability to reuse that space may be
dependent on lots of factors that the user can't control like the
sizes of newly-inserted tuples and the degree to which the free space
map is accurate.

I feel like we're drifting off into inventing new kinds of reporting
here instead of focusing on fixing the reported defects of the
already-committed patch, but perhaps I am taking too narrow a view of
the situation.

--
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 Fri, Apr 26, 2019 at 12:11 AM Robert Haas <[hidden email]> wrote:

>
> On Tue, Apr 23, 2019 at 7:09 AM Masahiko Sawada <[hidden email]> wrote:
> > For the second issue, I've changed lazy vacuum so that it reports both
> > the number of kilobytes we freed and the number of kilobytes can be
> > freed after index cleanup.
>
> I am not very convinced that this reporting is in any useful to users.
> Despite N kilobytes of tuples having been freed, the pages themselves
> are still allocated and the actual ability to reuse that space may be
> dependent on lots of factors that the user can't control like the
> sizes of newly-inserted tuples and the degree to which the free space
> map is accurate.
Hmm, it's a term problem? The phrase 'x bytes vacuumed' would solve it?

>
> I feel like we're drifting off into inventing new kinds of reporting
> here instead of focusing on fixing the reported defects of the
> already-committed patch, but perhaps I am taking too narrow a view of
> the situation.

I should have divided the patches into two: fixing assertion error and
the reporting. I think we could think the latter issue also is a kind
of bug because it can report something like "1000 index tuples
vacuumed but 0 heap tuple vacuumed" in case where the vacuumed table
had only dead line pointers. Maybe I should add an another open item
for the latter.

Attached the split version patches.

Regards,

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

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

Re: New vacuum option to do only freezing

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

This thread is referenced an open item, and we ought to make some
progress on it.

On a more cosmetic note:

On 2019-04-16 09:10:19 -0700, Andres Freund wrote:

> On 2019-04-16 12:01:36 -0400, Tom Lane wrote:
> > (BTW, I don't understand why that code will throw "found xmin %u from
> > before relfrozenxid %u" if HeapTupleHeaderXminFrozen is true?  Shouldn't
> > the whole if-branch at lines 6113ff be skipped if xmin_frozen?)
>
> I *think* that just looks odd, but isn't actively wrong. That's because
> TransactionIdIsNormal() won't trigger, as:
>
> #define HeapTupleHeaderGetXmin(tup) \
> ( \
> HeapTupleHeaderXminFrozen(tup) ? \
> FrozenTransactionId : HeapTupleHeaderGetRawXmin(tup) \
> )
>
> which afaict makes
> xmin_frozen = ((xid == FrozenTransactionId) ||
>   HeapTupleHeaderXminFrozen(tuple));
> redundant.
>
> Looks like that was introduced relatively recently, in
>
> commit d2599ecfcc74fea9fad1720a70210a740c716730
> Author: Alvaro Herrera <[hidden email]>
> Date:   2018-05-04 15:24:44 -0300
>
>     Don't mark pages all-visible spuriously
>
>
> @@ -6814,6 +6815,8 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
>
>     /* Process xmin */
>     xid = HeapTupleHeaderGetXmin(tuple);
> +   xmin_frozen = ((xid == FrozenTransactionId) ||
> +                  HeapTupleHeaderXminFrozen(tuple));
>     if (TransactionIdIsNormal(xid))
>     {
>         if (TransactionIdPrecedes(xid, relfrozenxid))
> @@ -6832,9 +6835,8 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
>
>             frz->t_infomask |= HEAP_XMIN_FROZEN;
>             changed = true;
> +           xmin_frozen = true;
>         }
> -       else
> -           totally_frozen = false;
>     }

Alvaro, could we perhaps clean this up a bit? This is pretty confusing
looking.  I think this probably could just be changed to

        bool            xmin_frozen = false;

        xid = HeapTupleHeaderGetXmin(tuple);

        if (xid == FrozenTransactionId)
                xmin_frozen = true;
        else if (TransactionIdIsNormal(xid))

or somesuch.  There's no need to check for
HeapTupleHeaderXminFrozen(tuple) etc, because HeapTupleHeaderGetXmin()
already does so - and if it didn't, the issue Tom points out would be
problematic.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Andres Freund
In reply to this post by Michael Paquier-2
Hi,

On 2019-04-06 16:13:53 +0900, Michael Paquier wrote:
> 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.

Robert, what is your stance on this open item? It's been an open item
for about three weeks, without any progress.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Robert Haas
On Wed, May 1, 2019 at 12:14 PM Andres Freund <[hidden email]> wrote:

> On 2019-04-06 16:13:53 +0900, Michael Paquier wrote:
> > 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.
>
> Robert, what is your stance on this open item? It's been an open item
> for about three weeks, without any progress.

The actual bug in this patch needs to be fixed, but I see we have
another open item for that.  This open item, as I understand it, is
all about whether we should add another reloption so that you can
control this behavior separately for TOAST tables.  In my opinion,
that's not a critical change and the open item should be dropped, but
others might see it differently.

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


123456