New vacuum option to do only freezing

classic Classic list List threaded Threaded
118 messages Options
1 ... 3456
Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Robert Haas
On Tue, Apr 16, 2019 at 12:44 PM Tom Lane <[hidden email]> wrote:

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

After studying this more carefully, I agree.  You and Andres and
Alvaro are all correct, and I'm plain wrong.  Thanks for explaining.
I have committed a patch that changes the logic as per your
suggestion, and also removes nleft_dead_tuples and nleft_dead_itemids.

I'll reply separately to your other point about tups_vacuumed reporting.

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

Andres Freund
Hi,

On 2019-05-02 10:20:25 -0400, Robert Haas wrote:

> On Tue, Apr 16, 2019 at 12:44 PM Tom Lane <[hidden email]> wrote:
> > So after thinking about this a bit more ...
> > 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.
>
> After studying this more carefully, I agree.  You and Andres and
> Alvaro are all correct, and I'm plain wrong.  Thanks for explaining.
> I have committed a patch that changes the logic as per your
> suggestion, and also removes nleft_dead_tuples and nleft_dead_itemids.

It'd be good if somebody could make a pass over the safety mechanisms in
heap_prepare_freeze_tuple(). I added them at some point, after a data
corrupting bug related to freezing, but they really were more intended
as a secondary layer of defense, rather than the primary one.  My
understanding is still that we assume that we never should reach
heap_prepare_freeze_tuple() for something that is below the horizon,
even after this change, right?

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Robert Haas
In reply to this post by Tom Lane-2
On Tue, Apr 16, 2019 at 4:00 PM 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.
>
> I'm not especially wedded to either of these --- any other ideas?

I think it's almost impossible to have clear reporting here with only
a single counter.  There are two clearly-distinct cleanup operations
going on here: (1) removing tuples from pages, and (2) making dead
line pointers unused so that they can be reused for new tuples.  They
happen in equal quantity when there are no HOT updates: pruning makes
dead tuples into dead line pointers, and then index vacuuming allows
the dead line pointers to be set unused.  But if there are HOT
updates, intermediate tuples in each HOT chain are removed from the
page but the line pointers are directly set to unused, so VACUUM could
remove a lot of tuples but not need to make very many dead line
pointers unused.  On the other hand, the opposite could also happen:
maybe lots of single-page HOT-pruning has happened prior to VACUUM, so
VACUUM has lots of dead line pointers to make unused but removes very
few tuples because that's already been done.

For the most part, tups_vacuumed seems to be intending to count #1
rather than #2. While the comments for heap_page_prune and
heap_prune_chain are not as clear as might be desirable, it appears to
me that those functions are counting tuples removed from a page,
ignoring everything that might happen to line pointers -- so using the
return value of this function is consistent with wanting to count #1.
However, there's one place that seems slightly unclear about this,
namely this comment:

            /*
             * 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 we're counting tuples removed from pages, then it's not merely that
we would be double-counting, but that we would be counting completely
the wrong thing.  However, as far as I can see, that's just an issue
with the comments; the code is in all cases counting tuple removals,
not dead line pointers marked unused.

If I understand correctly, your first proposal amounts to redefining
tups_vacuumed to count #2 rather than #1, and your second proposal
amounts to making tups_vacuumed count #1 + #2 rather than #1.  I
suggest a third option: have two counters.  tups_vacuum can continue
to count #1, with just a comment adjustment. And then we can add a
second counter which is incremented every time lazy_vacuum_page does
ItemIdSetUnused, which will count #2.

Thoughts?

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

Andres Freund
On 2019-05-02 11:09:10 -0400, Robert Haas wrote:
> If I understand correctly, your first proposal amounts to redefining
> tups_vacuumed to count #2 rather than #1, and your second proposal
> amounts to making tups_vacuumed count #1 + #2 rather than #1.  I
> suggest a third option: have two counters.

+1


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Robert Haas
In reply to this post by Andres Freund
On Thu, May 2, 2019 at 10:28 AM Andres Freund <[hidden email]> wrote:
> It'd be good if somebody could make a pass over the safety mechanisms in
> heap_prepare_freeze_tuple(). I added them at some point, after a data
> corrupting bug related to freezing, but they really were more intended
> as a secondary layer of defense, rather than the primary one.  My
> understanding is still that we assume that we never should reach
> heap_prepare_freeze_tuple() for something that is below the horizon,
> even after this change, right?

I think so.  This code is hit if the tuple wasn't dead yet at the time
that heap_page_prune() decided not to prune it, but it is dead by the
time we retest the tuple status.  But the same value of OldestXmin was
used for both checks, so the only way that can really happen is if the
XID in the tuple header was running before and is no longer running.
However, if the XID was running at the time that heap_page_prune()
ran, than OldestXmin certainly can't be newer than that XID.  And
therefore the value we're intending to set for relfrozenxid has surely
got to be older, so we could hardly prune using OldestXmin as the
threshold and then relfrozenxid to a newer XID.

Actually, I now believe that my original concern here was exactly
backwards.  Prior to the logic change in this commit, with index
vacuum disabled, a tuple that becomes dead between the
heap_page_prune() check and the lazy_scan_heap() check would have
followed the tupgone = true path.  That would cause it to be treated
as if the second vacuum pass were going to remove it - i.e. not
frozen.  But with index cleanup disabled, there will never be a second
vacuum pass.  So a tuple that was actually being kept was not sent to
heap_prepare_freeze_tuple() with, basically, no justification.
Imagine for example that the tuple was not old in terms of its XID
age, but its MXID age was somehow ancient.  Then we'd fail to freeze
it on the theory that it was going to be removed, but yet not remove
it.  Oops.  The revised logic - which is as per Tom's suggestion -
does the right thing, which is treat the tuple as one we've chosen to
keep.

While looking at this code, I think I may have spotted another bug, or
at least a near-bug. lazy_record_dead_tuple() thinks it's OK to just
forget about dead tuples if there's not enough memory, which I think
is OK in the normal case where the dead tuple has been truncated to a
dead line pointer.  But in the tupgone = true case it's NOT ok,
because in that case we're leaving behind not just a dead line pointer
but an actual tuple which we have declined to freeze on the assumption
that it will be removed later.  But if lazy_record_dead_tuple()
forgets about it, then it won't be removed later.  That could lead to
tuples remaining that precede the freeze horizons.  The reason why
this may be only a near-bug is that we seem to try pretty hard to make
sure that we'll never call that function in the first place without
enough space being available.  Still, it seems to me that it would be
safer to change the code to look like:

if (vacrelstats->num_dead_tuples >= vacrelstats->max_dead_tuples)
    elog(ERROR, "oh crap");

--
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
In reply to this post by Andres Freund
On 2019-May-01, Andres Freund wrote:

> 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.
Ah, yeah, that's simpler.  I would like to introduce a couple of very
minor changes to the proposed style, per the attached.

* don't initialize xmin_frozen at all; rather, only set its value to the
correct one when we have determined what it is.  Doing premature
initialization is what led to some of those old bugs, so I prefer not to
do it.

* Handle the BootstrapXid and InvalidXid cases explicitly, by setting
xmin_frozen to true when xmin is not normal.  After all, those XID
values do not need any freezing.

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

0001-heap_prepare_freeze_tuple-Simplify-coding.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Alvaro Herrera-9
Pushed.  I added one comment to explain xmin_frozen also, which
otherwise seemed a bit mysterious.  I did not backpatch, though, so
9.6-11 are a bit different, but I'm not sure it's a good idea at this
point, though it should be pretty innocuous.

--
Á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 Fri, May 3, 2019 at 12:09 AM Robert Haas <[hidden email]> wrote:

>
> On Tue, Apr 16, 2019 at 4:00 PM 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.
> >
> > I'm not especially wedded to either of these --- any other ideas?
>
> I think it's almost impossible to have clear reporting here with only
> a single counter.  There are two clearly-distinct cleanup operations
> going on here: (1) removing tuples from pages, and (2) making dead
> line pointers unused so that they can be reused for new tuples.  They
> happen in equal quantity when there are no HOT updates: pruning makes
> dead tuples into dead line pointers, and then index vacuuming allows
> the dead line pointers to be set unused.  But if there are HOT
> updates, intermediate tuples in each HOT chain are removed from the
> page but the line pointers are directly set to unused, so VACUUM could
> remove a lot of tuples but not need to make very many dead line
> pointers unused.  On the other hand, the opposite could also happen:
> maybe lots of single-page HOT-pruning has happened prior to VACUUM, so
> VACUUM has lots of dead line pointers to make unused but removes very
> few tuples because that's already been done.
>
> For the most part, tups_vacuumed seems to be intending to count #1
> rather than #2. While the comments for heap_page_prune and
> heap_prune_chain are not as clear as might be desirable, it appears to
> me that those functions are counting tuples removed from a page,
> ignoring everything that might happen to line pointers -- so using the
> return value of this function is consistent with wanting to count #1.
> However, there's one place that seems slightly unclear about this,
> namely this comment:
>
>             /*
>              * 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 we're counting tuples removed from pages, then it's not merely that
> we would be double-counting, but that we would be counting completely
> the wrong thing.  However, as far as I can see, that's just an issue
> with the comments; the code is in all cases counting tuple removals,
> not dead line pointers marked unused.
>
> If I understand correctly, your first proposal amounts to redefining
> tups_vacuumed to count #2 rather than #1, and your second proposal
> amounts to making tups_vacuumed count #1 + #2 rather than #1.  I
> suggest a third option: have two counters.  tups_vacuum can continue
> to count #1, with just a comment adjustment. And then we can add a
> second counter which is incremented every time lazy_vacuum_page does
> ItemIdSetUnused, which will count #2.
>
> Thoughts?

I agree to have an another counter. Please note that non-HOT-updated
tuples that became dead after hot pruning (that is 'tupgone' tuples)
are changed to unused directly in lazy_page_vacuum. Therefore we would
need not to increment tups_vacuumed in tupgone case if we increment
the new counter in lazy_page_vacuum.

For the contents of vacuum verbose report, it could be worth to
discuss whether reporting the number of deleted line pointers would be
helpful for users. The reporting the number of line pointers we left
might be more helpful for users.

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
In reply to this post by Robert Haas
On Thu, May 2, 2019 at 1:39 AM Robert Haas <[hidden email]> wrote:

>
> 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.
I agree that this item is neither critical and bug. But this is an
(my) oversight and is a small patch and I think there is no specific
reason why we don't dare to include this in 12. So if this patch could
get reviewed enough I think we can have it in 12. Since the previous
patch conflicts with current HEAD I've attached the rebased version
patch.

Regards,

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

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

Re: New vacuum option to do only freezing

Andres Freund
Hi,

On 2019-05-09 14:14:20 +0900, Masahiko Sawada wrote:
> I agree that this item is neither critical and bug. But this is an
> (my) oversight and is a small patch and I think there is no specific
> reason why we don't dare to include this in 12. So if this patch could
> get reviewed enough I think we can have it in 12. Since the previous
> patch conflicts with current HEAD I've attached the rebased version
> patch.

Robert, this indeed looks near trivial. What do you think?

> diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
> index 44a61ef..1e1b0e8 100644
> --- a/doc/src/sgml/ref/create_table.sgml
> +++ b/doc/src/sgml/ref/create_table.sgml
> @@ -1406,7 +1406,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
>     </varlistentry>
>  
>     <varlistentry id="reloption-vacuum-index-cleanup" xreflabel="vacuum_index_cleanup">
> -    <term><literal>vacuum_index_cleanup</literal> (<type>boolean</type>)
> +    <term><literal>vacuum_index_cleanup</literal>, <literal>toast.vacuum_index_cleanup</literal> (<type>boolean</type>)
>      <indexterm>
>       <primary><varname>vacuum_index_cleanup</varname> storage parameter</primary>
>      </indexterm>
> diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
> index cfbabb5..022b3a0 100644
> --- a/src/backend/access/common/reloptions.c
> +++ b/src/backend/access/common/reloptions.c
> @@ -147,7 +147,7 @@ static relopt_bool boolRelOpts[] =
>   {
>   "vacuum_index_cleanup",
>   "Enables index vacuuming and index cleanup",
> - RELOPT_KIND_HEAP,
> + RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
>   ShareUpdateExclusiveLock
>   },
>   true
> diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
> index e4c03de..2379b3d 100644
> --- a/src/bin/psql/tab-complete.c
> +++ b/src/bin/psql/tab-complete.c
> @@ -1056,6 +1056,7 @@ static const char *const table_storage_parameters[] = {
>   "toast.autovacuum_vacuum_scale_factor",
>   "toast.autovacuum_vacuum_threshold",
>   "toast.log_autovacuum_min_duration",
> + "toast.vacuum_index_clenaup",
>   "toast.vacuum_truncate",
>   "toast_tuple_target",
>   "user_catalog_table",

typo.

Sawada-san, it'd be good if you could add at least some minimal tests in
the style of the no_index_cleanup test in vacuum.sql.

- Andres


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Tom Lane-2
Andres Freund <[hidden email]> writes:
> Robert, this indeed looks near trivial. What do you think?

> On 2019-05-09 14:14:20 +0900, Masahiko Sawada wrote:
>> + "toast.vacuum_index_clenaup",

Not trivial enough to not have typos, apparently.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Masahiko Sawada
In reply to this post by Andres Freund
On Sat, May 18, 2019 at 5:55 AM Andres Freund <[hidden email]> wrote:

>
> Hi,
>
> On 2019-05-09 14:14:20 +0900, Masahiko Sawada wrote:
> > I agree that this item is neither critical and bug. But this is an
> > (my) oversight and is a small patch and I think there is no specific
> > reason why we don't dare to include this in 12. So if this patch could
> > get reviewed enough I think we can have it in 12. Since the previous
> > patch conflicts with current HEAD I've attached the rebased version
> > patch.
>
> Robert, this indeed looks near trivial. What do you think?
>
> > diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
> > index 44a61ef..1e1b0e8 100644
> > --- a/doc/src/sgml/ref/create_table.sgml
> > +++ b/doc/src/sgml/ref/create_table.sgml
> > @@ -1406,7 +1406,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
> >     </varlistentry>
> >
> >     <varlistentry id="reloption-vacuum-index-cleanup" xreflabel="vacuum_index_cleanup">
> > -    <term><literal>vacuum_index_cleanup</literal> (<type>boolean</type>)
> > +    <term><literal>vacuum_index_cleanup</literal>, <literal>toast.vacuum_index_cleanup</literal> (<type>boolean</type>)
> >      <indexterm>
> >       <primary><varname>vacuum_index_cleanup</varname> storage parameter</primary>
> >      </indexterm>
> > diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
> > index cfbabb5..022b3a0 100644
> > --- a/src/backend/access/common/reloptions.c
> > +++ b/src/backend/access/common/reloptions.c
> > @@ -147,7 +147,7 @@ static relopt_bool boolRelOpts[] =
> >               {
> >                       "vacuum_index_cleanup",
> >                       "Enables index vacuuming and index cleanup",
> > -                     RELOPT_KIND_HEAP,
> > +                     RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
> >                       ShareUpdateExclusiveLock
> >               },
> >               true
> > diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
> > index e4c03de..2379b3d 100644
> > --- a/src/bin/psql/tab-complete.c
> > +++ b/src/bin/psql/tab-complete.c
> > @@ -1056,6 +1056,7 @@ static const char *const table_storage_parameters[] = {
> >       "toast.autovacuum_vacuum_scale_factor",
> >       "toast.autovacuum_vacuum_threshold",
> >       "toast.log_autovacuum_min_duration",
> > +     "toast.vacuum_index_clenaup",
> >       "toast.vacuum_truncate",
> >       "toast_tuple_target",
> >       "user_catalog_table",
>
> typo.
>
> Sawada-san, it'd be good if you could add at least some minimal tests in
> the style of the no_index_cleanup test in vacuum.sql.
>
Thank you for comments. Attached updated version patch.

Regards,

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

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

Re: New vacuum option to do only freezing

Robert Haas
In reply to this post by Andres Freund
On Fri, May 17, 2019 at 4:55 PM Andres Freund <[hidden email]> wrote:
> Robert, this indeed looks near trivial. What do you think?

Hmm, yeah, I guess that'd be OK.

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

Michael Paquier-2
In reply to this post by Masahiko Sawada
On Mon, May 20, 2019 at 11:43:19AM +0900, Masahiko Sawada wrote:
> Thank you for comments. Attached updated version patch.

This is an open item present for quite some time, so I have looked at
the patch.  The base patch is fine.

+INSERT INTO no_index_cleanup(i, t) VALUES(1, repeat('1234567890',30000));
Do we really need a string as long as that?

Is actually the existing set of tests that helpful?  We now have only
two VACUUM queries which run on no_index_cleanup, both of them using
FULL so the reloption as well as the value of INDEX_CLEANUP are
ignored.  Wouldn't it be better to redesign a bit the tests with more
combinations of options like that?
-- Toast inherit the value from the table
ALTER TABLE no_index_cleanup SET (vacuum_index_cleanup = false);
-- Value directly defined for both
ALTER TABLE no_index_cleanup SET (vacuum_index_cleanup = false,
    toast.vacuum_index_cleanup = true);
ALTER TABLE no_index_cleanup SET (vacuum_index_cleanup = true,
    toast.vacuum_index_cleanup = false);

It seems to me that we'd want tests to make sure that indexes are
actually cleaned up, where pageinspect could prove to be useful.
--
Michael

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

Re: New vacuum option to do only freezing

Peter Geoghegan-4
On Tue, Jun 18, 2019 at 10:39 PM Michael Paquier <[hidden email]> wrote:
> +INSERT INTO no_index_cleanup(i, t) VALUES(1, repeat('1234567890',30000));
> Do we really need a string as long as that?

Specifying EXTERNAL storage might make things easier. I have used
PLAIN storage to test the 1/3 of a page restriction within nbtree, and
to test a bug in amcheck that was related to TOAST compression.

> It seems to me that we'd want tests to make sure that indexes are
> actually cleaned up, where pageinspect could prove to be useful.

That definitely seems preferable, but it'll be a bit tricky to do it
in a way that doesn't run into buildfarm issues due to alignment. I
suggest an index on a text column to avoid problems.

--
Peter Geoghegan


Reply | Threaded
Open this post in threaded view
|

Re: New vacuum option to do only freezing

Michael Paquier-2
On Wed, Jun 19, 2019 at 12:51:41PM -0700, Peter Geoghegan wrote:
> On Tue, Jun 18, 2019 at 10:39 PM Michael Paquier <[hidden email]> wrote:
>> +INSERT INTO no_index_cleanup(i, t) VALUES(1, repeat('1234567890',30000));
>> Do we really need a string as long as that?
>
> Specifying EXTERNAL storage might make things easier. I have used
> PLAIN storage to test the 1/3 of a page restriction within nbtree, and
> to test a bug in amcheck that was related to TOAST compression.

Ah, good point here.  That makes sense.

>> It seems to me that we'd want tests to make sure that indexes are
>> actually cleaned up, where pageinspect could prove to be useful.
>
> That definitely seems preferable, but it'll be a bit tricky to do it
> in a way that doesn't run into buildfarm issues due to alignment. I
> suggest an index on a text column to avoid problems.

I am not completely sure how tricky that may be, so I'll believe you
on this one :)

So, to keep things simple and if we want to get something into v12, I
would suggest to just stress more combinations even if the changes are
not entirely visible yet.  If we get a couple of queries to run with
the option disabled on the table, its toast or both by truncating and
filling in the table in-between, we may be able to catch some issues
by stressing those code paths.

And I finish with the attached.  Thoughts?
--
Michael

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

Re: New vacuum option to do only freezing

Michael Paquier-2
On Thu, Jun 20, 2019 at 03:50:32PM +0900, Michael Paquier wrote:
> And I finish with the attached.  Thoughts?

So, are there any objections with this patch?  Or do people think that
it's too late for v12 and that it is better to wait until v13 opens
for business?
--
Michael

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

Re: New vacuum option to do only freezing

Michael Paquier-2
On Sun, Jun 23, 2019 at 10:29:25PM +0900, Michael Paquier wrote:
> So, are there any objections with this patch?  Or do people think that
> it's too late for v12 and that it is better to wait until v13 opens
> for business?

Committed, and open item closed.
--
Michael

signature.asc (849 bytes) Download Attachment
1 ... 3456