Thread: PROC_VACUUM_FOR_WRAPAROUND doesn't work expectedly

PROC_VACUUM_FOR_WRAPAROUND doesn't work expectedly

From
ITAGAKI Takahiro
Date:
I found autovacuum can be canceled by blocked backends even if the vacuum
is for preventing XID wraparound in 8.3.0 and HEAD. Autovacuum sets
PROC_VACUUM_FOR_WRAPAROUND flag just before vacuum, but the flag will be
cleared at the beginning of vacuum; PROC_VACUUM_FOR_WRAPAROUND is not set
during the vacuum.

The sequence is below:
vacuum()-> CommitTransactionCommand()    -> ProcArrayEndTransaction()        -> proc->vacuumFlags &=
~PROC_VACUUM_STATE_MASK;->vacuum_rel()
 

PROC_VACUUM_STATE_MASK is defined as (0x0E), that is including
PROC_VACUUM_FOR_WRAPAROUND (0x08). The wraparound flag is cleared
before vacuum tasks.

I tried to make a patch to exclude PROC_VACUUM_FOR_WRAPAROUND
from PROC_VACUUM_STATE_MASK and make autovacuum workers to clear
PROC_VACUUM_FOR_WRAPAROUND by themselves. Is it a reasonable solution?


Index: src/backend/postmaster/autovacuum.c
===================================================================
--- src/backend/postmaster/autovacuum.c    (HEAD)
+++ src/backend/postmaster/autovacuum.c    (working copy)
@@ -2163,6 +2163,12 @@        PG_END_TRY();        /* the PGPROC flags are reset at the next end of transaction */
+        if (tab->at_wraparound)
+        {
+            LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+            MyProc->vacuumFlags &= ~PROC_VACUUM_FOR_WRAPAROUND;
+            LWLockRelease(ProcArrayLock);
+        }        /* be tidy */        pfree(tab);
Index: src/include/storage/proc.h
===================================================================
--- src/include/storage/proc.h    (HEAD)
+++ src/include/storage/proc.h    (working copy)
@@ -45,7 +45,7 @@#define        PROC_VACUUM_FOR_WRAPAROUND 0x08        /* set by autovac only *//* flags reset at
EOXact*/
 
-#define        PROC_VACUUM_STATE_MASK (0x0E)
+#define        PROC_VACUUM_STATE_MASK (PROC_IN_VACUUM | PROC_IN_ANALYZE)/* * Each backend has a PGPROC struct in
sharedmemory.  There is also a list of
 

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center



Re: PROC_VACUUM_FOR_WRAPAROUND doesn't work expectedly

From
"Pavan Deolasee"
Date:
On Fri, Mar 14, 2008 at 7:36 AM, ITAGAKI Takahiro
<itagaki.takahiro@oss.ntt.co.jp> wrote:
>
>  I tried to make a patch to exclude PROC_VACUUM_FOR_WRAPAROUND
>  from PROC_VACUUM_STATE_MASK and make autovacuum workers to clear
>  PROC_VACUUM_FOR_WRAPAROUND by themselves. Is it a reasonable solution?
>
>

Looks good to me. Otherwise we can pass additional parameter to
autovacuum_do_vac_analyze() and then use vacstmt to pass the information
to vacuum(). Not sure which is a cleaner way though.

I also noticed that inside autovacuum_do_vac_analyze(), we save the old
context (which is TopTransactionContext) and restore it back after vacuum()
returns. But vacuum() might have started a new transaction invalidating the
saved context. Do we see any problem here ?

Thanks,
Pavan


-- 
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com


Re: PROC_VACUUM_FOR_WRAPAROUND doesn't work expectedly

From
Alvaro Herrera
Date:
ITAGAKI Takahiro wrote:
> I found autovacuum can be canceled by blocked backends even if the vacuum
> is for preventing XID wraparound in 8.3.0 and HEAD. Autovacuum sets
> PROC_VACUUM_FOR_WRAPAROUND flag just before vacuum, but the flag will be
> cleared at the beginning of vacuum; PROC_VACUUM_FOR_WRAPAROUND is not set
> during the vacuum.

Rats.

How about this other patch?

--
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Attachment

Re: PROC_VACUUM_FOR_WRAPAROUND doesn't work expectedly

From
Tom Lane
Date:
Alvaro Herrera <alvherre@commandprompt.com> writes:
> How about this other patch?

That's really, really ugly.  I'd rather see the flag passed down to
vacuum_rel as a regular function argument.  I realize you'll need
to touch the signatures of a couple of levels of functions to do that,
but a global variable for this seems just dangerous.
        regards, tom lane


Re: PROC_VACUUM_FOR_WRAPAROUND doesn't work expectedly

From
Alvaro Herrera
Date:
Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > How about this other patch?
> 
> That's really, really ugly.  I'd rather see the flag passed down to
> vacuum_rel as a regular function argument.  I realize you'll need
> to touch the signatures of a couple of levels of functions to do that,
> but a global variable for this seems just dangerous.

Okay, I'll do that instead.  If I do it quickly, will it be present in
8.3.1?  I think it was already tagged so my guess is it won't.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support


Re: PROC_VACUUM_FOR_WRAPAROUND doesn't work expectedly

From
Tom Lane
Date:
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Okay, I'll do that instead.  If I do it quickly, will it be present in
> 8.3.1?  I think it was already tagged so my guess is it won't.

I think Marc is planning to rebundle this evening, so if you can get
it done in the next few hours...
        regards, tom lane


Re: PROC_VACUUM_FOR_WRAPAROUND doesn't work expectedly

From
Alvaro Herrera
Date:
Alvaro Herrera wrote:
> Tom Lane wrote:

> > That's really, really ugly.  I'd rather see the flag passed down to
> > vacuum_rel as a regular function argument.  I realize you'll need
> > to touch the signatures of a couple of levels of functions to do that,
> > but a global variable for this seems just dangerous.
>
> Okay, I'll do that instead.

Does this look better?

--
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Attachment

Re: PROC_VACUUM_FOR_WRAPAROUND doesn't work expectedly

From
Tom Lane
Date:
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Does this look better?

Yeah, works for me.  Please apply.
        regards, tom lane


Re: PROC_VACUUM_FOR_WRAPAROUND doesn't work expectedly

From
Alvaro Herrera
Date:
Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > Does this look better?
> 
> Yeah, works for me.  Please apply.

Applied to HEAD and 8.3.

Thanks, Takahiro-san, for the report!

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.


Re: PROC_VACUUM_FOR_WRAPAROUND doesn't work expectedly

From
Tom Lane
Date:
"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:
> I also noticed that inside autovacuum_do_vac_analyze(), we save the old
> context (which is TopTransactionContext) and restore it back after vacuum()
> returns. But vacuum() might have started a new transaction invalidating the
> saved context. Do we see any problem here ?

I agree, that looks pretty darn bogus.  The other problem with it is
that it's running vacuum() in an indefinite-lifespan context.  Perhaps
that has something to do with the report we saw awhile back of autovac
leaking memory ...
        regards, tom lane


Re: PROC_VACUUM_FOR_WRAPAROUND doesn't work expectedly

From
Alvaro Herrera
Date:
Tom Lane escribió:
> "Pavan Deolasee" <pavan.deolasee@gmail.com> writes:
> > I also noticed that inside autovacuum_do_vac_analyze(), we save the old
> > context (which is TopTransactionContext) and restore it back after vacuum()
> > returns. But vacuum() might have started a new transaction invalidating the
> > saved context. Do we see any problem here ?
> 
> I agree, that looks pretty darn bogus.

Sorry, I failed to notice this part of Pavan's reply.  Thanks for
patching it.

> The other problem with it is that it's running vacuum() in an
> indefinite-lifespan context.  Perhaps that has something to do with
> the report we saw awhile back of autovac leaking memory ...

Hmm, I'm not sure which memory leak are you referring to, but if it's
the same I'm thinking of, then it cannot be the same because this one
occurs on the worker and the other was on the launcher; also, I patched
that one:

----------------------------
revision 1.58
date: 2007-09-12 18:14:59 -0400;  author: alvherre;  state: Exp;  lines: +20 -3;Fix a memory leak in the autovacuum
launchercode.  Noted by Darcy Buskermolen,
 
who reported it privately to me.
----------------------------

Hmm, well, if he reported it privately to me then you cannot have heard
of it, right?  So perhaps it is indeed a different one ...

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support


Re: PROC_VACUUM_FOR_WRAPAROUND doesn't work expectedly

From
Tom Lane
Date:
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane escribi�:
>> The other problem with it is that it's running vacuum() in an
>> indefinite-lifespan context.  Perhaps that has something to do with
>> the report we saw awhile back of autovac leaking memory ...

> Hmm, I'm not sure which memory leak are you referring to, but if it's
> the same I'm thinking of, then it cannot be the same because this one
> occurs on the worker and the other was on the launcher; also, I patched
> that one:

I was thinking of Erik Jones' report of TopMemoryContext bloat in a
database with 200000 tables (in pre-8.3 code).  But I guess this still
doesn't fit, because any leakage induced by vacuum() would have been in
AutovacMemCxt, and that wasn't what he saw.  So I still don't know what
was happening with Erik's issue.
        regards, tom lane