Thread: Re: [HACKERS] Autovacuum improvements

Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > Note that currently there's no way for a backend to know whether another
> > backend is autovacuum or not.  I thought about adding a flag to PGPROC,
> > but eventually considered it ugly,
>
> No, that was exactly the way I thought we'd do it.  One thing to note is
> that to avoid race conditions, the PGPROC entry has to be marked as
> autovac from the instant it's inserted into the array --- with a
> separate area I think you'd have difficulty avoiding the race condition.

Here it is.

I have run the regression tests many times and they pass.  I added some
debug printouts (not in the patch) to make sure the kill code path was
being invoked, and while it seldom shows, it certainly does.

Note that I used the same DatabaseHasActiveBackends() function to do the
kill.  I had first added a different one to kill autovacuum, but then
noticed that this one has no callers that don't want the side effect, so
I merged them.  It seems a bit ugly to me to have a function named like
this and still have the side effect, but on the other hand it's quite
useless to have a version without the side effect that will never get
called.

Another point to make is that it only kills autovacuum, and only if no
other process is found.  So if there are two processes and autovacuum is
one of them, it will be allowed to continue.

I feel that changing the DROP DATABASE behavior with respect to killing
other backends is beyond the scope of this patch.  It seems easy enough
to do if somebody feels so inclined.

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

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Here it is.

I'd drop the InitProcess API change, which touches many more places than
you really need, and just have InitProcess check IsAutoVacuumProcess(),
which should be valid by the time control gets to it.  This is more like
the way that InitPostgres handles it, anyway.

> Note that I used the same DatabaseHasActiveBackends() function to do the
> kill.  I had first added a different one to kill autovacuum, but then
> noticed that this one has no callers that don't want the side effect, so
> I merged them.  It seems a bit ugly to me to have a function named like
> this and still have the side effect, but on the other hand it's quite
> useless to have a version without the side effect that will never get
> called.

Agreed; maybe change the name to something that sounds less like a
side-effect-free function?

> Another point to make is that it only kills autovacuum, and only if no
> other process is found.  So if there are two processes and autovacuum is
> one of them, it will be allowed to continue.

What if there are two autovac processes, which seems likely to be
possible soon enough?

> I feel that changing the DROP DATABASE behavior with respect to killing
> other backends is beyond the scope of this patch.  It seems easy enough
> to do if somebody feels so inclined.

I don't think the idea of killing non-autovac processes will fly.
Waiting for them, on the other hand, might.

> +         /* good, there's only an autovacuum -- kill it */
> +         kill(autovacPid, SIGINT);
> +         LWLockRelease(ProcArrayLock);

Please release the LWLock *before* executing a kernel call ...

            regards, tom lane

I wrote:
> Please release the LWLock *before* executing a kernel call ...

Oh, and there had definitely better be a CHECK_FOR_INTERRUPTS in
this loop ...

            regards, tom lane

Alvaro Herrera wrote:
> Tom Lane wrote:
> > Alvaro Herrera <alvherre@commandprompt.com> writes:
> > > Here it is.
> >
> > I'd drop the InitProcess API change, which touches many more places than
> > you really need, and just have InitProcess check IsAutoVacuumProcess(),
> > which should be valid by the time control gets to it.  This is more like
> > the way that InitPostgres handles it, anyway.
>
> Hmm, the problem is SubPostmasterMain, which is in the EXEC_BACKEND
> path.  It hasn't reached the autovacuum.c code yet, so it hasn't had the
> chance to set the am_autovacuum static variable (in autovacuum.c).  I
> guess the answer here is to allow that variable to be set from the
> outside.

New version of the patch attached.

I'll probably apply this tomorrow morning unless there are objections.

An important difference from the previous patch is that
DatabaseHasActiveBackends (now renamed to
DatabaseCancelAutovacuumActivity) cycles through the whole ProcArray
instead of stopping at the first occurence of a backend in that
database.  This is to be able to fulfill its mission of cancelling *any*
autovacuum activity that may be taking place on the database (not just
the one that happens to be below the first process in the ProcArray).


I also tried the EXEC_BACKEND case (albeit less extensively) and it
seems to work -- it cancels running autovacuums at least.

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

Alvaro Herrera wrote:
> Alvaro Herrera wrote:
> New version of the patch attached.
>
> I'll probably apply this tomorrow morning unless there are objections.
>
> An important difference from the previous patch is that
> DatabaseHasActiveBackends (now renamed to
> DatabaseCancelAutovacuumActivity) cycles through the whole ProcArray
> instead of stopping at the first occurence of a backend in that
> database.  This is to be able to fulfill its mission of cancelling *any*
> autovacuum activity that may be taking place on the database (not just
> the one that happens to be below the first process in the ProcArray).

Is there any chance of a race condition here?  That is, can the launcher
process start a new autovacuum process against that database that your
code will miss since it was started after you began your search?

"Matthew T. O'Connor" <matthew@zeut.net> writes:
> Is there any chance of a race condition here?  That is, can the launcher
> process start a new autovacuum process against that database that your
> code will miss since it was started after you began your search?

No; we're holding a lock against incoming processes in that database.

            regards, tom lane