Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On 10/9/18 5:15 AM, Kyotaro HORIGUCHI wrote: > At Fri, 5 Oct 2018 15:35:04 +0900, Michael Paquier > wrote in <20181005063504.gb14...@paquier.xyz> >> On Fri, Oct 05, 2018 at 12:16:03PM +0900, Michael Paquier wrote: >>> So, I have come back to this stuff, and finished with the attached >>> instead, so as the assertion is in a single place. I find that >>> clearer. The comments have also been improved. Thoughts? >> And so... I have been looking at committing this thing, and while >> testing in-depth I have been able to trigger a case where an autovacuum >> has been able to be not aggressive but anti-wraparound, which is exactly >> what should not be possible, no? I have simply created an instance with >> autovacuum_freeze_max_age = 20, then ran pgbench with >> autovacuum_freeze_table_age=20 set for each table, and also ran >> installcheck-world in parallel. This has been able to trigger the >> assertion pretty quickly. > I investigated it and in short, it can happen. > > It is a kind of race consdition between two autovacuum > processes. do_autovacuum() looks into pg_class (using a snapshot) > and vacuum_set_xid_limits() looks into relcache. If concurrent > vacuum happens and one has finished the relation, another gets > relcache invalidation and relfrozenxid is updated. If this > happens between do_autovacuum() and vacuum_set_xid_limits(), the > latter sees newer relfrozenxid than the former. The problem > happens when it moves by more than 5% of > autovacuum_freeze_max_age. > > If lazy_vacuum_rel() sees the situation, the relation is already > aggressively vacuumed by a cocurrent worker. We can just ingore > the state safely but also we know that the vacuum is useless. > > 1. Just allow the case there (and add comment). >Causes redundant anti-wraparound vacuum. > > 2. Skip the relation by the condition. > >I think that we can safely skip the relation in the >case. (attached) > > 3. Ensure that do_autovacuum always sees the same relfrozenxid >with vacuum_set_xid_limits(). > > 4. Prevent concurrent acuuming of the same relation rigorously, > somehow. > > Thoughts? > I notice that this seems never to have been acted on. I think we should apply this and remove the (confusing) message setting for the case we'll now be avoiding. If not we should at least comment there that this is a case we only expect to see in pathological cases. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Fri, Mar 08, 2019 at 05:05:53PM -0500, Andrew Dunstan wrote: > I notice that this seems never to have been acted on. I think we should > apply this and remove the (confusing) message setting for the case we'll > now be avoiding. If not we should at least comment there that this is a > case we only expect to see in pathological cases. Sorry for dropping the ball, I would have assumed that Robert would handle it as he is at the origin of the introduction of the aggressive option via fd31cd26. +elog(DEBUG1, "relation %d has been vacuumd ocncurrently, skip", The proposed patch has two typos in two words. I am adding an open item about that. I think I could commit the patch, but I need to study it a bit more first. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Sat, Mar 09, 2019 at 10:15:37AM +0900, Michael Paquier wrote: > I am adding an open item about that. I think I could commit the > patch, but I need to study it a bit more first. So, coming back to this thread, and studying the problem again, it looks that the diagnostic that a non-aggressive, anti-wraparound vacuum could be triggered because the worker sees trouble in the force because of some activity happening in parallel. Hence, if we face this case, it looks right to skip the vacuum for this relation. Attached is an updated patch with a better error message, more comments, and the removal of the anti-wraparound non-aggressive log which was added in 28a8fa9. The error message could be better I guess. Suggestions are welcome. Thoughts? -- Michael diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 5c554f9465..82be8c81f3 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -248,6 +248,25 @@ heap_vacuum_rel(Relation onerel, VacuumParams *params, if (params->options & VACOPT_DISABLE_PAGE_SKIPPING) aggressive = true; + /* + * When running an anti-wraparound vacuum, we expect relfrozenxid to be + * old enough so as aggressive is always set. If this is not the case, + * it could be possible that another concurrent vacuum process has done + * the work for this relation so that relfrozenxid in relcache has + * already been moved forward enough, causing this vacuum run to be + * non-aggressive. If that happens, note that this relation no longer + * needs to be vacuumed, so just skip it. + */ + if (params->is_wraparound && !aggressive) + { + ereport(LOG, +(errmsg_internal("found vacuum to prevent wraparound of table \"%s.%s.%s\" to be not aggressive, so skipping", + get_database_name(MyDatabaseId), + get_namespace_name(RelationGetNamespace(onerel)), + RelationGetRelationName(onerel; + return; + } + vacrelstats = (LVRelStats *) palloc0(sizeof(LVRelStats)); vacrelstats->old_rel_pages = onerel->rd_rel->relpages; @@ -375,10 +394,9 @@ heap_vacuum_rel(Relation onerel, VacuumParams *params, initStringInfo(&buf); if (params->is_wraparound) { -if (aggressive) - msgfmt = _("automatic aggressive vacuum to prevent wraparound of table \"%s.%s.%s\": index scans: %d\n"); -else - msgfmt = _("automatic vacuum to prevent wraparound of table \"%s.%s.%s\": index scans: %d\n"); +/* an anti-wraparound vacuum has to be aggressive */ +Assert(aggressive); +msgfmt = _("automatic aggressive vacuum to prevent wraparound of table \"%s.%s.%s\": index scans: %d\n"); } else { signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On 3/29/19 7:51 AM, Michael Paquier wrote: > On Sat, Mar 09, 2019 at 10:15:37AM +0900, Michael Paquier wrote: >> I am adding an open item about that. I think I could commit the >> patch, but I need to study it a bit more first. > So, coming back to this thread, and studying the problem again, it > looks that the diagnostic that a non-aggressive, anti-wraparound > vacuum could be triggered because the worker sees trouble in the > force because of some activity happening in parallel. Hence, if we > face this case, it looks right to skip the vacuum for this relation. > > Attached is an updated patch with a better error message, more > comments, and the removal of the anti-wraparound non-aggressive log > which was added in 28a8fa9. The error message could be better I > guess. Suggestions are welcome. > > Thoughts? + (errmsg_internal("found vacuum to prevent wraparound of table \"%s.%s.%s\" to be not aggressive, so skipping", This might convey something to hackers, but I doubt it will convey much to regular users. Perhaps something like "skipping redundant anti-wraparound vacuum of table ..." would be better. The comment is also a bit too tentative. Perhaps something like this would do: Normally the relfrozenxid for an anti-wraparound vacuum will be old enough to force an aggressive vacuum. However, a concurrent vacuum might have already done this work that the relfroxzenxid in relcache has been updated. If that happens this vacuum is redundant, so skip it. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Fri, Mar 29, 2019 at 09:11:47AM -0400, Andrew Dunstan wrote: > + (errmsg_internal("found vacuum to prevent wraparound of > table \"%s.%s.%s\" to be not aggressive, so skipping", > > This might convey something to hackers, but I doubt it will convey much > to regular users. Perhaps something like "skipping redundant > anti-wraparound vacuum of table ..." would be better. "skipping redundant" is much better. > The comment is also a bit too tentative. Perhaps something like this > would do: > > Normally the relfrozenxid for an anti-wraparound vacuum will be old > enough to force an aggressive vacuum. However, a concurrent vacuum > might have already done this work that the relfroxzenxid in relcache > has been updated. If that happens this vacuum is redundant, so skip it. That works for me. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On 2019-Mar-29, Michael Paquier wrote: > On Fri, Mar 29, 2019 at 09:11:47AM -0400, Andrew Dunstan wrote: > > + (errmsg_internal("found vacuum to prevent wraparound of > > table \"%s.%s.%s\" to be not aggressive, so skipping", > > > > This might convey something to hackers, but I doubt it will convey much > > to regular users. Perhaps something like "skipping redundant > > anti-wraparound vacuum of table ..." would be better. > > "skipping redundant" is much better. Yeah, that looks good to me too. I wonder if we really need it as LOG though; we don't say anything for actions unless they take more than the min duration, so why say something for a no-op that takes almost no time? Maybe make it DEBUG1. > > The comment is also a bit too tentative. Perhaps something like this > > would do: > > > > Normally the relfrozenxid for an anti-wraparound vacuum will be old > > enough to force an aggressive vacuum. However, a concurrent vacuum > > might have already done this work that the relfroxzenxid in relcache > > has been updated. If that happens this vacuum is redundant, so skip it. > > That works for me. s/relfroxzenxid/relfrozenxid/ -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Fri, Mar 29, 2019 at 11:22:55AM -0300, Alvaro Herrera wrote: > Yeah, that looks good to me too. I wonder if we really need it as LOG > though; we don't say anything for actions unless they take more than the > min duration, so why say something for a no-op that takes almost no time? > Maybe make it DEBUG1. I think that this does not justify a WARNING, as that's harmless for the user even if we use WARNING for other skips (see vacuum_is_relation_owner). However DEBUG1 is also too low in my opinion as this log can be used as an indicator that autovacuum is too much aggressive because there are too many workers for example. I have seen that matter in some CPU-bound environments. I won't fight hard if the consensus is to use DEBUG1 though. So, more opinions? Andrew perhaps? > s/relfroxzenxid/relfrozenxid/ Sure. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On 3/29/19 9:08 PM, Michael Paquier wrote: > On Fri, Mar 29, 2019 at 11:22:55AM -0300, Alvaro Herrera wrote: >> Yeah, that looks good to me too. I wonder if we really need it as LOG >> though; we don't say anything for actions unless they take more than the >> min duration, so why say something for a no-op that takes almost no time? >> Maybe make it DEBUG1. > I think that this does not justify a WARNING, as that's harmless for > the user even if we use WARNING for other skips (see > vacuum_is_relation_owner). However DEBUG1 is also too low in my > opinion as this log can be used as an indicator that autovacuum is too > much aggressive because there are too many workers for example. I > have seen that matter in some CPU-bound environments. I won't fight > hard if the consensus is to use DEBUG1 though. So, more opinions? > Andrew perhaps? > > It's really just a matter of housekeeping as I see it, so probably DEBUG1 is right. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Sat, Mar 30, 2019 at 08:16:21AM -0400, Andrew Dunstan wrote: > It's really just a matter of housekeeping as I see it, so probably > DEBUG1 is right. Okay, I'll use that then. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Sat, Mar 30, 2019 at 10:12:33PM +0900, Michael Paquier wrote: > Okay, I'll use that then. And finally committed. I have changed the debug1 message so as "to prevent wraparound" is used instead of "anti-wraparound". I have noticed something which was also missing from all the patches proposed on this thread: a reset of the progress state was not done, and we need to call pgstat_progress_end_command() in order to do that. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Fri, Sep 14, 2018 at 11:35 AM, Alvaro Herrera wrote: > On 2018-Sep-13, Michael Paquier wrote: >> Improve autovacuum logging for aggressive and anti-wraparound runs >> >> A log message was being generated when log_min_duration is reached for >> autovacuum on a given relation to indicate if it was an aggressive run, >> and missed the point of mentioning if it is doing an anti-wrapround >> run. The log message generated is improved so as one, both or no extra >> details are added depending on the option set. > > Hmm, can a for-wraparound vacuum really not be aggressive? I think one > of those four cases is really dead code. My first question was whether TWO of them were dead code ... isn't an aggressive vacuum to prevent wraparound, and a vacuum to prevent wraparound aggressive? I can't figure out what this is giving us that we didn't have before. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
Hello, Robert > My first question was whether TWO of them were dead code ... isn't an > aggressive vacuum to prevent wraparound, and a vacuum to prevent > wraparound aggressive? Maybe i am wrong, aggressive autovacuum was your commit. Message split was in b55509332f50f998b6e8b3830a51c5b9d8f666aa Aggressive autovacuum was in fd31cd265138019d9b5fe53043670898bc9f If aggressive really is wraparound without difference - i think we need refactor this code, it is difficult have two different flags for same purpose. But as far i can see it is possible have aggressive non-wraparound vacuum. One important difference - regular and aggressive regular can be canceled by backend,.wraparound autovacuum can not. (by checking PROC_VACUUM_FOR_WRAPAROUND in src/backend/storage/lmgr/proc.c ) regards, Sergei
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
Hi, On 2018-09-21 20:38:16 +0300, Sergei Kornilov wrote: > > My first question was whether TWO of them were dead code ... isn't an > > aggressive vacuum to prevent wraparound, and a vacuum to prevent > > wraparound aggressive? > Maybe i am wrong, aggressive autovacuum was your commit. > Message split was in b55509332f50f998b6e8b3830a51c5b9d8f666aa > Aggressive autovacuum was in fd31cd265138019d9b5fe53043670898bc9f > > If aggressive really is wraparound without difference - i think we need > refactor this code, it is difficult have two different flags for same purpose. > > But as far i can see it is possible have aggressive non-wraparound vacuum. > One important difference - regular and aggressive regular can be canceled by > backend,.wraparound autovacuum can not. (by checking > PROC_VACUUM_FOR_WRAPAROUND in src/backend/storage/lmgr/proc.c ) Yes, without checking the code, they should be different. Aggressive is controlled by vacuum_freeze_table_age whereas anti-wrap is controlled by autovacuum_freeze_max_age (but also implies aggressive). Greetings, Andres Freund
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On 2018-09-24 18:25:46 +, Nasby, Jim wrote: > > > On Sep 21, 2018, at 12:43 PM, Andres Freund wrote: > > > >> But as far i can see it is possible have aggressive non-wraparound vacuum. > >> One important difference - regular and aggressive regular can be canceled > >> by backend,.wraparound autovacuum can not. (by checking > >> PROC_VACUUM_FOR_WRAPAROUND in src/backend/storage/lmgr/proc.c ) > > > > Yes, without checking the code, they should be different. Aggressive is > > controlled by vacuum_freeze_table_age whereas anti-wrap is controlled by > > autovacuum_freeze_max_age (but also implies aggressive). > > Right, except that by the time you get into the vacuum code itself nothing > should really care about that difference. AFAICT, the only thing > is_wraparound is being used for is to set MyPgXact->vacuumFlags |= > PROC_VACUUM_FOR_WRAPAROUND, which prevents the deadlock detector from killing > an autovac process that’s trying to prevent a wraparound. I think it’d be > clearer to remove is_wraparound and move the check from vacuum_rel() into > lazy_vacuum_rel() (which is where the limits for HeapTupleSatisfiesVacuum get > determined). Something like the attached. I'm very doubtful this is an improvement. Especially with the upcoming pluggable storage work making vacuumlazy.c heap specific, while vacuum.c stays generic. The concept of something like PROC_VACUUM_FOR_WRAPAROUND, should imo not be pushed down that much (even if criteria for it might). Greetings, Andres Freund
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
> On Sep 24, 2018, at 1:29 PM, Andres Freund wrote: > > I'm very doubtful this is an improvement. Especially with the upcoming > pluggable storage work making vacuumlazy.c heap specific, while vacuum.c > stays generic. The concept of something like > PROC_VACUUM_FOR_WRAPAROUND, should imo not be pushed down that much > (even if criteria for it might). That’s already a problem since vacuum logging is spread all over while autovac logging is not. Perhaps there needs to be some sort of vacuum_log() function that immediately provides output for manual vacuums, but aggregates output for autovac. AFAIK that’s the only real reason for autovac logging being a special case today.
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
Hi > An autovacuum can't be just aggressive; it's either anti-wraparound or normal. But autovacuum _can_ be aggressive and not anti-wraparound. I build current master and can see 3 different line types: 2018-09-24 23:47:31.500 MSK 27939 @ from [vxid:4/272032 txid:0] [] LOG: automatic aggressive vacuum of table "postgres.public.foo": index scans: 0 2018-09-24 23:49:27.892 MSK 28333 @ from [vxid:4/284297 txid:0] [] LOG: automatic aggressive vacuum to prevent wraparound of table "postgres.public.foo": index scans: 0 2018-09-24 23:49:29.093 MSK 28337 @ from [vxid:4/284412 txid:0] [] LOG: automatic vacuum of table "postgres.public.foo": index scans: 0 regards, Sergei
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On 2018-Sep-24, Sergei Kornilov wrote: > Hi > > > An autovacuum can't be just aggressive; it's either anti-wraparound or > > normal. > But autovacuum _can_ be aggressive and not anti-wraparound. > I build current master and can see 3 different line types: > 2018-09-24 23:47:31.500 MSK 27939 @ from [vxid:4/272032 txid:0] [] LOG: > automatic aggressive vacuum of table "postgres.public.foo": index scans: 0 > 2018-09-24 23:49:27.892 MSK 28333 @ from [vxid:4/284297 txid:0] [] LOG: > automatic aggressive vacuum to prevent wraparound of table > "postgres.public.foo": index scans: 0 > 2018-09-24 23:49:29.093 MSK 28337 @ from [vxid:4/284412 txid:0] [] LOG: > automatic vacuum of table "postgres.public.foo": index scans: 0 Exactly. It cannot be anti-wraparound and not aggressive, which is the line type not shown. "Aggressive" means it scans all pages; "anti-wraparound" means it does not let itself be cancelled because of another process waiting for a lock on the table. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Tue, Sep 25, 2018 at 6:11 AM Alvaro Herrera wrote: > > On 2018-Sep-24, Sergei Kornilov wrote: > > > Hi > > > > > An autovacuum can't be just aggressive; it's either anti-wraparound or > > > normal. > > But autovacuum _can_ be aggressive and not anti-wraparound. > > I build current master and can see 3 different line types: > > 2018-09-24 23:47:31.500 MSK 27939 @ from [vxid:4/272032 txid:0] [] LOG: > > automatic aggressive vacuum of table "postgres.public.foo": index scans: 0 > > 2018-09-24 23:49:27.892 MSK 28333 @ from [vxid:4/284297 txid:0] [] LOG: > > automatic aggressive vacuum to prevent wraparound of table > > "postgres.public.foo": index scans: 0 > > 2018-09-24 23:49:29.093 MSK 28337 @ from [vxid:4/284412 txid:0] [] LOG: > > automatic vacuum of table "postgres.public.foo": index scans: 0 > > Exactly. > > It cannot be anti-wraparound and not aggressive, which is the line type > not shown. > > "Aggressive" means it scans all pages; "anti-wraparound" means it does > not let itself be cancelled because of another process waiting for a > lock on the table. > I agree. Can we fix this simply by the attached patch? Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center fix_autovacuum_log.patch Description: Binary data
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Fri, Sep 28, 2018 at 01:53:14PM +0900, Masahiko Sawada wrote: > I agree. Can we fix this simply by the attached patch? Thanks for sending a patch. +/* autovacuum cannot be anti-wraparound and not aggressive vacuum */ +Assert(aggressive); +msgfmt = _("automatic aggressive vacuum to prevent wraparound of table \"%s.%s.%s\": index scans: %d\n"); While adding this comment in lazy_vacuum_rel() is adapted, I think that we ought to make the assertion check more aggressive by not having it depend on if log_min_duration is set or not. So why not moving that to a place a bit higher, where aggressive gets defined? -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Tue, Oct 2, 2018 at 9:11 AM Michael Paquier wrote: > > On Fri, Sep 28, 2018 at 01:53:14PM +0900, Masahiko Sawada wrote: > > I agree. Can we fix this simply by the attached patch? > > Thanks for sending a patch. > > +/* autovacuum cannot be anti-wraparound and not aggressive vacuum */ > +Assert(aggressive); > +msgfmt = _("automatic aggressive vacuum to prevent wraparound of table > \"%s.%s.%s\": index scans: %d\n"); > > While adding this comment in lazy_vacuum_rel() is adapted, I think that > we ought to make the assertion check more aggressive by not having it > depend on if log_min_duration is set or not. So why not moving that to > a place a bit higher, where aggressive gets defined? Since there is no place where checks params->is_wraparound we will have to do something like; if (params->is_wraparound) Assert(aggressive); Or you meant the following? Assert(params->is_wraparound ? aggressive : true); I'm not sure both styles would be appropriate style in the postgres code so I would rather add elog(ERROR) instead. Thought? Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Tue, Oct 02, 2018 at 01:18:01PM +0900, Masahiko Sawada wrote: > I'm not sure both styles would be appropriate style in the postgres > code so I would rather add elog(ERROR) instead. Thought? My brain is rather fried for the rest of the day... But we could just be looking at using USE_ASSERT_CHECKING. Thoughts from other are welcome. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
Michael Paquier writes: > My brain is rather fried for the rest of the day... But we could just > be looking at using USE_ASSERT_CHECKING. Thoughts from other are > welcome. I'd go with folding the condition into a plain Assert. Then it's obvious that no code is added in a non-assert build. I can see that some cases might be so complicated that that isn't nice, but this case doesn't seem to qualify. regards, tom lane
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Tue, Oct 2, 2018 at 11:15 PM Tom Lane wrote: > > Michael Paquier writes: > > My brain is rather fried for the rest of the day... But we could just > > be looking at using USE_ASSERT_CHECKING. Thoughts from other are > > welcome. > > I'd go with folding the condition into a plain Assert. Then it's > obvious that no code is added in a non-assert build. I can see that > some cases might be so complicated that that isn't nice, but this > case doesn't seem to qualify. > Thank you for the comment. Attached the updated patch. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center fix_autovacuum_log_v2.patch Description: Binary data
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Wed, Oct 03, 2018 at 04:37:29PM +0900, Masahiko Sawada wrote: > Thank you for the comment. Attached the updated patch. Thanks for the patch. This looks clean to me at quick glance, not tested though.. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Wed, Oct 03, 2018 at 04:37:29PM +0900, Masahiko Sawada wrote: > Thank you for the comment. Attached the updated patch. So, I have come back to this stuff, and finished with the attached instead, so as the assertion is in a single place. I find that clearer. The comments have also been improved. Thoughts? -- Michael diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c index 8996d366e9..e73b548ac3 100644 --- a/src/backend/commands/vacuumlazy.c +++ b/src/backend/commands/vacuumlazy.c @@ -249,6 +249,10 @@ lazy_vacuum_rel(Relation onerel, int options, VacuumParams *params, if (options & VACOPT_DISABLE_PAGE_SKIPPING) aggressive = true; + /* vacuum preventing wraparound must be aggressive */ + Assert((params->is_wraparound && aggressive) || + !params->is_wraparound); + vacrelstats = (LVRelStats *) palloc0(sizeof(LVRelStats)); vacrelstats->old_rel_pages = onerel->rd_rel->relpages; @@ -376,10 +380,8 @@ lazy_vacuum_rel(Relation onerel, int options, VacuumParams *params, initStringInfo(&buf); if (params->is_wraparound) { -if (aggressive) - msgfmt = _("automatic aggressive vacuum to prevent wraparound of table \"%s.%s.%s\": index scans: %d\n"); -else - msgfmt = _("automatic vacuum to prevent wraparound of table \"%s.%s.%s\": index scans: %d\n"); +/* autovacuum preventing wraparound has to be aggressive */ +msgfmt = _("automatic aggressive vacuum to prevent wraparound of table \"%s.%s.%s\": index scans: %d\n"); } else { signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Fri, Oct 5, 2018 at 12:16 PM Michael Paquier wrote: > > On Wed, Oct 03, 2018 at 04:37:29PM +0900, Masahiko Sawada wrote: > > Thank you for the comment. Attached the updated patch. > > So, I have come back to this stuff, and finished with the attached > instead, so as the assertion is in a single place. I find that > clearer. The comments have also been improved. Thoughts? Thank you! The patch looks good to me. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Fri, Oct 05, 2018 at 12:16:03PM +0900, Michael Paquier wrote: > So, I have come back to this stuff, and finished with the attached > instead, so as the assertion is in a single place. I find that > clearer. The comments have also been improved. Thoughts? And so... I have been looking at committing this thing, and while testing in-depth I have been able to trigger a case where an autovacuum has been able to be not aggressive but anti-wraparound, which is exactly what should not be possible, no? I have simply created an instance with autovacuum_freeze_max_age = 20, then ran pgbench with autovacuum_freeze_table_age=20 set for each table, and also ran installcheck-world in parallel. This has been able to trigger the assertion pretty quickly. Here is the stack trace: #2 0x55e1a12ef87b in ExceptionalCondition (conditionName=0x55e1a14b45c8 "!((params->is_wraparound && aggressive) || !params->is_wraparound)", errorType=0x55e1a14b459d "FailedAssertion", fileName=0x55e1a14b4590 "vacuumlazy.c", lineNumber=254) at assert.c:54 #3 0x55e1a0f6c6ad in lazy_vacuum_rel (onerel=0x7f163e7ea710, options=65, params=0x55e1a2eeba70, bstrategy=0x55e1a2f5c1a0) at vacuumlazy.c:253 #4 0x55e1a0f6c217 in vacuum_rel (relid=1260, relation=0x55e1a2f5d748, options=65, params=0x55e1a2eeba70) at vacuum.c:1714 #5 0x55e1a0f6a3ac in vacuum (options=65, relations=0x55e1a2f2f050, params=0x55e1a2eeba70, bstrategy=0x55e1a2f5c1a0, isTopLevel=true) at vacuum.c:340 #6 0x55e1a10c1ddd in autovacuum_do_vac_analyze (tab=0x55e1a2eeba68, bstrategy=0x55e1a2f5c1a0) at autovacuum.c:3121 #7 0x55e1a10c0e19 in do_autovacuum () at autovacuum.c:2476 $2 = {spcNode = 1664, dbNode = 0, relNode = 1260} (gdb) p onerel->rd_node.relNode $3 = 1260 (gdb) p params->is_wraparound $4 = true (gdb) p aggressive $5 = false I still have the core file, the binaries and the data folder, so it's not a problem to dig into it. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
At Fri, 5 Oct 2018 15:35:04 +0900, Michael Paquier wrote in <20181005063504.gb14...@paquier.xyz> > On Fri, Oct 05, 2018 at 12:16:03PM +0900, Michael Paquier wrote: > > So, I have come back to this stuff, and finished with the attached > > instead, so as the assertion is in a single place. I find that > > clearer. The comments have also been improved. Thoughts? > > And so... I have been looking at committing this thing, and while > testing in-depth I have been able to trigger a case where an autovacuum > has been able to be not aggressive but anti-wraparound, which is exactly > what should not be possible, no? I have simply created an instance with > autovacuum_freeze_max_age = 20, then ran pgbench with > autovacuum_freeze_table_age=20 set for each table, and also ran > installcheck-world in parallel. This has been able to trigger the > assertion pretty quickly. I investigated it and in short, it can happen. It is a kind of race consdition between two autovacuum processes. do_autovacuum() looks into pg_class (using a snapshot) and vacuum_set_xid_limits() looks into relcache. If concurrent vacuum happens and one has finished the relation, another gets relcache invalidation and relfrozenxid is updated. If this happens between do_autovacuum() and vacuum_set_xid_limits(), the latter sees newer relfrozenxid than the former. The problem happens when it moves by more than 5% of autovacuum_freeze_max_age. If lazy_vacuum_rel() sees the situation, the relation is already aggressively vacuumed by a cocurrent worker. We can just ingore the state safely but also we know that the vacuum is useless. 1. Just allow the case there (and add comment). Causes redundant anti-wraparound vacuum. 2. Skip the relation by the condition. I think that we can safely skip the relation in the case. (attached) 3. Ensure that do_autovacuum always sees the same relfrozenxid with vacuum_set_xid_limits(). 4. Prevent concurrent acuuming of the same relation rigorously, somehow. Thoughts? regards. -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c index 8996d366e9..436d573454 100644 --- a/src/backend/commands/vacuumlazy.c +++ b/src/backend/commands/vacuumlazy.c @@ -249,6 +249,21 @@ lazy_vacuum_rel(Relation onerel, int options, VacuumParams *params, if (options & VACOPT_DISABLE_PAGE_SKIPPING) aggressive = true; + /* + * When is_wraparound, relfrozenxid is old enough and aggressive must be + * true here. However, if another concurrent vacuum process has processed + * this relation, relfrozenxid in relcache can be moved forward enough so + * that aggressive is calculated as false here. This relation no longer + * needs to be vacuumed. Bail out. + */ + if (params->is_wraparound && !aggressive) + { + elog(DEBUG1, "relation %d has been vacuumd ocncurrently, skip", + onerel->rd_id); + return; + } + + vacrelstats = (LVRelStats *) palloc0(sizeof(LVRelStats)); vacrelstats->old_rel_pages = onerel->rd_rel->relpages;
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On 2018-Sep-13, Michael Paquier wrote: > Improve autovacuum logging for aggressive and anti-wraparound runs > > A log message was being generated when log_min_duration is reached for > autovacuum on a given relation to indicate if it was an aggressive run, > and missed the point of mentioning if it is doing an anti-wrapround > run. The log message generated is improved so as one, both or no extra > details are added depending on the option set. Hmm, can a for-wraparound vacuum really not be aggressive? I think one of those four cases is really dead code. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Fri, Sep 14, 2018 at 12:35:54PM -0300, Alvaro Herrera wrote: > On 2018-Sep-13, Michael Paquier wrote: >> Improve autovacuum logging for aggressive and anti-wraparound runs >> >> A log message was being generated when log_min_duration is reached for >> autovacuum on a given relation to indicate if it was an aggressive run, >> and missed the point of mentioning if it is doing an anti-wrapround >> run. The log message generated is improved so as one, both or no extra >> details are added depending on the option set. > > Hmm, can a for-wraparound vacuum really not be aggressive? I think one > of those four cases is really dead code. Sure, at the same time it is a no-brainer to keep the code as is, and seeing log messages where (!aggressive && wraparound) would be an indication of surrounding bugs, no? There have been issues in this area in the past. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On 10/9/18 5:15 AM, Kyotaro HORIGUCHI wrote: > At Fri, 5 Oct 2018 15:35:04 +0900, Michael Paquier > wrote in <20181005063504.gb14...@paquier.xyz> >> On Fri, Oct 05, 2018 at 12:16:03PM +0900, Michael Paquier wrote: >>> So, I have come back to this stuff, and finished with the attached >>> instead, so as the assertion is in a single place. I find that >>> clearer. The comments have also been improved. Thoughts? >> And so... I have been looking at committing this thing, and while >> testing in-depth I have been able to trigger a case where an autovacuum >> has been able to be not aggressive but anti-wraparound, which is exactly >> what should not be possible, no? I have simply created an instance with >> autovacuum_freeze_max_age = 20, then ran pgbench with >> autovacuum_freeze_table_age=20 set for each table, and also ran >> installcheck-world in parallel. This has been able to trigger the >> assertion pretty quickly. > I investigated it and in short, it can happen. > > It is a kind of race consdition between two autovacuum > processes. do_autovacuum() looks into pg_class (using a snapshot) > and vacuum_set_xid_limits() looks into relcache. If concurrent > vacuum happens and one has finished the relation, another gets > relcache invalidation and relfrozenxid is updated. If this > happens between do_autovacuum() and vacuum_set_xid_limits(), the > latter sees newer relfrozenxid than the former. The problem > happens when it moves by more than 5% of > autovacuum_freeze_max_age. > > If lazy_vacuum_rel() sees the situation, the relation is already > aggressively vacuumed by a cocurrent worker. We can just ingore > the state safely but also we know that the vacuum is useless. > > 1. Just allow the case there (and add comment). >Causes redundant anti-wraparound vacuum. > > 2. Skip the relation by the condition. > >I think that we can safely skip the relation in the >case. (attached) > > 3. Ensure that do_autovacuum always sees the same relfrozenxid >with vacuum_set_xid_limits(). > > 4. Prevent concurrent acuuming of the same relation rigorously, > somehow. > > Thoughts? > I notice that this seems never to have been acted on. I think we should apply this and remove the (confusing) message setting for the case we'll now be avoiding. If not we should at least comment there that this is a case we only expect to see in pathological cases. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Fri, Mar 08, 2019 at 05:05:53PM -0500, Andrew Dunstan wrote: > I notice that this seems never to have been acted on. I think we should > apply this and remove the (confusing) message setting for the case we'll > now be avoiding. If not we should at least comment there that this is a > case we only expect to see in pathological cases. Sorry for dropping the ball, I would have assumed that Robert would handle it as he is at the origin of the introduction of the aggressive option via fd31cd26. +elog(DEBUG1, "relation %d has been vacuumd ocncurrently, skip", The proposed patch has two typos in two words. I am adding an open item about that. I think I could commit the patch, but I need to study it a bit more first. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Sat, Mar 09, 2019 at 10:15:37AM +0900, Michael Paquier wrote: > I am adding an open item about that. I think I could commit the > patch, but I need to study it a bit more first. So, coming back to this thread, and studying the problem again, it looks that the diagnostic that a non-aggressive, anti-wraparound vacuum could be triggered because the worker sees trouble in the force because of some activity happening in parallel. Hence, if we face this case, it looks right to skip the vacuum for this relation. Attached is an updated patch with a better error message, more comments, and the removal of the anti-wraparound non-aggressive log which was added in 28a8fa9. The error message could be better I guess. Suggestions are welcome. Thoughts? -- Michael diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 5c554f9465..82be8c81f3 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -248,6 +248,25 @@ heap_vacuum_rel(Relation onerel, VacuumParams *params, if (params->options & VACOPT_DISABLE_PAGE_SKIPPING) aggressive = true; + /* + * When running an anti-wraparound vacuum, we expect relfrozenxid to be + * old enough so as aggressive is always set. If this is not the case, + * it could be possible that another concurrent vacuum process has done + * the work for this relation so that relfrozenxid in relcache has + * already been moved forward enough, causing this vacuum run to be + * non-aggressive. If that happens, note that this relation no longer + * needs to be vacuumed, so just skip it. + */ + if (params->is_wraparound && !aggressive) + { + ereport(LOG, +(errmsg_internal("found vacuum to prevent wraparound of table \"%s.%s.%s\" to be not aggressive, so skipping", + get_database_name(MyDatabaseId), + get_namespace_name(RelationGetNamespace(onerel)), + RelationGetRelationName(onerel; + return; + } + vacrelstats = (LVRelStats *) palloc0(sizeof(LVRelStats)); vacrelstats->old_rel_pages = onerel->rd_rel->relpages; @@ -375,10 +394,9 @@ heap_vacuum_rel(Relation onerel, VacuumParams *params, initStringInfo(&buf); if (params->is_wraparound) { -if (aggressive) - msgfmt = _("automatic aggressive vacuum to prevent wraparound of table \"%s.%s.%s\": index scans: %d\n"); -else - msgfmt = _("automatic vacuum to prevent wraparound of table \"%s.%s.%s\": index scans: %d\n"); +/* an anti-wraparound vacuum has to be aggressive */ +Assert(aggressive); +msgfmt = _("automatic aggressive vacuum to prevent wraparound of table \"%s.%s.%s\": index scans: %d\n"); } else { signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On 3/29/19 7:51 AM, Michael Paquier wrote: > On Sat, Mar 09, 2019 at 10:15:37AM +0900, Michael Paquier wrote: >> I am adding an open item about that. I think I could commit the >> patch, but I need to study it a bit more first. > So, coming back to this thread, and studying the problem again, it > looks that the diagnostic that a non-aggressive, anti-wraparound > vacuum could be triggered because the worker sees trouble in the > force because of some activity happening in parallel. Hence, if we > face this case, it looks right to skip the vacuum for this relation. > > Attached is an updated patch with a better error message, more > comments, and the removal of the anti-wraparound non-aggressive log > which was added in 28a8fa9. The error message could be better I > guess. Suggestions are welcome. > > Thoughts? + (errmsg_internal("found vacuum to prevent wraparound of table \"%s.%s.%s\" to be not aggressive, so skipping", This might convey something to hackers, but I doubt it will convey much to regular users. Perhaps something like "skipping redundant anti-wraparound vacuum of table ..." would be better. The comment is also a bit too tentative. Perhaps something like this would do: Normally the relfrozenxid for an anti-wraparound vacuum will be old enough to force an aggressive vacuum. However, a concurrent vacuum might have already done this work that the relfroxzenxid in relcache has been updated. If that happens this vacuum is redundant, so skip it. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Fri, Mar 29, 2019 at 09:11:47AM -0400, Andrew Dunstan wrote: > + (errmsg_internal("found vacuum to prevent wraparound of > table \"%s.%s.%s\" to be not aggressive, so skipping", > > This might convey something to hackers, but I doubt it will convey much > to regular users. Perhaps something like "skipping redundant > anti-wraparound vacuum of table ..." would be better. "skipping redundant" is much better. > The comment is also a bit too tentative. Perhaps something like this > would do: > > Normally the relfrozenxid for an anti-wraparound vacuum will be old > enough to force an aggressive vacuum. However, a concurrent vacuum > might have already done this work that the relfroxzenxid in relcache > has been updated. If that happens this vacuum is redundant, so skip it. That works for me. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On 2019-Mar-29, Michael Paquier wrote: > On Fri, Mar 29, 2019 at 09:11:47AM -0400, Andrew Dunstan wrote: > > + (errmsg_internal("found vacuum to prevent wraparound of > > table \"%s.%s.%s\" to be not aggressive, so skipping", > > > > This might convey something to hackers, but I doubt it will convey much > > to regular users. Perhaps something like "skipping redundant > > anti-wraparound vacuum of table ..." would be better. > > "skipping redundant" is much better. Yeah, that looks good to me too. I wonder if we really need it as LOG though; we don't say anything for actions unless they take more than the min duration, so why say something for a no-op that takes almost no time? Maybe make it DEBUG1. > > The comment is also a bit too tentative. Perhaps something like this > > would do: > > > > Normally the relfrozenxid for an anti-wraparound vacuum will be old > > enough to force an aggressive vacuum. However, a concurrent vacuum > > might have already done this work that the relfroxzenxid in relcache > > has been updated. If that happens this vacuum is redundant, so skip it. > > That works for me. s/relfroxzenxid/relfrozenxid/ -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Fri, Mar 29, 2019 at 11:22:55AM -0300, Alvaro Herrera wrote: > Yeah, that looks good to me too. I wonder if we really need it as LOG > though; we don't say anything for actions unless they take more than the > min duration, so why say something for a no-op that takes almost no time? > Maybe make it DEBUG1. I think that this does not justify a WARNING, as that's harmless for the user even if we use WARNING for other skips (see vacuum_is_relation_owner). However DEBUG1 is also too low in my opinion as this log can be used as an indicator that autovacuum is too much aggressive because there are too many workers for example. I have seen that matter in some CPU-bound environments. I won't fight hard if the consensus is to use DEBUG1 though. So, more opinions? Andrew perhaps? > s/relfroxzenxid/relfrozenxid/ Sure. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
Hi, On 2019-03-29 20:51:38 +0900, Michael Paquier wrote: > So, coming back to this thread, and studying the problem again, it > looks that the diagnostic that a non-aggressive, anti-wraparound > vacuum could be triggered because the worker sees trouble in the > force because of some activity happening in parallel. Hence, if we > face this case, it looks right to skip the vacuum for this relation. > > Attached is an updated patch with a better error message, more > comments, and the removal of the anti-wraparound non-aggressive log > which was added in 28a8fa9. The error message could be better I > guess. Suggestions are welcome. > diff --git a/src/backend/access/heap/vacuumlazy.c > b/src/backend/access/heap/vacuumlazy.c > index 5c554f9465..82be8c81f3 100644 > --- a/src/backend/access/heap/vacuumlazy.c > +++ b/src/backend/access/heap/vacuumlazy.c > @@ -248,6 +248,25 @@ heap_vacuum_rel(Relation onerel, VacuumParams *params, > if (params->options & VACOPT_DISABLE_PAGE_SKIPPING) > aggressive = true; > > + /* > + * When running an anti-wraparound vacuum, we expect relfrozenxid to be > + * old enough so as aggressive is always set. If this is not the case, > + * it could be possible that another concurrent vacuum process has done > + * the work for this relation so that relfrozenxid in relcache has > + * already been moved forward enough, causing this vacuum run to be > + * non-aggressive. If that happens, note that this relation no longer > + * needs to be vacuumed, so just skip it. > + */ > + if (params->is_wraparound && !aggressive) > + { > + ereport(LOG, > + (errmsg_internal("found vacuum to prevent > wraparound of table \"%s.%s.%s\" to be not aggressive, so skipping", > + > get_database_name(MyDatabaseId), > + > get_namespace_name(RelationGetNamespace(onerel)), > + > RelationGetRelationName(onerel; > + return; > + } > + Which valid scenario can lead to this? Neither the comment, nor commit message explain it. Unless you're thinking of scenarios where autovacuum and manual vacuum are mixed, I don't really see valid reasons? Normally autovacuum's locking + the table_recheck_autovac() check should prevent problematic scenarios. I do see a few scenarios that can trigger this - but they all more or less are bugs. It doesn't strike me as a good idea to work around such bugs by silently neutering heap_vacuum_rel(). The likelihood of that temporarily covering up more severe problems seems significant - they're likely to then later bite you with a cluster shutdown. Greetings, Andres Freund
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Mon, Mar 23, 2020 at 06:41:50PM -0700, Andres Freund wrote: > Which valid scenario can lead to this? Neither the comment, nor commit > message explain it. The commit message mentions that concurrent autovacuum jobs can lead to the creation of non-aggressive and anti-wraparound jobs, which have no sense because an aggressive and anti-wraparound job was already done in parallel with a different worker, and that this was possible because of inconsistent relcache lookups across concurrent jobs. This was mentioned upthread. > Unless you're thinking of scenarios where autovacuum > and manual vacuum are mixed, I don't really see valid reasons? Normally > autovacuum's locking + the table_recheck_autovac() check should prevent > problematic scenarios. > > I do see a few scenarios that can trigger this - but they all more or > less are bugs. Hmm. OK. > It doesn't strike me as a good idea to work around such bugs by silently > neutering heap_vacuum_rel(). The likelihood of that temporarily covering > up more severe problems seems significant - they're likely to then later > bite you with a cluster shutdown. Saying that, I have been thinking about this one for a couple of days now and it seems to me that this is a factor contributing to what we are seeing in [1], and I agree that this is just an incorrect approach that makes easier to trigger the real underlying issues, while table_recheck_autovac() ought to be the only code path doing the skip job. Note that I have failed to reproduce the behavior of the other thread though, always finishing with a non-aggressive anti-wraparound skipped because of an aggressive and anti-wraparound job happened just before in parallel, and autovacuum was always able to continue triggering new jobs, keeping the relfrozenxid age at bay. So I would like to first revert that part, to have a cleaner state to work on the underlying issues. A pure revert means also adding back the log message for non-aggressive and anti-wraparound jobs that should never exist, which should be replaced by an assertion once all the holes are fixed. What do you think? [1]: https://www.postgresql.org/message-id/cae39h23rtx1jkyjwc5tccv34hwwraizacuxomdqdpm+zt5-...@mail.gmail.com -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On 3/29/19 9:08 PM, Michael Paquier wrote: > On Fri, Mar 29, 2019 at 11:22:55AM -0300, Alvaro Herrera wrote: >> Yeah, that looks good to me too. I wonder if we really need it as LOG >> though; we don't say anything for actions unless they take more than the >> min duration, so why say something for a no-op that takes almost no time? >> Maybe make it DEBUG1. > I think that this does not justify a WARNING, as that's harmless for > the user even if we use WARNING for other skips (see > vacuum_is_relation_owner). However DEBUG1 is also too low in my > opinion as this log can be used as an indicator that autovacuum is too > much aggressive because there are too many workers for example. I > have seen that matter in some CPU-bound environments. I won't fight > hard if the consensus is to use DEBUG1 though. So, more opinions? > Andrew perhaps? > > It's really just a matter of housekeeping as I see it, so probably DEBUG1 is right. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Sat, Mar 30, 2019 at 08:16:21AM -0400, Andrew Dunstan wrote: > It's really just a matter of housekeeping as I see it, so probably > DEBUG1 is right. Okay, I'll use that then. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Sat, Mar 30, 2019 at 10:12:33PM +0900, Michael Paquier wrote: > Okay, I'll use that then. And finally committed. I have changed the debug1 message so as "to prevent wraparound" is used instead of "anti-wraparound". I have noticed something which was also missing from all the patches proposed on this thread: a reset of the progress state was not done, and we need to call pgstat_progress_end_command() in order to do that. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On 2018-Sep-13, Michael Paquier wrote: > Improve autovacuum logging for aggressive and anti-wraparound runs > > A log message was being generated when log_min_duration is reached for > autovacuum on a given relation to indicate if it was an aggressive run, > and missed the point of mentioning if it is doing an anti-wrapround > run. The log message generated is improved so as one, both or no extra > details are added depending on the option set. Hmm, can a for-wraparound vacuum really not be aggressive? I think one of those four cases is really dead code. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Fri, Sep 14, 2018 at 12:35:54PM -0300, Alvaro Herrera wrote: > On 2018-Sep-13, Michael Paquier wrote: >> Improve autovacuum logging for aggressive and anti-wraparound runs >> >> A log message was being generated when log_min_duration is reached for >> autovacuum on a given relation to indicate if it was an aggressive run, >> and missed the point of mentioning if it is doing an anti-wrapround >> run. The log message generated is improved so as one, both or no extra >> details are added depending on the option set. > > Hmm, can a for-wraparound vacuum really not be aggressive? I think one > of those four cases is really dead code. Sure, at the same time it is a no-brainer to keep the code as is, and seeing log messages where (!aggressive && wraparound) would be an indication of surrounding bugs, no? There have been issues in this area in the past. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Fri, Sep 14, 2018 at 11:35 AM, Alvaro Herrera wrote: > On 2018-Sep-13, Michael Paquier wrote: >> Improve autovacuum logging for aggressive and anti-wraparound runs >> >> A log message was being generated when log_min_duration is reached for >> autovacuum on a given relation to indicate if it was an aggressive run, >> and missed the point of mentioning if it is doing an anti-wrapround >> run. The log message generated is improved so as one, both or no extra >> details are added depending on the option set. > > Hmm, can a for-wraparound vacuum really not be aggressive? I think one > of those four cases is really dead code. My first question was whether TWO of them were dead code ... isn't an aggressive vacuum to prevent wraparound, and a vacuum to prevent wraparound aggressive? I can't figure out what this is giving us that we didn't have before. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
Hello, Robert > My first question was whether TWO of them were dead code ... isn't an > aggressive vacuum to prevent wraparound, and a vacuum to prevent > wraparound aggressive? Maybe i am wrong, aggressive autovacuum was your commit. Message split was in b55509332f50f998b6e8b3830a51c5b9d8f666aa Aggressive autovacuum was in fd31cd265138019d9b5fe53043670898bc9f If aggressive really is wraparound without difference - i think we need refactor this code, it is difficult have two different flags for same purpose. But as far i can see it is possible have aggressive non-wraparound vacuum. One important difference - regular and aggressive regular can be canceled by backend,.wraparound autovacuum can not. (by checking PROC_VACUUM_FOR_WRAPAROUND in src/backend/storage/lmgr/proc.c ) regards, Sergei
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
Hi, On 2018-09-21 20:38:16 +0300, Sergei Kornilov wrote: > > My first question was whether TWO of them were dead code ... isn't an > > aggressive vacuum to prevent wraparound, and a vacuum to prevent > > wraparound aggressive? > Maybe i am wrong, aggressive autovacuum was your commit. > Message split was in b55509332f50f998b6e8b3830a51c5b9d8f666aa > Aggressive autovacuum was in fd31cd265138019d9b5fe53043670898bc9f > > If aggressive really is wraparound without difference - i think we need > refactor this code, it is difficult have two different flags for same purpose. > > But as far i can see it is possible have aggressive non-wraparound vacuum. > One important difference - regular and aggressive regular can be canceled by > backend,.wraparound autovacuum can not. (by checking > PROC_VACUUM_FOR_WRAPAROUND in src/backend/storage/lmgr/proc.c ) Yes, without checking the code, they should be different. Aggressive is controlled by vacuum_freeze_table_age whereas anti-wrap is controlled by autovacuum_freeze_max_age (but also implies aggressive). Greetings, Andres Freund
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On 2018-09-24 18:25:46 +, Nasby, Jim wrote: > > > On Sep 21, 2018, at 12:43 PM, Andres Freund wrote: > > > >> But as far i can see it is possible have aggressive non-wraparound vacuum. > >> One important difference - regular and aggressive regular can be canceled > >> by backend,.wraparound autovacuum can not. (by checking > >> PROC_VACUUM_FOR_WRAPAROUND in src/backend/storage/lmgr/proc.c ) > > > > Yes, without checking the code, they should be different. Aggressive is > > controlled by vacuum_freeze_table_age whereas anti-wrap is controlled by > > autovacuum_freeze_max_age (but also implies aggressive). > > Right, except that by the time you get into the vacuum code itself nothing > should really care about that difference. AFAICT, the only thing > is_wraparound is being used for is to set MyPgXact->vacuumFlags |= > PROC_VACUUM_FOR_WRAPAROUND, which prevents the deadlock detector from killing > an autovac process that’s trying to prevent a wraparound. I think it’d be > clearer to remove is_wraparound and move the check from vacuum_rel() into > lazy_vacuum_rel() (which is where the limits for HeapTupleSatisfiesVacuum get > determined). Something like the attached. I'm very doubtful this is an improvement. Especially with the upcoming pluggable storage work making vacuumlazy.c heap specific, while vacuum.c stays generic. The concept of something like PROC_VACUUM_FOR_WRAPAROUND, should imo not be pushed down that much (even if criteria for it might). Greetings, Andres Freund
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
> On Sep 24, 2018, at 1:29 PM, Andres Freund wrote: > > I'm very doubtful this is an improvement. Especially with the upcoming > pluggable storage work making vacuumlazy.c heap specific, while vacuum.c > stays generic. The concept of something like > PROC_VACUUM_FOR_WRAPAROUND, should imo not be pushed down that much > (even if criteria for it might). That’s already a problem since vacuum logging is spread all over while autovac logging is not. Perhaps there needs to be some sort of vacuum_log() function that immediately provides output for manual vacuums, but aggregates output for autovac. AFAIK that’s the only real reason for autovac logging being a special case today.
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
Hi > An autovacuum can't be just aggressive; it's either anti-wraparound or normal. But autovacuum _can_ be aggressive and not anti-wraparound. I build current master and can see 3 different line types: 2018-09-24 23:47:31.500 MSK 27939 @ from [vxid:4/272032 txid:0] [] LOG: automatic aggressive vacuum of table "postgres.public.foo": index scans: 0 2018-09-24 23:49:27.892 MSK 28333 @ from [vxid:4/284297 txid:0] [] LOG: automatic aggressive vacuum to prevent wraparound of table "postgres.public.foo": index scans: 0 2018-09-24 23:49:29.093 MSK 28337 @ from [vxid:4/284412 txid:0] [] LOG: automatic vacuum of table "postgres.public.foo": index scans: 0 regards, Sergei
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On 2018-Sep-24, Sergei Kornilov wrote: > Hi > > > An autovacuum can't be just aggressive; it's either anti-wraparound or > > normal. > But autovacuum _can_ be aggressive and not anti-wraparound. > I build current master and can see 3 different line types: > 2018-09-24 23:47:31.500 MSK 27939 @ from [vxid:4/272032 txid:0] [] LOG: > automatic aggressive vacuum of table "postgres.public.foo": index scans: 0 > 2018-09-24 23:49:27.892 MSK 28333 @ from [vxid:4/284297 txid:0] [] LOG: > automatic aggressive vacuum to prevent wraparound of table > "postgres.public.foo": index scans: 0 > 2018-09-24 23:49:29.093 MSK 28337 @ from [vxid:4/284412 txid:0] [] LOG: > automatic vacuum of table "postgres.public.foo": index scans: 0 Exactly. It cannot be anti-wraparound and not aggressive, which is the line type not shown. "Aggressive" means it scans all pages; "anti-wraparound" means it does not let itself be cancelled because of another process waiting for a lock on the table. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Tue, Sep 25, 2018 at 6:11 AM Alvaro Herrera wrote: > > On 2018-Sep-24, Sergei Kornilov wrote: > > > Hi > > > > > An autovacuum can't be just aggressive; it's either anti-wraparound or > > > normal. > > But autovacuum _can_ be aggressive and not anti-wraparound. > > I build current master and can see 3 different line types: > > 2018-09-24 23:47:31.500 MSK 27939 @ from [vxid:4/272032 txid:0] [] LOG: > > automatic aggressive vacuum of table "postgres.public.foo": index scans: 0 > > 2018-09-24 23:49:27.892 MSK 28333 @ from [vxid:4/284297 txid:0] [] LOG: > > automatic aggressive vacuum to prevent wraparound of table > > "postgres.public.foo": index scans: 0 > > 2018-09-24 23:49:29.093 MSK 28337 @ from [vxid:4/284412 txid:0] [] LOG: > > automatic vacuum of table "postgres.public.foo": index scans: 0 > > Exactly. > > It cannot be anti-wraparound and not aggressive, which is the line type > not shown. > > "Aggressive" means it scans all pages; "anti-wraparound" means it does > not let itself be cancelled because of another process waiting for a > lock on the table. > I agree. Can we fix this simply by the attached patch? Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center fix_autovacuum_log.patch Description: Binary data
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Fri, Sep 28, 2018 at 01:53:14PM +0900, Masahiko Sawada wrote: > I agree. Can we fix this simply by the attached patch? Thanks for sending a patch. +/* autovacuum cannot be anti-wraparound and not aggressive vacuum */ +Assert(aggressive); +msgfmt = _("automatic aggressive vacuum to prevent wraparound of table \"%s.%s.%s\": index scans: %d\n"); While adding this comment in lazy_vacuum_rel() is adapted, I think that we ought to make the assertion check more aggressive by not having it depend on if log_min_duration is set or not. So why not moving that to a place a bit higher, where aggressive gets defined? -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Tue, Oct 2, 2018 at 9:11 AM Michael Paquier wrote: > > On Fri, Sep 28, 2018 at 01:53:14PM +0900, Masahiko Sawada wrote: > > I agree. Can we fix this simply by the attached patch? > > Thanks for sending a patch. > > +/* autovacuum cannot be anti-wraparound and not aggressive vacuum */ > +Assert(aggressive); > +msgfmt = _("automatic aggressive vacuum to prevent wraparound of table > \"%s.%s.%s\": index scans: %d\n"); > > While adding this comment in lazy_vacuum_rel() is adapted, I think that > we ought to make the assertion check more aggressive by not having it > depend on if log_min_duration is set or not. So why not moving that to > a place a bit higher, where aggressive gets defined? Since there is no place where checks params->is_wraparound we will have to do something like; if (params->is_wraparound) Assert(aggressive); Or you meant the following? Assert(params->is_wraparound ? aggressive : true); I'm not sure both styles would be appropriate style in the postgres code so I would rather add elog(ERROR) instead. Thought? Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Tue, Oct 02, 2018 at 01:18:01PM +0900, Masahiko Sawada wrote: > I'm not sure both styles would be appropriate style in the postgres > code so I would rather add elog(ERROR) instead. Thought? My brain is rather fried for the rest of the day... But we could just be looking at using USE_ASSERT_CHECKING. Thoughts from other are welcome. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
Michael Paquier writes: > My brain is rather fried for the rest of the day... But we could just > be looking at using USE_ASSERT_CHECKING. Thoughts from other are > welcome. I'd go with folding the condition into a plain Assert. Then it's obvious that no code is added in a non-assert build. I can see that some cases might be so complicated that that isn't nice, but this case doesn't seem to qualify. regards, tom lane
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Tue, Oct 2, 2018 at 11:15 PM Tom Lane wrote: > > Michael Paquier writes: > > My brain is rather fried for the rest of the day... But we could just > > be looking at using USE_ASSERT_CHECKING. Thoughts from other are > > welcome. > > I'd go with folding the condition into a plain Assert. Then it's > obvious that no code is added in a non-assert build. I can see that > some cases might be so complicated that that isn't nice, but this > case doesn't seem to qualify. > Thank you for the comment. Attached the updated patch. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center fix_autovacuum_log_v2.patch Description: Binary data
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Wed, Oct 03, 2018 at 04:37:29PM +0900, Masahiko Sawada wrote: > Thank you for the comment. Attached the updated patch. Thanks for the patch. This looks clean to me at quick glance, not tested though.. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Wed, Oct 03, 2018 at 04:37:29PM +0900, Masahiko Sawada wrote: > Thank you for the comment. Attached the updated patch. So, I have come back to this stuff, and finished with the attached instead, so as the assertion is in a single place. I find that clearer. The comments have also been improved. Thoughts? -- Michael diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c index 8996d366e9..e73b548ac3 100644 --- a/src/backend/commands/vacuumlazy.c +++ b/src/backend/commands/vacuumlazy.c @@ -249,6 +249,10 @@ lazy_vacuum_rel(Relation onerel, int options, VacuumParams *params, if (options & VACOPT_DISABLE_PAGE_SKIPPING) aggressive = true; + /* vacuum preventing wraparound must be aggressive */ + Assert((params->is_wraparound && aggressive) || + !params->is_wraparound); + vacrelstats = (LVRelStats *) palloc0(sizeof(LVRelStats)); vacrelstats->old_rel_pages = onerel->rd_rel->relpages; @@ -376,10 +380,8 @@ lazy_vacuum_rel(Relation onerel, int options, VacuumParams *params, initStringInfo(&buf); if (params->is_wraparound) { -if (aggressive) - msgfmt = _("automatic aggressive vacuum to prevent wraparound of table \"%s.%s.%s\": index scans: %d\n"); -else - msgfmt = _("automatic vacuum to prevent wraparound of table \"%s.%s.%s\": index scans: %d\n"); +/* autovacuum preventing wraparound has to be aggressive */ +msgfmt = _("automatic aggressive vacuum to prevent wraparound of table \"%s.%s.%s\": index scans: %d\n"); } else { signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Fri, Oct 5, 2018 at 12:16 PM Michael Paquier wrote: > > On Wed, Oct 03, 2018 at 04:37:29PM +0900, Masahiko Sawada wrote: > > Thank you for the comment. Attached the updated patch. > > So, I have come back to this stuff, and finished with the attached > instead, so as the assertion is in a single place. I find that > clearer. The comments have also been improved. Thoughts? Thank you! The patch looks good to me. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
On Fri, Oct 05, 2018 at 12:16:03PM +0900, Michael Paquier wrote: > So, I have come back to this stuff, and finished with the attached > instead, so as the assertion is in a single place. I find that > clearer. The comments have also been improved. Thoughts? And so... I have been looking at committing this thing, and while testing in-depth I have been able to trigger a case where an autovacuum has been able to be not aggressive but anti-wraparound, which is exactly what should not be possible, no? I have simply created an instance with autovacuum_freeze_max_age = 20, then ran pgbench with autovacuum_freeze_table_age=20 set for each table, and also ran installcheck-world in parallel. This has been able to trigger the assertion pretty quickly. Here is the stack trace: #2 0x55e1a12ef87b in ExceptionalCondition (conditionName=0x55e1a14b45c8 "!((params->is_wraparound && aggressive) || !params->is_wraparound)", errorType=0x55e1a14b459d "FailedAssertion", fileName=0x55e1a14b4590 "vacuumlazy.c", lineNumber=254) at assert.c:54 #3 0x55e1a0f6c6ad in lazy_vacuum_rel (onerel=0x7f163e7ea710, options=65, params=0x55e1a2eeba70, bstrategy=0x55e1a2f5c1a0) at vacuumlazy.c:253 #4 0x55e1a0f6c217 in vacuum_rel (relid=1260, relation=0x55e1a2f5d748, options=65, params=0x55e1a2eeba70) at vacuum.c:1714 #5 0x55e1a0f6a3ac in vacuum (options=65, relations=0x55e1a2f2f050, params=0x55e1a2eeba70, bstrategy=0x55e1a2f5c1a0, isTopLevel=true) at vacuum.c:340 #6 0x55e1a10c1ddd in autovacuum_do_vac_analyze (tab=0x55e1a2eeba68, bstrategy=0x55e1a2f5c1a0) at autovacuum.c:3121 #7 0x55e1a10c0e19 in do_autovacuum () at autovacuum.c:2476 $2 = {spcNode = 1664, dbNode = 0, relNode = 1260} (gdb) p onerel->rd_node.relNode $3 = 1260 (gdb) p params->is_wraparound $4 = true (gdb) p aggressive $5 = false I still have the core file, the binaries and the data folder, so it's not a problem to dig into it. -- Michael signature.asc Description: PGP signature
Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru
At Fri, 5 Oct 2018 15:35:04 +0900, Michael Paquier wrote in <20181005063504.gb14...@paquier.xyz> > On Fri, Oct 05, 2018 at 12:16:03PM +0900, Michael Paquier wrote: > > So, I have come back to this stuff, and finished with the attached > > instead, so as the assertion is in a single place. I find that > > clearer. The comments have also been improved. Thoughts? > > And so... I have been looking at committing this thing, and while > testing in-depth I have been able to trigger a case where an autovacuum > has been able to be not aggressive but anti-wraparound, which is exactly > what should not be possible, no? I have simply created an instance with > autovacuum_freeze_max_age = 20, then ran pgbench with > autovacuum_freeze_table_age=20 set for each table, and also ran > installcheck-world in parallel. This has been able to trigger the > assertion pretty quickly. I investigated it and in short, it can happen. It is a kind of race consdition between two autovacuum processes. do_autovacuum() looks into pg_class (using a snapshot) and vacuum_set_xid_limits() looks into relcache. If concurrent vacuum happens and one has finished the relation, another gets relcache invalidation and relfrozenxid is updated. If this happens between do_autovacuum() and vacuum_set_xid_limits(), the latter sees newer relfrozenxid than the former. The problem happens when it moves by more than 5% of autovacuum_freeze_max_age. If lazy_vacuum_rel() sees the situation, the relation is already aggressively vacuumed by a cocurrent worker. We can just ingore the state safely but also we know that the vacuum is useless. 1. Just allow the case there (and add comment). Causes redundant anti-wraparound vacuum. 2. Skip the relation by the condition. I think that we can safely skip the relation in the case. (attached) 3. Ensure that do_autovacuum always sees the same relfrozenxid with vacuum_set_xid_limits(). 4. Prevent concurrent acuuming of the same relation rigorously, somehow. Thoughts? regards. -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c index 8996d366e9..436d573454 100644 --- a/src/backend/commands/vacuumlazy.c +++ b/src/backend/commands/vacuumlazy.c @@ -249,6 +249,21 @@ lazy_vacuum_rel(Relation onerel, int options, VacuumParams *params, if (options & VACOPT_DISABLE_PAGE_SKIPPING) aggressive = true; + /* + * When is_wraparound, relfrozenxid is old enough and aggressive must be + * true here. However, if another concurrent vacuum process has processed + * this relation, relfrozenxid in relcache can be moved forward enough so + * that aggressive is calculated as false here. This relation no longer + * needs to be vacuumed. Bail out. + */ + if (params->is_wraparound && !aggressive) + { + elog(DEBUG1, "relation %d has been vacuumd ocncurrently, skip", + onerel->rd_id); + return; + } + + vacrelstats = (LVRelStats *) palloc0(sizeof(LVRelStats)); vacrelstats->old_rel_pages = onerel->rd_rel->relpages;