Re: 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 shared memory. 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(dot)takahiro(at)oss(dot)ntt(dot)co(dot)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

Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Does this look better?

Yeah, works for me. Please apply.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)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(dot)deolasee(at)gmail(dot)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(dot)deolasee(at)gmail(dot)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 launcher code. 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(at)commandprompt(dot)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