Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Alvaro Herrera-9
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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Michael Paquier-2
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 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Robert Haas
In reply to this post by Alvaro Herrera-9
On Fri, Sep 14, 2018 at 11:35 AM, Alvaro Herrera
<[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Sergei Kornilov
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 fd31cd265138019dcccc9b5fe53043670898bc9f

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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Andres Freund
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 fd31cd265138019dcccc9b5fe53043670898bc9f
>
> 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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Nasby, Jim-2

> On Sep 21, 2018, at 12:43 PM, Andres Freund <[hidden email]> 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.

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

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Andres Freund
On 2018-09-24 18:25:46 +0000, Nasby, Jim wrote:

>
> > On Sep 21, 2018, at 12:43 PM, Andres Freund <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Nasby, Jim-2

> On Sep 24, 2018, at 1:29 PM, Andres Freund <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Sergei Kornilov
In reply to this post by Nasby, Jim-2
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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Alvaro Herrera-9
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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Masahiko Sawada
On Tue, Sep 25, 2018 at 6:11 AM Alvaro Herrera <[hidden email]> 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 (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Michael Paquier-2
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 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Masahiko Sawada
On Tue, Oct 2, 2018 at 9:11 AM Michael Paquier <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Michael Paquier-2
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 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Tom Lane-2
Michael Paquier <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Masahiko Sawada
On Tue, Oct 2, 2018 at 11:15 PM Tom Lane <[hidden email]> wrote:

>
> Michael Paquier <[hidden email]> 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 (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Michael Paquier-2
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 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Michael Paquier-2
In reply to this post by Masahiko Sawada
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

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

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Masahiko Sawada
On Fri, Oct 5, 2018 at 12:16 PM Michael Paquier <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

Michael Paquier-2
In reply to this post by Michael Paquier-2
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 = 200000, then ran pgbench with
autovacuum_freeze_table_age=200000 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  0x000055e1a12ef87b 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  0x000055e1a0f6c6ad in lazy_vacuum_rel (onerel=0x7f163e7ea710,
options=65, params=0x55e1a2eeba70, bstrategy=0x55e1a2f5c1a0) at
vacuumlazy.c:253
#4  0x000055e1a0f6c217 in vacuum_rel (relid=1260,
relation=0x55e1a2f5d748, options=65, params=0x55e1a2eeba70) at
vacuum.c:1714
#5  0x000055e1a0f6a3ac in vacuum (options=65, relations=0x55e1a2f2f050,
params=0x55e1a2eeba70, bstrategy=0x55e1a2f5c1a0, isTopLevel=true) at
vacuum.c:340
#6  0x000055e1a10c1ddd in autovacuum_do_vac_analyze
(tab=0x55e1a2eeba68, bstrategy=0x55e1a2f5c1a0) at autovacuum.c:3121
#7  0x000055e1a10c0e19 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 (849 bytes) Download Attachment
12