Thread: PROC_VACUUM_FOR_WRAPAROUND doesn't work expectedly
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
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
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
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
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
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
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
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Does this look better?
Yeah, works for me. Please apply.
regards, tom lane
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.
"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
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
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