Re: [pgsql-patches] [HACKERS] Autovacuum improvements
"Matthew T. O'Connor" 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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [pgsql-patches] [HACKERS] Autovacuum improvements
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? ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [pgsql-patches] [HACKERS] Autovacuum improvements
Alvaro Herrera wrote:
> Tom Lane wrote:
> > Alvaro Herrera <[EMAIL PROTECTED]> 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 Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Index: src/backend/access/transam/twophase.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/transam/twophase.c,v
retrieving revision 1.26
diff -c -p -r1.26 twophase.c
*** src/backend/access/transam/twophase.c 5 Jan 2007 22:19:23 - 1.26
--- src/backend/access/transam/twophase.c 15 Jan 2007 19:11:15 -
*** MarkAsPreparing(TransactionId xid, const
*** 280,285
--- 280,286
gxact->proc.databaseId = databaseid;
gxact->proc.roleId = owner;
gxact->proc.inVacuum = false;
+ gxact->proc.isAutovacuum = false;
gxact->proc.lwWaiting = false;
gxact->proc.lwExclusive = false;
gxact->proc.lwWaitLink = NULL;
Index: src/backend/commands/dbcommands.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/commands/dbcommands.c,v
retrieving revision 1.188
diff -c -p -r1.188 dbcommands.c
*** src/backend/commands/dbcommands.c 5 Jan 2007 22:19:25 - 1.188
--- src/backend/commands/dbcommands.c 15 Jan 2007 22:00:56 -
*** createdb(const CreatedbStmt *stmt)
*** 250,260
* (exception is to allow CREATE DB while connected to template1).
* Otherwise we might copy inconsistent data.
*/
! if (DatabaseHasActiveBackends(src_dboid, true))
ereport(ERROR,
(errcode(ERRCODE_OBJECT_IN_USE),
! errmsg("source database \"%s\" is being accessed by other users",
! dbtemplate)));
/* If encoding is defaulted, use source's encoding */
if (encoding < 0)
--- 250,260
* (exception is to allow CREATE DB while connected to template1).
* Otherwise we might copy inconsistent data.
*/
! if (DatabaseCancelAutovacuumActivity(src_dboid, true))
ereport(ERROR,
(errcode(ERRCODE_OBJECT_IN_USE),
! errmsg("source database \"%s\" is being accessed by other users",
! dbtemplate)));
/* If encoding is defaulted, use source's encoding */
if (encoding < 0)
*** dropdb(const char *dbname, bool missing_
*** 602,608
* Check for active backends in the target database. (Because we hold the
* database lock, no new ones can start after this.)
*/
! if (DatabaseHasActiveBackends(db_id, false))
ereport(ERROR,
(errcode(ERRCODE_OBJECT_IN_USE),
errmsg("database \"%s\" is being accessed by other users",
--- 602,608
* Check for active backends in the target database. (Because we hold the
* database lock, no new ones can start after this.)
*/
! if (DatabaseCancelAutovacuumActivity(db_id, false))
ereport(ERROR,
(errcode(ERRCODE_OBJECT_IN_USE),
errmsg("database \"%s\" is being accessed by other users",
*** RenameDatabase(const char *oldname, cons
*** 706,712
* Make sure the database does not have active sessions. This is the same
* concern as above, but applied to other sessions.
*/
! if (DatabaseHasActiveBackends(db_id, false))
ereport(ERROR,
(errcode(ERRCODE_OBJECT_IN_USE),
errmsg("database \"%s\" is being accessed by other users",
--- 706,712
* Make sure the database does not have active sessions. This is the same
* concern as above, but applied to other sessions.
*/
! if (DatabaseCancelAutovacuumActivity(db_id, false))
ereport(ERROR,
(errcode(ERRCODE_OBJECT_IN_USE),
errmsg("database \"%s\" is being accessed by other users",
Index: src/backend/postmaster/autova
Re: [pgsql-patches] [HACKERS] Autovacuum improvements
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 ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [pgsql-patches] [HACKERS] Autovacuum improvements
Alvaro Herrera <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [pgsql-patches] [HACKERS] Autovacuum improvements
Tom Lane wrote: > Alvaro Herrera <[EMAIL PROTECTED]> 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 Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. Index: src/backend/access/transam/twophase.c === RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/transam/twophase.c,v retrieving revision 1.26 diff -c -p -r1.26 twophase.c *** src/backend/access/transam/twophase.c 5 Jan 2007 22:19:23 - 1.26 --- src/backend/access/transam/twophase.c 15 Jan 2007 19:11:15 - *** MarkAsPreparing(TransactionId xid, const *** 280,285 --- 280,286 gxact->proc.databaseId = databaseid; gxact->proc.roleId = owner; gxact->proc.inVacuum = false; + gxact->proc.isAutovacuum = false; gxact->proc.lwWaiting = false; gxact->proc.lwExclusive = false; gxact->proc.lwWaitLink = NULL; Index: src/backend/bootstrap/bootstrap.c === RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/bootstrap/bootstrap.c,v retrieving revision 1.228 diff -c -p -r1.228 bootstrap.c *** src/backend/bootstrap/bootstrap.c 5 Jan 2007 22:19:24 - 1.228 --- src/backend/bootstrap/bootstrap.c 15 Jan 2007 19:08:47 - *** BootstrapMain(int argc, char *argv[]) *** 446,452 /* * Do backend-like initialization for bootstrap mode */ ! InitProcess(); (void) InitPostgres(dbname, NULL); /* --- 446,452 /* * Do backend-like initialization for bootstrap mode */ ! InitProcess(false); (void) InitPostgres(dbname, NULL); /* Index: src/backend/postmaster/autovacuum.c === RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/postmaster/autovacuum.c,v retrieving revision 1.30 diff -c -p -r1.30 autovacuum.c *** src/backend/postmaster/autovacuum.c 5 Jan 2007 22:19:36 - 1.30 --- src/backend/postmaster/autovacuum.c 15 Jan 2007 20:20:28 - *** AutoVacMain(int argc, char *argv[]) *** 290,296 * had to do some stuff with LWLocks). */ #ifndef EXEC_BACKEND ! InitProcess(); #endif /* --- 290,296 * had to do some stuff with LWLocks). */ #ifndef EXEC_BACKEND ! InitProcess(true); #endif /* *** AutoVacMain(int argc, char *argv[]) *** 307,313 EmitErrorReport(); /* ! * We can now go away. Note that because we'll call InitProcess, a * callback will be registered to do ProcKill, which will clean up * necessary state. */ --- 307,313 EmitErrorReport(); /* ! * We can now go away. Note that because we called InitProcess, a * callback will be registered to do ProcKill, which will clean up * necessary state. */ Index: src/backend/postmaster/postmaster.c === RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/postmaster/postmaster.c,v retrieving revision 1.507 diff -c -p -r1.507 postmaster.c *** src/backend/postmaster/postmaster.c 5 Jan 2007 22:19:36 - 1.507 --- src/backend/postmaster/postmaster.c 15 Jan 2007 19:09:29 - *** SubPostmasterMain(int argc, char *argv[] *** 3347,3353 InitShmemAccess(UsedShmemSegAddr); /* Need a PGPROC to run CreateSharedMemoryAndSemaphores */ ! InitProcess(); /* * Attach process to shared data structures. If te
