autovacuum launcher using InitPostgres
Hi,
This seems to be the last holdup for flatfiles.c. This patch removes
usage of that code in autovacuum launcher, instead making it go through
the most basic relcache initialization so that it is able to read
pg_database.
To this end, InitPostgres has been split in two. The launcher only
calls the first half, the rest of the callers have been patched to
invoke the second half.
The only thing I'm aware is missing from this patch is fixing up
avlauncher's signal handling, and adding a bit more commentary; also I
haven't tested it under EXEC_BACKEND yet.
Comments?
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachments:
avlauncher-proc.patchtext/x-diff; charset=us-asciiDownload
Index: src/backend/bootstrap/bootstrap.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/bootstrap/bootstrap.c,v
retrieving revision 1.252
diff -c -p -r1.252 bootstrap.c
*** src/backend/bootstrap/bootstrap.c 2 Aug 2009 22:14:51 -0000 1.252
--- src/backend/bootstrap/bootstrap.c 31 Aug 2009 13:57:37 -0000
*************** CheckerModeMain(void)
*** 469,475 ****
* Do backend-like initialization for bootstrap mode
*/
InitProcess();
! InitPostgres(NULL, InvalidOid, NULL, NULL);
proc_exit(0);
}
--- 469,476 ----
* Do backend-like initialization for bootstrap mode
*/
InitProcess();
! InitPostgres();
! InitPostgresPhase2(NULL, InvalidOid, NULL, NULL);
proc_exit(0);
}
*************** BootstrapModeMain(void)
*** 493,499 ****
* Do backend-like initialization for bootstrap mode
*/
InitProcess();
! InitPostgres(NULL, InvalidOid, NULL, NULL);
/* Initialize stuff for bootstrap-file processing */
for (i = 0; i < MAXATTR; i++)
--- 494,501 ----
* Do backend-like initialization for bootstrap mode
*/
InitProcess();
! InitPostgres();
! InitPostgresPhase2(NULL, InvalidOid, NULL, NULL);
/* Initialize stuff for bootstrap-file processing */
for (i = 0; i < MAXATTR; i++)
Index: src/backend/postmaster/autovacuum.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/postmaster/autovacuum.c,v
retrieving revision 1.103
diff -c -p -r1.103 autovacuum.c
*** src/backend/postmaster/autovacuum.c 27 Aug 2009 17:18:44 -0000 1.103
--- src/backend/postmaster/autovacuum.c 31 Aug 2009 14:45:39 -0000
*************** StartAutoVacLauncher(void)
*** 386,391 ****
--- 386,392 ----
return 0;
}
+ bool in_transaction = false;
/*
* Main loop for the autovacuum launcher process.
*/
*************** AutoVacLauncherMain(int argc, char *argv
*** 424,432 ****
#endif
/*
! * Set up signal handlers. Since this is an auxiliary process, it has
! * particular signal requirements -- no deadlock checker or sinval
! * catchup, for example.
*/
pqsignal(SIGHUP, avl_sighup_handler);
--- 425,433 ----
#endif
/*
! * Set up signal handlers. We operate on databases much like a regular
! * backend, so we use the same signal handling. See equivalent code in
! * tcop/postgres.c.
*/
pqsignal(SIGHUP, avl_sighup_handler);
*************** AutoVacLauncherMain(int argc, char *argv
*** 451,459 ****
* had to do some stuff with LWLocks).
*/
#ifndef EXEC_BACKEND
! InitAuxiliaryProcess();
#endif
/*
* Create a memory context that we will do all our work in. We do this so
* that we can reset the context during error recovery and thereby avoid
--- 452,462 ----
* had to do some stuff with LWLocks).
*/
#ifndef EXEC_BACKEND
! InitProcess();
#endif
+ InitPostgres();
+
/*
* Create a memory context that we will do all our work in. We do this so
* that we can reset the context during error recovery and thereby avoid
*************** AutoVacLauncherMain(int argc, char *argv
*** 483,496 ****
/* Report the error to the server log */
EmitErrorReport();
! /*
! * These operations are really just a minimal subset of
! * AbortTransaction(). We don't have very many resources to worry
! * about, but we do have LWLocks.
! */
! LWLockReleaseAll();
! AtEOXact_Files();
! AtEOXact_HashTables(false);
/*
* Now return to normal top-level context and clear ErrorContext for
--- 486,507 ----
/* Report the error to the server log */
EmitErrorReport();
! if (in_transaction)
! {
! AbortCurrentTransaction();
! in_transaction = false;
! }
! else
! {
! /*
! * These operations are really just a minimal subset of
! * AbortTransaction(). We don't have very many resources to worry
! * about, but we do have LWLocks.
! */
! LWLockReleaseAll();
! AtEOXact_Files();
! AtEOXact_HashTables(false);
! }
/*
* Now return to normal top-level context and clear ErrorContext for
*************** AutoVacWorkerMain(int argc, char *argv[]
*** 1620,1626 ****
* Note: if we have selected a just-deleted database (due to using
* stale stats info), we'll fail and exit here.
*/
! InitPostgres(NULL, dbid, NULL, dbname);
SetProcessingMode(NormalProcessing);
set_ps_display(dbname, false);
ereport(DEBUG1,
--- 1631,1638 ----
* Note: if we have selected a just-deleted database (due to using
* stale stats info), we'll fail and exit here.
*/
! InitPostgres();
! InitPostgresPhase2(NULL, dbid, NULL, dbname);
SetProcessingMode(NormalProcessing);
set_ps_display(dbname, false);
ereport(DEBUG1,
*************** autovac_balance_cost(void)
*** 1784,1829 ****
/*
* get_database_list
*
! * Return a list of all databases. Note we cannot use pg_database,
! * because we aren't connected; we use the flat database file.
*/
static List *
get_database_list(void)
{
- char *filename;
List *dblist = NIL;
! char thisname[NAMEDATALEN];
! FILE *db_file;
! Oid db_id;
! Oid db_tablespace;
! TransactionId db_frozenxid;
!
! filename = database_getflatfilename();
! db_file = AllocateFile(filename, "r");
! if (db_file == NULL)
! ereport(FATAL,
! (errcode_for_file_access(),
! errmsg("could not open file \"%s\": %m", filename)));
! while (read_pg_database_line(db_file, thisname, &db_id,
! &db_tablespace, &db_frozenxid))
{
! avw_dbase *avdb;
avdb = (avw_dbase *) palloc(sizeof(avw_dbase));
! avdb->adw_datid = db_id;
! avdb->adw_name = pstrdup(thisname);
! avdb->adw_frozenxid = db_frozenxid;
/* this gets set later: */
avdb->adw_entry = NULL;
dblist = lappend(dblist, avdb);
}
! FreeFile(db_file);
! pfree(filename);
return dblist;
}
--- 1796,1843 ----
/*
* get_database_list
+ * Return a list of all databases.
*
! * Note: this is the only function in which the autovacuum launcher uses a
! * transaction.
*/
static List *
get_database_list(void)
{
List *dblist = NIL;
! Relation rel;
! HeapScanDesc scan;
! HeapTuple tup;
!
! StartTransactionCommand();
! in_transaction = true;
! MemoryContextSwitchTo(AutovacMemCxt);
!
! rel = heap_open(DatabaseRelationId, AccessShareLock);
! scan = heap_beginscan(rel, SnapshotNow, 0, NULL);
!
! while (HeapTupleIsValid(tup = heap_getnext(scan, ForwardScanDirection)))
{
! Form_pg_database pgdatabase = (Form_pg_database) GETSTRUCT(tup);
! avw_dbase *avdb;
avdb = (avw_dbase *) palloc(sizeof(avw_dbase));
! avdb->adw_datid = HeapTupleGetOid(tup);
! avdb->adw_name = pstrdup(NameStr(pgdatabase->datname));
! avdb->adw_frozenxid = pgdatabase->datfrozenxid;
/* this gets set later: */
avdb->adw_entry = NULL;
dblist = lappend(dblist, avdb);
}
! heap_endscan(scan);
! heap_close(rel, AccessShareLock);
!
! CommitTransactionCommand();
! in_transaction = false;
return dblist;
}
Index: src/backend/postmaster/postmaster.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.593
diff -c -p -r1.593 postmaster.c
*** src/backend/postmaster/postmaster.c 29 Aug 2009 19:26:51 -0000 1.593
--- src/backend/postmaster/postmaster.c 31 Aug 2009 13:37:29 -0000
*************** SubPostmasterMain(int argc, char *argv[]
*** 3865,3871 ****
InitShmemAccess(UsedShmemSegAddr);
/* Need a PGPROC to run CreateSharedMemoryAndSemaphores */
! InitAuxiliaryProcess();
/* Attach process to shared data structures */
CreateSharedMemoryAndSemaphores(false, 0);
--- 3865,3871 ----
InitShmemAccess(UsedShmemSegAddr);
/* Need a PGPROC to run CreateSharedMemoryAndSemaphores */
! InitProcess();
/* Attach process to shared data structures */
CreateSharedMemoryAndSemaphores(false, 0);
Index: src/backend/storage/lmgr/proc.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/storage/lmgr/proc.c,v
retrieving revision 1.208
diff -c -p -r1.208 proc.c
*** src/backend/storage/lmgr/proc.c 12 Aug 2009 20:53:30 -0000 1.208
--- src/backend/storage/lmgr/proc.c 31 Aug 2009 13:36:56 -0000
*************** ProcGlobalShmemSize(void)
*** 102,109 ****
size = add_size(size, sizeof(PROC_HDR));
/* AuxiliaryProcs */
size = add_size(size, mul_size(NUM_AUXILIARY_PROCS, sizeof(PGPROC)));
! /* MyProcs, including autovacuum */
! size = add_size(size, mul_size(MaxBackends, sizeof(PGPROC)));
/* ProcStructLock */
size = add_size(size, sizeof(slock_t));
--- 102,109 ----
size = add_size(size, sizeof(PROC_HDR));
/* AuxiliaryProcs */
size = add_size(size, mul_size(NUM_AUXILIARY_PROCS, sizeof(PGPROC)));
! /* MyProcs, including autovacuum workers and launcher */
! size = add_size(size, mul_size(MaxBackends + 1, sizeof(PGPROC)));
/* ProcStructLock */
size = add_size(size, sizeof(slock_t));
*************** InitProcGlobal(void)
*** 192,204 ****
ProcGlobal->freeProcs = &procs[i];
}
! procs = (PGPROC *) ShmemAlloc((autovacuum_max_workers) * sizeof(PGPROC));
if (!procs)
ereport(FATAL,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of shared memory")));
! MemSet(procs, 0, autovacuum_max_workers * sizeof(PGPROC));
! for (i = 0; i < autovacuum_max_workers; i++)
{
PGSemaphoreCreate(&(procs[i].sem));
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->autovacFreeProcs;
--- 192,204 ----
ProcGlobal->freeProcs = &procs[i];
}
! procs = (PGPROC *) ShmemAlloc((autovacuum_max_workers + 1) * sizeof(PGPROC));
if (!procs)
ereport(FATAL,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of shared memory")));
! MemSet(procs, 0, (autovacuum_max_workers + 1) * sizeof(PGPROC));
! for (i = 0; i < autovacuum_max_workers + 1; i++)
{
PGSemaphoreCreate(&(procs[i].sem));
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->autovacFreeProcs;
*************** InitProcess(void)
*** 248,261 ****
set_spins_per_delay(procglobal->spins_per_delay);
! if (IsAutoVacuumWorkerProcess())
MyProc = procglobal->autovacFreeProcs;
else
MyProc = procglobal->freeProcs;
if (MyProc != NULL)
{
! if (IsAutoVacuumWorkerProcess())
procglobal->autovacFreeProcs = (PGPROC *) MyProc->links.next;
else
procglobal->freeProcs = (PGPROC *) MyProc->links.next;
--- 248,261 ----
set_spins_per_delay(procglobal->spins_per_delay);
! if (IsAnyAutoVacuumProcess())
MyProc = procglobal->autovacFreeProcs;
else
MyProc = procglobal->freeProcs;
if (MyProc != NULL)
{
! if (IsAnyAutoVacuumProcess())
procglobal->autovacFreeProcs = (PGPROC *) MyProc->links.next;
else
procglobal->freeProcs = (PGPROC *) MyProc->links.next;
*************** InitProcess(void)
*** 280,286 ****
* child; this is so that the postmaster can detect it if we exit without
* cleaning up.
*/
! if (IsUnderPostmaster)
MarkPostmasterChildActive();
/*
--- 280,286 ----
* child; this is so that the postmaster can detect it if we exit without
* cleaning up.
*/
! if (IsUnderPostmaster && !IsAutoVacuumLauncherProcess())
MarkPostmasterChildActive();
/*
*************** InitProcess(void)
*** 299,304 ****
--- 299,305 ----
MyProc->roleId = InvalidOid;
MyProc->inCommit = false;
MyProc->vacuumFlags = 0;
+ /* NB -- autovac launcher does not set this flag to avoid getting killed */
if (IsAutoVacuumWorkerProcess())
MyProc->vacuumFlags |= PROC_IS_AUTOVACUUM;
MyProc->lwWaiting = false;
*************** InitAuxiliaryProcess(void)
*** 429,435 ****
MyProc->databaseId = InvalidOid;
MyProc->roleId = InvalidOid;
MyProc->inCommit = false;
- /* we don't set the "is autovacuum" flag in the launcher */
MyProc->vacuumFlags = 0;
MyProc->lwWaiting = false;
MyProc->lwExclusive = false;
--- 430,435 ----
*************** ProcKill(int code, Datum arg)
*** 596,602 ****
SpinLockAcquire(ProcStructLock);
/* Return PGPROC structure (and semaphore) to freelist */
! if (IsAutoVacuumWorkerProcess())
{
MyProc->links.next = (SHM_QUEUE *) procglobal->autovacFreeProcs;
procglobal->autovacFreeProcs = MyProc;
--- 596,602 ----
SpinLockAcquire(ProcStructLock);
/* Return PGPROC structure (and semaphore) to freelist */
! if (IsAnyAutoVacuumProcess())
{
MyProc->links.next = (SHM_QUEUE *) procglobal->autovacFreeProcs;
procglobal->autovacFreeProcs = MyProc;
*************** ProcKill(int code, Datum arg)
*** 619,625 ****
* This process is no longer present in shared memory in any meaningful
* way, so tell the postmaster we've cleaned up acceptably well.
*/
! if (IsUnderPostmaster)
MarkPostmasterChildInactive();
/* wake autovac launcher if needed -- see comments in FreeWorkerInfo */
--- 619,625 ----
* This process is no longer present in shared memory in any meaningful
* way, so tell the postmaster we've cleaned up acceptably well.
*/
! if (IsUnderPostmaster && !IsAutoVacuumLauncherProcess())
MarkPostmasterChildInactive();
/* wake autovac launcher if needed -- see comments in FreeWorkerInfo */
Index: src/backend/tcop/postgres.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/tcop/postgres.c,v
retrieving revision 1.571
diff -c -p -r1.571 postgres.c
*** src/backend/tcop/postgres.c 29 Aug 2009 19:26:51 -0000 1.571
--- src/backend/tcop/postgres.c 31 Aug 2009 13:56:52 -0000
*************** PostgresMain(int argc, char *argv[], con
*** 3314,3320 ****
* it inside InitPostgres() instead. In particular, anything that
* involves database access should be there, not here.
*/
! am_superuser = InitPostgres(dbname, InvalidOid, username, NULL);
/*
* If the PostmasterContext is still around, recycle the space; we don't
--- 3314,3321 ----
* it inside InitPostgres() instead. In particular, anything that
* involves database access should be there, not here.
*/
! InitPostgres();
! am_superuser = InitPostgresPhase2(dbname, InvalidOid, username, NULL);
/*
* If the PostmasterContext is still around, recycle the space; we don't
Index: src/backend/utils/init/postinit.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/init/postinit.c,v
retrieving revision 1.195
diff -c -p -r1.195 postinit.c
*** src/backend/utils/init/postinit.c 29 Aug 2009 19:26:51 -0000 1.195
--- src/backend/utils/init/postinit.c 31 Aug 2009 14:54:30 -0000
*************** BaseInit(void)
*** 455,470 ****
* InitPostgres
* Initialize POSTGRES.
*
! * The database can be specified by name, using the in_dbname parameter, or by
! * OID, using the dboid parameter. In the latter case, the actual database
! * name can be returned to the caller in out_dbname. If out_dbname isn't
! * NULL, it must point to a buffer of size NAMEDATALEN.
! *
! * In bootstrap mode no parameters are used.
! *
! * The return value indicates whether the userID is a superuser. (That
! * can only be tested inside a transaction, so we want to do it during
! * the startup transaction rather than doing a separate one in postgres.c.)
*
* As of PostgreSQL 8.2, we expect InitProcess() was already called, so we
* already have a PGPROC struct ... but it's not completely filled in yet.
--- 455,464 ----
* InitPostgres
* Initialize POSTGRES.
*
! * As of Postgres 8.5, this is split in two phases. The first phase prepares
! * things so that a process can have access to shared catalogs (only
! * pg_database at the moment actually). Processes needing access to a specific
! * database can connect to it by executing the second phase.
*
* As of PostgreSQL 8.2, we expect InitProcess() was already called, so we
* already have a PGPROC struct ... but it's not completely filled in yet.
*************** BaseInit(void)
*** 473,487 ****
* Be very careful with the order of calls in the InitPostgres function.
* --------------------------------
*/
! bool
! InitPostgres(const char *in_dbname, Oid dboid, const char *username,
! char *out_dbname)
{
bool bootstrap = IsBootstrapProcessingMode();
- bool autovacuum = IsAutoVacuumWorkerProcess();
- bool am_superuser;
- char *fullpath;
- char dbname[NAMEDATALEN];
elog(DEBUG3, "InitPostgres");
--- 467,476 ----
* Be very careful with the order of calls in the InitPostgres function.
* --------------------------------
*/
! void
! InitPostgres(void)
{
bool bootstrap = IsBootstrapProcessingMode();
elog(DEBUG3, "InitPostgres");
*************** InitPostgres(const char *in_dbname, Oid
*** 553,559 ****
* snapshot. We don't have a use for the snapshot itself, but we're
* interested in the secondary effect that it sets RecentGlobalXmin.
*/
! if (!bootstrap)
{
StartTransactionCommand();
(void) GetTransactionSnapshot();
--- 542,548 ----
* snapshot. We don't have a use for the snapshot itself, but we're
* interested in the secondary effect that it sets RecentGlobalXmin.
*/
! if (!bootstrap && !IsAutoVacuumLauncherProcess())
{
StartTransactionCommand();
(void) GetTransactionSnapshot();
*************** InitPostgres(const char *in_dbname, Oid
*** 564,569 ****
--- 553,583 ----
* create at least an entry for pg_database.
*/
RelationCacheInitializePhase2();
+ }
+
+ /*
+ * Second phase of InitPostgres.
+ *
+ * The database can be specified by name, using the in_dbname parameter, or by
+ * OID, using the dboid parameter. In the latter case, the actual database
+ * name can be returned to the caller in out_dbname. If out_dbname isn't
+ * NULL, it must point to a buffer of size NAMEDATALEN.
+ *
+ * In bootstrap mode no parameters are used.
+ *
+ * The return value indicates whether the userID is a superuser. (That
+ * can only be tested inside a transaction, so we want to do it during
+ * the startup transaction rather than doing a separate one in postgres.c.)
+ */
+ bool
+ InitPostgresPhase2(const char *in_dbname, Oid dboid, const char *username,
+ char *out_dbname)
+ {
+ bool bootstrap = IsBootstrapProcessingMode();
+ bool autovacuum = IsAutoVacuumWorkerProcess();
+ bool am_superuser;
+ char *fullpath;
+ char dbname[NAMEDATALEN];
/*
* Set up the global variables holding database id and default tablespace.
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/misc/guc.c,v
retrieving revision 1.513
diff -c -p -r1.513 guc.c
*** src/backend/utils/misc/guc.c 31 Aug 2009 02:23:22 -0000 1.513
--- src/backend/utils/misc/guc.c 31 Aug 2009 03:07:47 -0000
*************** static struct config_int ConfigureNamesI
*** 1335,1341 ****
* Note: MaxBackends is limited to INT_MAX/4 because some places compute
* 4*MaxBackends without any overflow check. This check is made in
* assign_maxconnections, since MaxBackends is computed as MaxConnections
! * plus autovacuum_max_workers.
*
* Likewise we have to limit NBuffers to INT_MAX/2.
*/
--- 1335,1341 ----
* Note: MaxBackends is limited to INT_MAX/4 because some places compute
* 4*MaxBackends without any overflow check. This check is made in
* assign_maxconnections, since MaxBackends is computed as MaxConnections
! * plus autovacuum_max_workers plus one (for the autovacuum launcher).
*
* Likewise we have to limit NBuffers to INT_MAX/2.
*/
*************** assign_maxconnections(int newval, bool d
*** 7574,7580 ****
return false;
if (doit)
! MaxBackends = newval + autovacuum_max_workers;
return true;
}
--- 7574,7580 ----
return false;
if (doit)
! MaxBackends = newval + autovacuum_max_workers + 1;
return true;
}
*************** assign_autovacuum_max_workers(int newval
*** 7586,7592 ****
return false;
if (doit)
! MaxBackends = newval + MaxConnections;
return true;
}
--- 7586,7592 ----
return false;
if (doit)
! MaxBackends = newval + MaxConnections + 1;
return true;
}
Index: src/include/miscadmin.h
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/include/miscadmin.h,v
retrieving revision 1.213
diff -c -p -r1.213 miscadmin.h
*** src/include/miscadmin.h 29 Aug 2009 19:26:51 -0000 1.213
--- src/include/miscadmin.h 31 Aug 2009 13:57:20 -0000
*************** extern ProcessingMode Mode;
*** 324,330 ****
/* in utils/init/postinit.c */
extern void pg_split_opts(char **argv, int *argcp, char *optstr);
! extern bool InitPostgres(const char *in_dbname, Oid dboid, const char *username,
char *out_dbname);
extern void BaseInit(void);
--- 324,331 ----
/* in utils/init/postinit.c */
extern void pg_split_opts(char **argv, int *argcp, char *optstr);
! extern void InitPostgres(void);
! extern bool InitPostgresPhase2(const char *in_dbname, Oid dboid, const char *username,
char *out_dbname);
extern void BaseInit(void);
Index: src/include/postmaster/autovacuum.h
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/include/postmaster/autovacuum.h,v
retrieving revision 1.15
diff -c -p -r1.15 autovacuum.h
*** src/include/postmaster/autovacuum.h 1 Jan 2009 17:24:01 -0000 1.15
--- src/include/postmaster/autovacuum.h 31 Aug 2009 03:29:17 -0000
*************** extern int Log_autovacuum_min_duration;
*** 37,42 ****
--- 37,44 ----
extern bool AutoVacuumingActive(void);
extern bool IsAutoVacuumLauncherProcess(void);
extern bool IsAutoVacuumWorkerProcess(void);
+ #define IsAnyAutoVacuumProcess() (IsAutoVacuumLauncherProcess() || \
+ IsAutoVacuumWorkerProcess())
/* Functions to start autovacuum process, called from postmaster */
extern void autovac_init(void);
Index: src/include/storage/proc.h
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/include/storage/proc.h,v
retrieving revision 1.113
diff -c -p -r1.113 proc.h
*** src/include/storage/proc.h 12 Aug 2009 20:53:30 -0000 1.113
--- src/include/storage/proc.h 31 Aug 2009 03:08:42 -0000
*************** typedef struct PROC_HDR
*** 141,152 ****
* We set aside some extra PGPROC structures for auxiliary processes,
* ie things that aren't full-fledged backends but need shmem access.
*
! * Background writer, WAL writer, and autovacuum launcher run during
! * normal operation. Startup process also consumes one slot, but WAL
! * writer and autovacuum launcher are launched only after it has
* exited.
*/
! #define NUM_AUXILIARY_PROCS 3
/* configurable options */
--- 141,151 ----
* We set aside some extra PGPROC structures for auxiliary processes,
* ie things that aren't full-fledged backends but need shmem access.
*
! * Background writer and WAL writer run during normal operation. Startup
! * process also consumes one slot, but WAL writer is launched only after it has
* exited.
*/
! #define NUM_AUXILIARY_PROCS 2
/* configurable options */
Alvaro Herrera <alvherre@commandprompt.com> writes:
To this end, InitPostgres has been split in two. The launcher only
calls the first half, the rest of the callers have been patched to
invoke the second half.
This just seems truly messy :-(. Let me see if I can find something
cleaner.
BTW, is it *really* the case that the AV launcher won't need
RecentGlobalXmin? The way the HOT stuff works, I think anything that
examines heap pages at all had better have that set.
regards, tom lane
Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
To this end, InitPostgres has been split in two. The launcher only
calls the first half, the rest of the callers have been patched to
invoke the second half.This just seems truly messy :-(. Let me see if I can find something
cleaner.
I was considering having InitPostgres be an umbrella function, so that
extant callers stay as today, but the various underlying pieces are
skipped depending on who's calling. For example I didn't like the bit
about starting a transaction or not depending on whether it was the
launcher.
BTW, is it *really* the case that the AV launcher won't need
RecentGlobalXmin? The way the HOT stuff works, I think anything that
examines heap pages at all had better have that set.
Ugh. I forgot about that.
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tom Lane wrote:
This just seems truly messy :-(. Let me see if I can find something
cleaner.
I was considering having InitPostgres be an umbrella function, so that
extant callers stay as today, but the various underlying pieces are
skipped depending on who's calling. For example I didn't like the bit
about starting a transaction or not depending on whether it was the
launcher.
Yeah. If you have InitPostgres know that much about the AV launcher's
requirements, it's not clear why it shouldn't just know everything.
Having it return with the initial transaction still open just seems
completely horrid.
BTW, is it *really* the case that the AV launcher won't need
RecentGlobalXmin? The way the HOT stuff works, I think anything that
examines heap pages at all had better have that set.
Ugh. I forgot about that.
We could possibly put
if (IsAutovacuumLauncher())
{
CommitTransactionCommand();
return;
}
right after the RelationCacheInitializePhase2 call. No uglier than
what's here, for sure.
While I was looking at this I wondered whether
RelationCacheInitializePhase2 really needs to be inside the startup
transaction at all. I think it could probably be moved up before
that. However, if the AV launcher has to do GetTransactionSnapshot
then it's not clear that improves matters anyway.
regards, tom lane
Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tom Lane wrote:
This just seems truly messy :-(. Let me see if I can find something
cleaner.I was considering having InitPostgres be an umbrella function, so that
extant callers stay as today, but the various underlying pieces are
skipped depending on who's calling. For example I didn't like the bit
about starting a transaction or not depending on whether it was the
launcher.Yeah. If you have InitPostgres know that much about the AV launcher's
requirements, it's not clear why it shouldn't just know everything.
Having it return with the initial transaction still open just seems
completely horrid.
How about this?
While I was looking at this I wondered whether
RelationCacheInitializePhase2 really needs to be inside the startup
transaction at all. I think it could probably be moved up before
that. However, if the AV launcher has to do GetTransactionSnapshot
then it's not clear that improves matters anyway.
Well, the difference is that the initial transaction would be a few
microsec shorter ... not sure if that matters.
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Attachments:
avlauncher-proc-2.patchtext/x-diff; charset=us-asciiDownload
Index: src/backend/postmaster/autovacuum.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/postmaster/autovacuum.c,v
retrieving revision 1.103
diff -c -p -r1.103 autovacuum.c
*** src/backend/postmaster/autovacuum.c 27 Aug 2009 17:18:44 -0000 1.103
--- src/backend/postmaster/autovacuum.c 31 Aug 2009 15:49:14 -0000
*************** AutoVacLauncherMain(int argc, char *argv
*** 424,432 ****
#endif
/*
! * Set up signal handlers. Since this is an auxiliary process, it has
! * particular signal requirements -- no deadlock checker or sinval
! * catchup, for example.
*/
pqsignal(SIGHUP, avl_sighup_handler);
--- 424,432 ----
#endif
/*
! * Set up signal handlers. We operate on databases much like a regular
! * backend, so we use the same signal handling. See equivalent code in
! * tcop/postgres.c.
*/
pqsignal(SIGHUP, avl_sighup_handler);
*************** AutoVacLauncherMain(int argc, char *argv
*** 451,459 ****
* had to do some stuff with LWLocks).
*/
#ifndef EXEC_BACKEND
! InitAuxiliaryProcess();
#endif
/*
* Create a memory context that we will do all our work in. We do this so
* that we can reset the context during error recovery and thereby avoid
--- 451,461 ----
* had to do some stuff with LWLocks).
*/
#ifndef EXEC_BACKEND
! InitProcess();
#endif
+ InitPostgres(NULL, InvalidOid, NULL, NULL);
+
/*
* Create a memory context that we will do all our work in. We do this so
* that we can reset the context during error recovery and thereby avoid
*************** AutoVacLauncherMain(int argc, char *argv
*** 470,476 ****
/*
* If an exception is encountered, processing resumes here.
*
! * This code is heavily based on bgwriter.c, q.v.
*/
if (sigsetjmp(local_sigjmp_buf, 1) != 0)
{
--- 472,478 ----
/*
* If an exception is encountered, processing resumes here.
*
! * This code is a stripped down version of PostgresMain error recovery.
*/
if (sigsetjmp(local_sigjmp_buf, 1) != 0)
{
*************** AutoVacLauncherMain(int argc, char *argv
*** 483,496 ****
/* Report the error to the server log */
EmitErrorReport();
! /*
! * These operations are really just a minimal subset of
! * AbortTransaction(). We don't have very many resources to worry
! * about, but we do have LWLocks.
! */
! LWLockReleaseAll();
! AtEOXact_Files();
! AtEOXact_HashTables(false);
/*
* Now return to normal top-level context and clear ErrorContext for
--- 485,492 ----
/* Report the error to the server log */
EmitErrorReport();
! /* Abort the current transaction in order to recover */
! AbortCurrentTransaction();
/*
* Now return to normal top-level context and clear ErrorContext for
*************** autovac_balance_cost(void)
*** 1784,1829 ****
/*
* get_database_list
*
! * Return a list of all databases. Note we cannot use pg_database,
! * because we aren't connected; we use the flat database file.
*/
static List *
get_database_list(void)
{
- char *filename;
List *dblist = NIL;
! char thisname[NAMEDATALEN];
! FILE *db_file;
! Oid db_id;
! Oid db_tablespace;
! TransactionId db_frozenxid;
!
! filename = database_getflatfilename();
! db_file = AllocateFile(filename, "r");
! if (db_file == NULL)
! ereport(FATAL,
! (errcode_for_file_access(),
! errmsg("could not open file \"%s\": %m", filename)));
! while (read_pg_database_line(db_file, thisname, &db_id,
! &db_tablespace, &db_frozenxid))
{
! avw_dbase *avdb;
avdb = (avw_dbase *) palloc(sizeof(avw_dbase));
! avdb->adw_datid = db_id;
! avdb->adw_name = pstrdup(thisname);
! avdb->adw_frozenxid = db_frozenxid;
/* this gets set later: */
avdb->adw_entry = NULL;
dblist = lappend(dblist, avdb);
}
! FreeFile(db_file);
! pfree(filename);
return dblist;
}
--- 1780,1825 ----
/*
* get_database_list
+ * Return a list of all databases found in pg_database.
*
! * Note: this is the only function in which the autovacuum launcher uses a
! * transaction.
*/
static List *
get_database_list(void)
{
List *dblist = NIL;
! Relation rel;
! HeapScanDesc scan;
! HeapTuple tup;
! StartTransactionCommand();
!
! MemoryContextSwitchTo(AutovacMemCxt);
!
! rel = heap_open(DatabaseRelationId, AccessShareLock);
! scan = heap_beginscan(rel, SnapshotNow, 0, NULL);
!
! while (HeapTupleIsValid(tup = heap_getnext(scan, ForwardScanDirection)))
{
! Form_pg_database pgdatabase = (Form_pg_database) GETSTRUCT(tup);
! avw_dbase *avdb;
avdb = (avw_dbase *) palloc(sizeof(avw_dbase));
! avdb->adw_datid = HeapTupleGetOid(tup);
! avdb->adw_name = pstrdup(NameStr(pgdatabase->datname));
! avdb->adw_frozenxid = pgdatabase->datfrozenxid;
/* this gets set later: */
avdb->adw_entry = NULL;
dblist = lappend(dblist, avdb);
}
! heap_endscan(scan);
! heap_close(rel, AccessShareLock);
!
! CommitTransactionCommand();
return dblist;
}
Index: src/backend/postmaster/postmaster.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.593
diff -c -p -r1.593 postmaster.c
*** src/backend/postmaster/postmaster.c 29 Aug 2009 19:26:51 -0000 1.593
--- src/backend/postmaster/postmaster.c 31 Aug 2009 13:37:29 -0000
*************** SubPostmasterMain(int argc, char *argv[]
*** 3865,3871 ****
InitShmemAccess(UsedShmemSegAddr);
/* Need a PGPROC to run CreateSharedMemoryAndSemaphores */
! InitAuxiliaryProcess();
/* Attach process to shared data structures */
CreateSharedMemoryAndSemaphores(false, 0);
--- 3865,3871 ----
InitShmemAccess(UsedShmemSegAddr);
/* Need a PGPROC to run CreateSharedMemoryAndSemaphores */
! InitProcess();
/* Attach process to shared data structures */
CreateSharedMemoryAndSemaphores(false, 0);
Index: src/backend/storage/lmgr/proc.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/storage/lmgr/proc.c,v
retrieving revision 1.208
diff -c -p -r1.208 proc.c
*** src/backend/storage/lmgr/proc.c 12 Aug 2009 20:53:30 -0000 1.208
--- src/backend/storage/lmgr/proc.c 31 Aug 2009 13:36:56 -0000
*************** ProcGlobalShmemSize(void)
*** 102,109 ****
size = add_size(size, sizeof(PROC_HDR));
/* AuxiliaryProcs */
size = add_size(size, mul_size(NUM_AUXILIARY_PROCS, sizeof(PGPROC)));
! /* MyProcs, including autovacuum */
! size = add_size(size, mul_size(MaxBackends, sizeof(PGPROC)));
/* ProcStructLock */
size = add_size(size, sizeof(slock_t));
--- 102,109 ----
size = add_size(size, sizeof(PROC_HDR));
/* AuxiliaryProcs */
size = add_size(size, mul_size(NUM_AUXILIARY_PROCS, sizeof(PGPROC)));
! /* MyProcs, including autovacuum workers and launcher */
! size = add_size(size, mul_size(MaxBackends + 1, sizeof(PGPROC)));
/* ProcStructLock */
size = add_size(size, sizeof(slock_t));
*************** InitProcGlobal(void)
*** 192,204 ****
ProcGlobal->freeProcs = &procs[i];
}
! procs = (PGPROC *) ShmemAlloc((autovacuum_max_workers) * sizeof(PGPROC));
if (!procs)
ereport(FATAL,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of shared memory")));
! MemSet(procs, 0, autovacuum_max_workers * sizeof(PGPROC));
! for (i = 0; i < autovacuum_max_workers; i++)
{
PGSemaphoreCreate(&(procs[i].sem));
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->autovacFreeProcs;
--- 192,204 ----
ProcGlobal->freeProcs = &procs[i];
}
! procs = (PGPROC *) ShmemAlloc((autovacuum_max_workers + 1) * sizeof(PGPROC));
if (!procs)
ereport(FATAL,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of shared memory")));
! MemSet(procs, 0, (autovacuum_max_workers + 1) * sizeof(PGPROC));
! for (i = 0; i < autovacuum_max_workers + 1; i++)
{
PGSemaphoreCreate(&(procs[i].sem));
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->autovacFreeProcs;
*************** InitProcess(void)
*** 248,261 ****
set_spins_per_delay(procglobal->spins_per_delay);
! if (IsAutoVacuumWorkerProcess())
MyProc = procglobal->autovacFreeProcs;
else
MyProc = procglobal->freeProcs;
if (MyProc != NULL)
{
! if (IsAutoVacuumWorkerProcess())
procglobal->autovacFreeProcs = (PGPROC *) MyProc->links.next;
else
procglobal->freeProcs = (PGPROC *) MyProc->links.next;
--- 248,261 ----
set_spins_per_delay(procglobal->spins_per_delay);
! if (IsAnyAutoVacuumProcess())
MyProc = procglobal->autovacFreeProcs;
else
MyProc = procglobal->freeProcs;
if (MyProc != NULL)
{
! if (IsAnyAutoVacuumProcess())
procglobal->autovacFreeProcs = (PGPROC *) MyProc->links.next;
else
procglobal->freeProcs = (PGPROC *) MyProc->links.next;
*************** InitProcess(void)
*** 280,286 ****
* child; this is so that the postmaster can detect it if we exit without
* cleaning up.
*/
! if (IsUnderPostmaster)
MarkPostmasterChildActive();
/*
--- 280,286 ----
* child; this is so that the postmaster can detect it if we exit without
* cleaning up.
*/
! if (IsUnderPostmaster && !IsAutoVacuumLauncherProcess())
MarkPostmasterChildActive();
/*
*************** InitProcess(void)
*** 299,304 ****
--- 299,305 ----
MyProc->roleId = InvalidOid;
MyProc->inCommit = false;
MyProc->vacuumFlags = 0;
+ /* NB -- autovac launcher does not set this flag to avoid getting killed */
if (IsAutoVacuumWorkerProcess())
MyProc->vacuumFlags |= PROC_IS_AUTOVACUUM;
MyProc->lwWaiting = false;
*************** InitAuxiliaryProcess(void)
*** 429,435 ****
MyProc->databaseId = InvalidOid;
MyProc->roleId = InvalidOid;
MyProc->inCommit = false;
- /* we don't set the "is autovacuum" flag in the launcher */
MyProc->vacuumFlags = 0;
MyProc->lwWaiting = false;
MyProc->lwExclusive = false;
--- 430,435 ----
*************** ProcKill(int code, Datum arg)
*** 596,602 ****
SpinLockAcquire(ProcStructLock);
/* Return PGPROC structure (and semaphore) to freelist */
! if (IsAutoVacuumWorkerProcess())
{
MyProc->links.next = (SHM_QUEUE *) procglobal->autovacFreeProcs;
procglobal->autovacFreeProcs = MyProc;
--- 596,602 ----
SpinLockAcquire(ProcStructLock);
/* Return PGPROC structure (and semaphore) to freelist */
! if (IsAnyAutoVacuumProcess())
{
MyProc->links.next = (SHM_QUEUE *) procglobal->autovacFreeProcs;
procglobal->autovacFreeProcs = MyProc;
*************** ProcKill(int code, Datum arg)
*** 619,625 ****
* This process is no longer present in shared memory in any meaningful
* way, so tell the postmaster we've cleaned up acceptably well.
*/
! if (IsUnderPostmaster)
MarkPostmasterChildInactive();
/* wake autovac launcher if needed -- see comments in FreeWorkerInfo */
--- 619,625 ----
* This process is no longer present in shared memory in any meaningful
* way, so tell the postmaster we've cleaned up acceptably well.
*/
! if (IsUnderPostmaster && !IsAutoVacuumLauncherProcess())
MarkPostmasterChildInactive();
/* wake autovac launcher if needed -- see comments in FreeWorkerInfo */
Index: src/backend/utils/init/postinit.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/init/postinit.c,v
retrieving revision 1.195
diff -c -p -r1.195 postinit.c
*** src/backend/utils/init/postinit.c 29 Aug 2009 19:26:51 -0000 1.195
--- src/backend/utils/init/postinit.c 31 Aug 2009 15:48:10 -0000
*************** InitPostgres(const char *in_dbname, Oid
*** 552,557 ****
--- 552,560 ----
* Start a new transaction here before first access to db, and get a
* snapshot. We don't have a use for the snapshot itself, but we're
* interested in the secondary effect that it sets RecentGlobalXmin.
+ * This is critical for anything that reads heap pages, because HOT
+ * may decide to prune them even if the process doesn't attempt to
+ * modify it.
*/
if (!bootstrap)
{
*************** InitPostgres(const char *in_dbname, Oid
*** 565,570 ****
--- 568,577 ----
*/
RelationCacheInitializePhase2();
+ /* The autovacuum launcher is done here */
+ if (IsAutoVacuumLauncherProcess())
+ goto done;
+
/*
* Set up the global variables holding database id and default tablespace.
* But note we won't actually try to touch the database just yet.
*************** InitPostgres(const char *in_dbname, Oid
*** 774,779 ****
--- 781,787 ----
if (!bootstrap)
pgstat_bestart();
+ done:
/* close the transaction we started above */
if (!bootstrap)
CommitTransactionCommand();
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/misc/guc.c,v
retrieving revision 1.513
diff -c -p -r1.513 guc.c
*** src/backend/utils/misc/guc.c 31 Aug 2009 02:23:22 -0000 1.513
--- src/backend/utils/misc/guc.c 31 Aug 2009 03:07:47 -0000
*************** static struct config_int ConfigureNamesI
*** 1335,1341 ****
* Note: MaxBackends is limited to INT_MAX/4 because some places compute
* 4*MaxBackends without any overflow check. This check is made in
* assign_maxconnections, since MaxBackends is computed as MaxConnections
! * plus autovacuum_max_workers.
*
* Likewise we have to limit NBuffers to INT_MAX/2.
*/
--- 1335,1341 ----
* Note: MaxBackends is limited to INT_MAX/4 because some places compute
* 4*MaxBackends without any overflow check. This check is made in
* assign_maxconnections, since MaxBackends is computed as MaxConnections
! * plus autovacuum_max_workers plus one (for the autovacuum launcher).
*
* Likewise we have to limit NBuffers to INT_MAX/2.
*/
*************** assign_maxconnections(int newval, bool d
*** 7574,7580 ****
return false;
if (doit)
! MaxBackends = newval + autovacuum_max_workers;
return true;
}
--- 7574,7580 ----
return false;
if (doit)
! MaxBackends = newval + autovacuum_max_workers + 1;
return true;
}
*************** assign_autovacuum_max_workers(int newval
*** 7586,7592 ****
return false;
if (doit)
! MaxBackends = newval + MaxConnections;
return true;
}
--- 7586,7592 ----
return false;
if (doit)
! MaxBackends = newval + MaxConnections + 1;
return true;
}
Index: src/include/postmaster/autovacuum.h
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/include/postmaster/autovacuum.h,v
retrieving revision 1.15
diff -c -p -r1.15 autovacuum.h
*** src/include/postmaster/autovacuum.h 1 Jan 2009 17:24:01 -0000 1.15
--- src/include/postmaster/autovacuum.h 31 Aug 2009 03:29:17 -0000
*************** extern int Log_autovacuum_min_duration;
*** 37,42 ****
--- 37,44 ----
extern bool AutoVacuumingActive(void);
extern bool IsAutoVacuumLauncherProcess(void);
extern bool IsAutoVacuumWorkerProcess(void);
+ #define IsAnyAutoVacuumProcess() (IsAutoVacuumLauncherProcess() || \
+ IsAutoVacuumWorkerProcess())
/* Functions to start autovacuum process, called from postmaster */
extern void autovac_init(void);
Index: src/include/storage/proc.h
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/include/storage/proc.h,v
retrieving revision 1.113
diff -c -p -r1.113 proc.h
*** src/include/storage/proc.h 12 Aug 2009 20:53:30 -0000 1.113
--- src/include/storage/proc.h 31 Aug 2009 03:08:42 -0000
*************** typedef struct PROC_HDR
*** 141,152 ****
* We set aside some extra PGPROC structures for auxiliary processes,
* ie things that aren't full-fledged backends but need shmem access.
*
! * Background writer, WAL writer, and autovacuum launcher run during
! * normal operation. Startup process also consumes one slot, but WAL
! * writer and autovacuum launcher are launched only after it has
* exited.
*/
! #define NUM_AUXILIARY_PROCS 3
/* configurable options */
--- 141,151 ----
* We set aside some extra PGPROC structures for auxiliary processes,
* ie things that aren't full-fledged backends but need shmem access.
*
! * Background writer and WAL writer run during normal operation. Startup
! * process also consumes one slot, but WAL writer is launched only after it has
* exited.
*/
! #define NUM_AUXILIARY_PROCS 2
/* configurable options */
Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tom Lane wrote:
This just seems truly messy :-(. Let me see if I can find something
cleaner.
I quite like the idea of splitting initialization into two phases, one
that let's you access shared catalogs, and one to bind to a database. I
didn't look into the details, though.
I was considering having InitPostgres be an umbrella function, so that
extant callers stay as today, but the various underlying pieces are
skipped depending on who's calling. For example I didn't like the bit
about starting a transaction or not depending on whether it was the
launcher.Yeah. If you have InitPostgres know that much about the AV launcher's
requirements, it's not clear why it shouldn't just know everything.
Having it return with the initial transaction still open just seems
completely horrid.
Yeah, that sounds messy. Can AV launcher simply open a 2nd initial
transaction?
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas wrote:
Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tom Lane wrote:
This just seems truly messy :-(. Let me see if I can find something
cleaner.I quite like the idea of splitting initialization into two phases, one
that let's you access shared catalogs, and one to bind to a database. I
didn't look into the details, though.
The problem is that it only gives you access to pg_database, because the
other shared catalogs require more relcache initialization than we do.
So I'm not sure it'd be useful for anything else.
I was considering having InitPostgres be an umbrella function, so that
extant callers stay as today, but the various underlying pieces are
skipped depending on who's calling. For example I didn't like the bit
about starting a transaction or not depending on whether it was the
launcher.Yeah. If you have InitPostgres know that much about the AV launcher's
requirements, it's not clear why it shouldn't just know everything.
Having it return with the initial transaction still open just seems
completely horrid.Yeah, that sounds messy. Can AV launcher simply open a 2nd initial
transaction?
The main body of avlauncher opens a new transaction whenever it needs
one. The problem is that the transaction in InitPostgres is closed in
the second half -- the code I had was skipping StartTransactionCommand
in the launcher case, but as Tom says that was wrong because it was
failing to set RecentGlobalXmin.
If we're looking at simplifing InitPostgres, one thing we could do is
separate the part for the bootstrap process, which is like 10% of the
work for a regular backend ...
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
How about this?
I think the accounting for the AV launcher in shmem allocation is a bit
confused yet --- for instance, isn't MaxBackends already including the
launcher? I wonder if it would be cleaner to include the launcher in
the autovacuum_max_workers parameter, and increase the min/default
values of that by one.
While I was looking at this I wondered whether
RelationCacheInitializePhase2 really needs to be inside the startup
transaction at all. I think it could probably be moved up before
that. However, if the AV launcher has to do GetTransactionSnapshot
then it's not clear that improves matters anyway.
Well, the difference is that the initial transaction would be a few
microsec shorter ... not sure if that matters.
I can't see how it would. At the point where that runs we'd not be
holding any locks of interest, so there's no concurrency benefit
to be gained. With this setup it wouldn't make InitPostgres any
cleaner anyway, so I'd leave it alone.
regards, tom lane
Alvaro Herrera <alvherre@commandprompt.com> writes:
Heikki Linnakangas wrote:
I quite like the idea of splitting initialization into two phases, one
that let's you access shared catalogs, and one to bind to a database. I
didn't look into the details, though.
The problem is that it only gives you access to pg_database, because the
other shared catalogs require more relcache initialization than we do.
So I'm not sure it'd be useful for anything else.
The part I was unhappy about was falling out with the startup
transaction still open (which the patch as written didn't do, but
would have needed to given the snapshot issue). I don't object to
trying to restructure InitPostgres along the above lines if it can be
done cleanly. But on the third hand, Alvaro's right that there isn't
any other obvious use for it.
We've also got the client_encoding (GUC initialization timing) issue
to fix in this area. I suggest that any major restructuring-for-beauty
wait till after the dust settles. I think what we have here (in the
revised patch) is ugly but not too ugly to live with.
regards, tom lane
Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
How about this?
I think the accounting for the AV launcher in shmem allocation is a bit
confused yet --- for instance, isn't MaxBackends already including the
launcher? I wonder if it would be cleaner to include the launcher in
the autovacuum_max_workers parameter, and increase the min/default
values of that by one.
Huh, yeah, sorry about that -- fixed here. I think the name of the
param, which includes "worker", precludes from raising the values.
Changes between v2 and v3:
diff -u src/backend/storage/lmgr/proc.c src/backend/storage/lmgr/proc.c
--- src/backend/storage/lmgr/proc.c 31 Aug 2009 13:36:56 -0000
+++ src/backend/storage/lmgr/proc.c 31 Aug 2009 16:14:08 -0000
@@ -103,7 +103,7 @@
/* AuxiliaryProcs */
size = add_size(size, mul_size(NUM_AUXILIARY_PROCS, sizeof(PGPROC)));
/* MyProcs, including autovacuum workers and launcher */
- size = add_size(size, mul_size(MaxBackends + 1, sizeof(PGPROC)));
+ size = add_size(size, mul_size(MaxBackends, sizeof(PGPROC)));
/* ProcStructLock */
size = add_size(size, sizeof(slock_t));@@ -192,6 +192,7 @@
ProcGlobal->freeProcs = &procs[i];
}
+ /* note: the "+1" here accounts for the autovac launcher */
procs = (PGPROC *) ShmemAlloc((autovacuum_max_workers + 1) * sizeof(PGPROC));
if (!procs)
ereport(FATAL,
diff -u src/backend/utils/misc/guc.c src/backend/utils/misc/guc.c
--- src/backend/utils/misc/guc.c 31 Aug 2009 03:07:47 -0000
+++ src/backend/utils/misc/guc.c 31 Aug 2009 16:12:56 -0000
@@ -7570,7 +7570,7 @@
static bool
assign_maxconnections(int newval, bool doit, GucSource source)
{
- if (newval + autovacuum_max_workers > INT_MAX / 4)
+ if (newval + autovacuum_max_workers + 1 > INT_MAX / 4)
return false;if (doit)
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachments:
avlauncher-proc-3.patchtext/x-diff; charset=us-asciiDownload
Index: src/backend/postmaster/autovacuum.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/postmaster/autovacuum.c,v
retrieving revision 1.103
diff -c -p -r1.103 autovacuum.c
*** src/backend/postmaster/autovacuum.c 27 Aug 2009 17:18:44 -0000 1.103
--- src/backend/postmaster/autovacuum.c 31 Aug 2009 15:49:14 -0000
*************** AutoVacLauncherMain(int argc, char *argv
*** 424,432 ****
#endif
/*
! * Set up signal handlers. Since this is an auxiliary process, it has
! * particular signal requirements -- no deadlock checker or sinval
! * catchup, for example.
*/
pqsignal(SIGHUP, avl_sighup_handler);
--- 424,432 ----
#endif
/*
! * Set up signal handlers. We operate on databases much like a regular
! * backend, so we use the same signal handling. See equivalent code in
! * tcop/postgres.c.
*/
pqsignal(SIGHUP, avl_sighup_handler);
*************** AutoVacLauncherMain(int argc, char *argv
*** 451,459 ****
* had to do some stuff with LWLocks).
*/
#ifndef EXEC_BACKEND
! InitAuxiliaryProcess();
#endif
/*
* Create a memory context that we will do all our work in. We do this so
* that we can reset the context during error recovery and thereby avoid
--- 451,461 ----
* had to do some stuff with LWLocks).
*/
#ifndef EXEC_BACKEND
! InitProcess();
#endif
+ InitPostgres(NULL, InvalidOid, NULL, NULL);
+
/*
* Create a memory context that we will do all our work in. We do this so
* that we can reset the context during error recovery and thereby avoid
*************** AutoVacLauncherMain(int argc, char *argv
*** 470,476 ****
/*
* If an exception is encountered, processing resumes here.
*
! * This code is heavily based on bgwriter.c, q.v.
*/
if (sigsetjmp(local_sigjmp_buf, 1) != 0)
{
--- 472,478 ----
/*
* If an exception is encountered, processing resumes here.
*
! * This code is a stripped down version of PostgresMain error recovery.
*/
if (sigsetjmp(local_sigjmp_buf, 1) != 0)
{
*************** AutoVacLauncherMain(int argc, char *argv
*** 483,496 ****
/* Report the error to the server log */
EmitErrorReport();
! /*
! * These operations are really just a minimal subset of
! * AbortTransaction(). We don't have very many resources to worry
! * about, but we do have LWLocks.
! */
! LWLockReleaseAll();
! AtEOXact_Files();
! AtEOXact_HashTables(false);
/*
* Now return to normal top-level context and clear ErrorContext for
--- 485,492 ----
/* Report the error to the server log */
EmitErrorReport();
! /* Abort the current transaction in order to recover */
! AbortCurrentTransaction();
/*
* Now return to normal top-level context and clear ErrorContext for
*************** autovac_balance_cost(void)
*** 1784,1829 ****
/*
* get_database_list
*
! * Return a list of all databases. Note we cannot use pg_database,
! * because we aren't connected; we use the flat database file.
*/
static List *
get_database_list(void)
{
- char *filename;
List *dblist = NIL;
! char thisname[NAMEDATALEN];
! FILE *db_file;
! Oid db_id;
! Oid db_tablespace;
! TransactionId db_frozenxid;
!
! filename = database_getflatfilename();
! db_file = AllocateFile(filename, "r");
! if (db_file == NULL)
! ereport(FATAL,
! (errcode_for_file_access(),
! errmsg("could not open file \"%s\": %m", filename)));
! while (read_pg_database_line(db_file, thisname, &db_id,
! &db_tablespace, &db_frozenxid))
{
! avw_dbase *avdb;
avdb = (avw_dbase *) palloc(sizeof(avw_dbase));
! avdb->adw_datid = db_id;
! avdb->adw_name = pstrdup(thisname);
! avdb->adw_frozenxid = db_frozenxid;
/* this gets set later: */
avdb->adw_entry = NULL;
dblist = lappend(dblist, avdb);
}
! FreeFile(db_file);
! pfree(filename);
return dblist;
}
--- 1780,1825 ----
/*
* get_database_list
+ * Return a list of all databases found in pg_database.
*
! * Note: this is the only function in which the autovacuum launcher uses a
! * transaction.
*/
static List *
get_database_list(void)
{
List *dblist = NIL;
! Relation rel;
! HeapScanDesc scan;
! HeapTuple tup;
! StartTransactionCommand();
!
! MemoryContextSwitchTo(AutovacMemCxt);
!
! rel = heap_open(DatabaseRelationId, AccessShareLock);
! scan = heap_beginscan(rel, SnapshotNow, 0, NULL);
!
! while (HeapTupleIsValid(tup = heap_getnext(scan, ForwardScanDirection)))
{
! Form_pg_database pgdatabase = (Form_pg_database) GETSTRUCT(tup);
! avw_dbase *avdb;
avdb = (avw_dbase *) palloc(sizeof(avw_dbase));
! avdb->adw_datid = HeapTupleGetOid(tup);
! avdb->adw_name = pstrdup(NameStr(pgdatabase->datname));
! avdb->adw_frozenxid = pgdatabase->datfrozenxid;
/* this gets set later: */
avdb->adw_entry = NULL;
dblist = lappend(dblist, avdb);
}
! heap_endscan(scan);
! heap_close(rel, AccessShareLock);
!
! CommitTransactionCommand();
return dblist;
}
Index: src/backend/postmaster/postmaster.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.593
diff -c -p -r1.593 postmaster.c
*** src/backend/postmaster/postmaster.c 29 Aug 2009 19:26:51 -0000 1.593
--- src/backend/postmaster/postmaster.c 31 Aug 2009 13:37:29 -0000
*************** SubPostmasterMain(int argc, char *argv[]
*** 3865,3871 ****
InitShmemAccess(UsedShmemSegAddr);
/* Need a PGPROC to run CreateSharedMemoryAndSemaphores */
! InitAuxiliaryProcess();
/* Attach process to shared data structures */
CreateSharedMemoryAndSemaphores(false, 0);
--- 3865,3871 ----
InitShmemAccess(UsedShmemSegAddr);
/* Need a PGPROC to run CreateSharedMemoryAndSemaphores */
! InitProcess();
/* Attach process to shared data structures */
CreateSharedMemoryAndSemaphores(false, 0);
Index: src/backend/storage/lmgr/proc.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/storage/lmgr/proc.c,v
retrieving revision 1.208
diff -c -p -r1.208 proc.c
*** src/backend/storage/lmgr/proc.c 12 Aug 2009 20:53:30 -0000 1.208
--- src/backend/storage/lmgr/proc.c 31 Aug 2009 16:14:08 -0000
*************** ProcGlobalShmemSize(void)
*** 102,108 ****
size = add_size(size, sizeof(PROC_HDR));
/* AuxiliaryProcs */
size = add_size(size, mul_size(NUM_AUXILIARY_PROCS, sizeof(PGPROC)));
! /* MyProcs, including autovacuum */
size = add_size(size, mul_size(MaxBackends, sizeof(PGPROC)));
/* ProcStructLock */
size = add_size(size, sizeof(slock_t));
--- 102,108 ----
size = add_size(size, sizeof(PROC_HDR));
/* AuxiliaryProcs */
size = add_size(size, mul_size(NUM_AUXILIARY_PROCS, sizeof(PGPROC)));
! /* MyProcs, including autovacuum workers and launcher */
size = add_size(size, mul_size(MaxBackends, sizeof(PGPROC)));
/* ProcStructLock */
size = add_size(size, sizeof(slock_t));
*************** InitProcGlobal(void)
*** 192,204 ****
ProcGlobal->freeProcs = &procs[i];
}
! procs = (PGPROC *) ShmemAlloc((autovacuum_max_workers) * sizeof(PGPROC));
if (!procs)
ereport(FATAL,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of shared memory")));
! MemSet(procs, 0, autovacuum_max_workers * sizeof(PGPROC));
! for (i = 0; i < autovacuum_max_workers; i++)
{
PGSemaphoreCreate(&(procs[i].sem));
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->autovacFreeProcs;
--- 192,205 ----
ProcGlobal->freeProcs = &procs[i];
}
! /* note: the "+1" here accounts for the autovac launcher */
! procs = (PGPROC *) ShmemAlloc((autovacuum_max_workers + 1) * sizeof(PGPROC));
if (!procs)
ereport(FATAL,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of shared memory")));
! MemSet(procs, 0, (autovacuum_max_workers + 1) * sizeof(PGPROC));
! for (i = 0; i < autovacuum_max_workers + 1; i++)
{
PGSemaphoreCreate(&(procs[i].sem));
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->autovacFreeProcs;
*************** InitProcess(void)
*** 248,261 ****
set_spins_per_delay(procglobal->spins_per_delay);
! if (IsAutoVacuumWorkerProcess())
MyProc = procglobal->autovacFreeProcs;
else
MyProc = procglobal->freeProcs;
if (MyProc != NULL)
{
! if (IsAutoVacuumWorkerProcess())
procglobal->autovacFreeProcs = (PGPROC *) MyProc->links.next;
else
procglobal->freeProcs = (PGPROC *) MyProc->links.next;
--- 249,262 ----
set_spins_per_delay(procglobal->spins_per_delay);
! if (IsAnyAutoVacuumProcess())
MyProc = procglobal->autovacFreeProcs;
else
MyProc = procglobal->freeProcs;
if (MyProc != NULL)
{
! if (IsAnyAutoVacuumProcess())
procglobal->autovacFreeProcs = (PGPROC *) MyProc->links.next;
else
procglobal->freeProcs = (PGPROC *) MyProc->links.next;
*************** InitProcess(void)
*** 280,286 ****
* child; this is so that the postmaster can detect it if we exit without
* cleaning up.
*/
! if (IsUnderPostmaster)
MarkPostmasterChildActive();
/*
--- 281,287 ----
* child; this is so that the postmaster can detect it if we exit without
* cleaning up.
*/
! if (IsUnderPostmaster && !IsAutoVacuumLauncherProcess())
MarkPostmasterChildActive();
/*
*************** InitProcess(void)
*** 299,304 ****
--- 300,306 ----
MyProc->roleId = InvalidOid;
MyProc->inCommit = false;
MyProc->vacuumFlags = 0;
+ /* NB -- autovac launcher does not set this flag to avoid getting killed */
if (IsAutoVacuumWorkerProcess())
MyProc->vacuumFlags |= PROC_IS_AUTOVACUUM;
MyProc->lwWaiting = false;
*************** InitAuxiliaryProcess(void)
*** 429,435 ****
MyProc->databaseId = InvalidOid;
MyProc->roleId = InvalidOid;
MyProc->inCommit = false;
- /* we don't set the "is autovacuum" flag in the launcher */
MyProc->vacuumFlags = 0;
MyProc->lwWaiting = false;
MyProc->lwExclusive = false;
--- 431,436 ----
*************** ProcKill(int code, Datum arg)
*** 596,602 ****
SpinLockAcquire(ProcStructLock);
/* Return PGPROC structure (and semaphore) to freelist */
! if (IsAutoVacuumWorkerProcess())
{
MyProc->links.next = (SHM_QUEUE *) procglobal->autovacFreeProcs;
procglobal->autovacFreeProcs = MyProc;
--- 597,603 ----
SpinLockAcquire(ProcStructLock);
/* Return PGPROC structure (and semaphore) to freelist */
! if (IsAnyAutoVacuumProcess())
{
MyProc->links.next = (SHM_QUEUE *) procglobal->autovacFreeProcs;
procglobal->autovacFreeProcs = MyProc;
*************** ProcKill(int code, Datum arg)
*** 619,625 ****
* This process is no longer present in shared memory in any meaningful
* way, so tell the postmaster we've cleaned up acceptably well.
*/
! if (IsUnderPostmaster)
MarkPostmasterChildInactive();
/* wake autovac launcher if needed -- see comments in FreeWorkerInfo */
--- 620,626 ----
* This process is no longer present in shared memory in any meaningful
* way, so tell the postmaster we've cleaned up acceptably well.
*/
! if (IsUnderPostmaster && !IsAutoVacuumLauncherProcess())
MarkPostmasterChildInactive();
/* wake autovac launcher if needed -- see comments in FreeWorkerInfo */
Index: src/backend/utils/init/postinit.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/init/postinit.c,v
retrieving revision 1.195
diff -c -p -r1.195 postinit.c
*** src/backend/utils/init/postinit.c 29 Aug 2009 19:26:51 -0000 1.195
--- src/backend/utils/init/postinit.c 31 Aug 2009 15:48:10 -0000
*************** InitPostgres(const char *in_dbname, Oid
*** 552,557 ****
--- 552,560 ----
* Start a new transaction here before first access to db, and get a
* snapshot. We don't have a use for the snapshot itself, but we're
* interested in the secondary effect that it sets RecentGlobalXmin.
+ * This is critical for anything that reads heap pages, because HOT
+ * may decide to prune them even if the process doesn't attempt to
+ * modify it.
*/
if (!bootstrap)
{
*************** InitPostgres(const char *in_dbname, Oid
*** 565,570 ****
--- 568,577 ----
*/
RelationCacheInitializePhase2();
+ /* The autovacuum launcher is done here */
+ if (IsAutoVacuumLauncherProcess())
+ goto done;
+
/*
* Set up the global variables holding database id and default tablespace.
* But note we won't actually try to touch the database just yet.
*************** InitPostgres(const char *in_dbname, Oid
*** 774,779 ****
--- 781,787 ----
if (!bootstrap)
pgstat_bestart();
+ done:
/* close the transaction we started above */
if (!bootstrap)
CommitTransactionCommand();
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/misc/guc.c,v
retrieving revision 1.513
diff -c -p -r1.513 guc.c
*** src/backend/utils/misc/guc.c 31 Aug 2009 02:23:22 -0000 1.513
--- src/backend/utils/misc/guc.c 31 Aug 2009 16:12:56 -0000
*************** static struct config_int ConfigureNamesI
*** 1335,1341 ****
* Note: MaxBackends is limited to INT_MAX/4 because some places compute
* 4*MaxBackends without any overflow check. This check is made in
* assign_maxconnections, since MaxBackends is computed as MaxConnections
! * plus autovacuum_max_workers.
*
* Likewise we have to limit NBuffers to INT_MAX/2.
*/
--- 1335,1341 ----
* Note: MaxBackends is limited to INT_MAX/4 because some places compute
* 4*MaxBackends without any overflow check. This check is made in
* assign_maxconnections, since MaxBackends is computed as MaxConnections
! * plus autovacuum_max_workers plus one (for the autovacuum launcher).
*
* Likewise we have to limit NBuffers to INT_MAX/2.
*/
*************** show_tcp_keepalives_count(void)
*** 7570,7580 ****
static bool
assign_maxconnections(int newval, bool doit, GucSource source)
{
! if (newval + autovacuum_max_workers > INT_MAX / 4)
return false;
if (doit)
! MaxBackends = newval + autovacuum_max_workers;
return true;
}
--- 7570,7580 ----
static bool
assign_maxconnections(int newval, bool doit, GucSource source)
{
! if (newval + autovacuum_max_workers + 1 > INT_MAX / 4)
return false;
if (doit)
! MaxBackends = newval + autovacuum_max_workers + 1;
return true;
}
*************** assign_autovacuum_max_workers(int newval
*** 7586,7592 ****
return false;
if (doit)
! MaxBackends = newval + MaxConnections;
return true;
}
--- 7586,7592 ----
return false;
if (doit)
! MaxBackends = newval + MaxConnections + 1;
return true;
}
Index: src/include/postmaster/autovacuum.h
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/include/postmaster/autovacuum.h,v
retrieving revision 1.15
diff -c -p -r1.15 autovacuum.h
*** src/include/postmaster/autovacuum.h 1 Jan 2009 17:24:01 -0000 1.15
--- src/include/postmaster/autovacuum.h 31 Aug 2009 03:29:17 -0000
*************** extern int Log_autovacuum_min_duration;
*** 37,42 ****
--- 37,44 ----
extern bool AutoVacuumingActive(void);
extern bool IsAutoVacuumLauncherProcess(void);
extern bool IsAutoVacuumWorkerProcess(void);
+ #define IsAnyAutoVacuumProcess() (IsAutoVacuumLauncherProcess() || \
+ IsAutoVacuumWorkerProcess())
/* Functions to start autovacuum process, called from postmaster */
extern void autovac_init(void);
Index: src/include/storage/proc.h
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/include/storage/proc.h,v
retrieving revision 1.113
diff -c -p -r1.113 proc.h
*** src/include/storage/proc.h 12 Aug 2009 20:53:30 -0000 1.113
--- src/include/storage/proc.h 31 Aug 2009 03:08:42 -0000
*************** typedef struct PROC_HDR
*** 141,152 ****
* We set aside some extra PGPROC structures for auxiliary processes,
* ie things that aren't full-fledged backends but need shmem access.
*
! * Background writer, WAL writer, and autovacuum launcher run during
! * normal operation. Startup process also consumes one slot, but WAL
! * writer and autovacuum launcher are launched only after it has
* exited.
*/
! #define NUM_AUXILIARY_PROCS 3
/* configurable options */
--- 141,151 ----
* We set aside some extra PGPROC structures for auxiliary processes,
* ie things that aren't full-fledged backends but need shmem access.
*
! * Background writer and WAL writer run during normal operation. Startup
! * process also consumes one slot, but WAL writer is launched only after it has
* exited.
*/
! #define NUM_AUXILIARY_PROCS 2
/* configurable options */
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tom Lane wrote:
I wonder if it would be cleaner to include the launcher in
the autovacuum_max_workers parameter, and increase the min/default
values of that by one.
Huh, yeah, sorry about that -- fixed here. I think the name of the
param, which includes "worker", precludes from raising the values.
Well, I'm not sure the average user knows or cares about the difference
between the launcher and the workers. The thing that was in the back of
my mind was that we would now have the option to have the launcher show
up in pg_stat_activity. If we were to do that then the case for
counting it in the user-visible number-of-processes parameter would get
a lot stronger (enough to justify renaming the parameter, if you insist
that the launcher isn't a worker). I don't however have any strong
opinion on whether we *should* include it in pg_stat_activity ---
comments?
In the meantime, this looks reasonably sane in a fast read-through,
but I saw a few comments that could use improvement, and I have not
tried to actually review it (like look for missed places to change).
Do you mind if I work it over for an hour or two?
regards, tom lane
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tom Lane wrote:
While I was looking at this I wondered whether
RelationCacheInitializePhase2 really needs to be inside the startup
transaction at all. I think it could probably be moved up before
that. However, if the AV launcher has to do GetTransactionSnapshot
then it's not clear that improves matters anyway.
Well, the difference is that the initial transaction would be a few
microsec shorter ... not sure if that matters.
Actually, there is a better way to do this: if we move up the
RelationCacheInitializePhase2 call, then we can have the AV launcher
case just fall out *before* the transaction start. It can do
GetTransactionSnapshot inside its own transaction that reads
pg_database. This is a better solution because it'll have a more
up-to-date version of RecentGlobalXmin while scanning pg_database.
(Indeed, I think this might be *necessary* over the very long haul.)
I think I've got the signal handling cleaned up, but need to test.
Is there any really good reason why autovacuum has its own avl_quickdie
instead of using quickdie() for SIGQUIT?
regards, tom lane
Tom Lane wrote:
Actually, there is a better way to do this: if we move up the
RelationCacheInitializePhase2 call, then we can have the AV launcher
case just fall out *before* the transaction start. It can do
GetTransactionSnapshot inside its own transaction that reads
pg_database. This is a better solution because it'll have a more
up-to-date version of RecentGlobalXmin while scanning pg_database.
(Indeed, I think this might be *necessary* over the very long haul.)
Hmm, good idea.
I think I've got the signal handling cleaned up, but need to test.
Is there any really good reason why autovacuum has its own avl_quickdie
instead of using quickdie() for SIGQUIT?
No, probably I just copied it because the others were static. Not sure
about quickdie() itself.
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tom Lane wrote:
I wonder if it would be cleaner to include the launcher in
the autovacuum_max_workers parameter, and increase the min/default
values of that by one.Huh, yeah, sorry about that -- fixed here. I think the name of the
param, which includes "worker", precludes from raising the values.Well, I'm not sure the average user knows or cares about the difference
between the launcher and the workers. The thing that was in the back of
my mind was that we would now have the option to have the launcher show
up in pg_stat_activity. If we were to do that then the case for
counting it in the user-visible number-of-processes parameter would get
a lot stronger (enough to justify renaming the parameter, if you insist
that the launcher isn't a worker). I don't however have any strong
opinion on whether we *should* include it in pg_stat_activity ---
comments?
The user may not care about the difference, but there's a point in
having the limit be the simpler concept of "this is the maximum amount
of processes running vacuum at any time". The launcher is very
uninteresting to users.
In the meantime, this looks reasonably sane in a fast read-through,
but I saw a few comments that could use improvement, and I have not
tried to actually review it (like look for missed places to change).
Do you mind if I work it over for an hour or two?
Please go ahead.
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
The only thing I'm aware is missing from this patch is fixing up
avlauncher's signal handling, and adding a bit more commentary; also I
haven't tested it under EXEC_BACKEND yet.
I did the signal handling work and fixed a couple of small oversights,
and applied it. I'm not sure what other commentary you had in mind,
but feel free to add.
regards, tom lane
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tom Lane wrote:
Well, I'm not sure the average user knows or cares about the difference
between the launcher and the workers. The thing that was in the back of
my mind was that we would now have the option to have the launcher show
up in pg_stat_activity. If we were to do that then the case for
counting it in the user-visible number-of-processes parameter would get
a lot stronger (enough to justify renaming the parameter, if you insist
that the launcher isn't a worker). I don't however have any strong
opinion on whether we *should* include it in pg_stat_activity ---
comments?
The user may not care about the difference, but there's a point in
having the limit be the simpler concept of "this is the maximum amount
of processes running vacuum at any time". The launcher is very
uninteresting to users.
I committed things that way, but I'm still not convinced that we
shouldn't expose the launcher in pg_stat_activity. The thing that
is bothering me is that it is now able to take locks and potentially
could block some other process or even participate in a deadlock.
Do we really want to have entries in pg_locks that don't match any
entry in pg_stat_activity?
regards, tom lane
Hi,
Tom Lane <tgl@sss.pgh.pa.us> writes:
The user may not care about the difference, but there's a point in
having the limit be the simpler concept of "this is the maximum amount
of processes running vacuum at any time". The launcher is very
uninteresting to users.
Adding to this, the launcher will not consume maintenance_work_mem
whereas each worker is able to allocate that much, IIUC.
I committed things that way, but I'm still not convinced that we
shouldn't expose the launcher in pg_stat_activity. The thing that
is bothering me is that it is now able to take locks and potentially
could block some other process or even participate in a deadlock.
Do we really want to have entries in pg_locks that don't match any
entry in pg_stat_activity?
Having the launcher locks show as such gets my vote too, but then I'm
following on your opinion that a launcher ain't a worker and that users
need to know about it.
Let's keep the autovacuum_max_workers GUC naming, not counting the
"there can be only one" launcher so that we're able to size
maintenance_work_mem.
Regards,
--
dim
On Mon, Aug 31, 2009 at 21:49, Tom Lane<tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tom Lane wrote:
Well, I'm not sure the average user knows or cares about the difference
between the launcher and the workers. The thing that was in the back of
my mind was that we would now have the option to have the launcher show
up in pg_stat_activity. If we were to do that then the case for
counting it in the user-visible number-of-processes parameter would get
a lot stronger (enough to justify renaming the parameter, if you insist
that the launcher isn't a worker). I don't however have any strong
opinion on whether we *should* include it in pg_stat_activity ---
comments?The user may not care about the difference, but there's a point in
having the limit be the simpler concept of "this is the maximum amount
of processes running vacuum at any time". The launcher is very
uninteresting to users.I committed things that way, but I'm still not convinced that we
shouldn't expose the launcher in pg_stat_activity. The thing that
is bothering me is that it is now able to take locks and potentially
could block some other process or even participate in a deadlock.
Do we really want to have entries in pg_locks that don't match any
entry in pg_stat_activity?
At first I'd have voted that we don't have it in pg_stat_activity, but
the argument about pg_locks really is the killer on. IMHO, it *has* to
go there then.
I would also suggest that we add a separate column to pg_stat_activity
indicating what type of process it is, so that monitoring tools can
figure that out without having to resort to string matching on the
current query field. This field would indicate if it's a backend,
autovac worker, autovac launcher. And perhaps we should even add the
other utility processes to pg_stat_activity, for completeness?
Another thought along a similar line is some way to detect the idle
and idle-in-transaction states without doing string matching as well.
Perhaps this could be overloaded on said extra field, or should it
have a separate one?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/