Re: [PATCHES] vacuum as flags in PGPROC
Tom Lane wrote: Alvaro Herrera [EMAIL PROTECTED] writes: Also, I forgot to mention it on the first email, but this patch adds errcontext() lines when an autovacuum/analyze is being aborted. It works fine, but I'm not seeing code anywhere else that does something like this. This is a little bit scary because you might be invoking system catalog reads after the transaction has already failed. What would it take to save the names involved before starting the TRY block? I'm not worried about the errcontext() call per se, only about the syscache fetches. Hmm, now it's misbehaving strangely. I just got this log output. The interesting thing is the previous-to-last line (CONTEXT). 29789 DEBUG: autovacuum: processing database alvherre 29789 DEBUG: autovac_balance_cost(pid=29789 db=16384, rel=16413, cost_limit=200, cost_delay=20) 29789 DEBUG: vacuuming public.a 29790 DEBUG: autovacuum: processing database alvherre 29742 DEBUG: server process (PID 29790) exited with exit code 0 29791 DEBUG: autovacuum: processing database alvherre 29742 DEBUG: server process (PID 29791) exited with exit code 0 29792 DEBUG: autovacuum: processing database alvherre 29742 DEBUG: server process (PID 29792) exited with exit code 0 29793 DEBUG: autovacuum: processing database alvherre 29742 DEBUG: server process (PID 29793) exited with exit code 0 29789 DEBUG: a: removed 100 row versions in 4425 pages 29789 DEBUG: a: found 100 removable, 100 nonremovable row versions in 8850 pages 29789 DETAIL: 0 dead row versions cannot be removed yet. There were 0 unused item pointers. 4426 pages contain useful free space. 0 pages are entirely empty. CPU 0.00s/0.25u sec elapsed 23.91 sec. 29794 DEBUG: autovacuum: processing database alvherre 29742 DEBUG: server process (PID 29794) exited with exit code 0 29800 DEBUG: autovacuum: processing database alvherre 29742 DEBUG: server process (PID 29800) exited with exit code 0 29744 DEBUG: recycled transaction log file 00010064 29744 DEBUG: recycled transaction log file 00010065 29744 DEBUG: recycled transaction log file 00010063 29806 DEBUG: autovacuum: processing database alvherre 29742 DEBUG: server process (PID 29806) exited with exit code 0 29744 LOG: checkpoints are occurring too frequently (9 seconds apart) 29744 HINT: Consider increasing the configuration parameter checkpoint_segments. 29753 DEBUG: drop auto-cascades to type pg_temp_16413 29753 DEBUG: drop auto-cascades to type pg_temp_16413[] 29789 DEBUG: analyzing public.a 29753 DEBUG: sending cancel to blocking autovacuum pid = 29789 29789 ERROR: canceling statement due to user request 29789 CONTEXT: automatic vacuum of table .. 29742 DEBUG: server process (PID 29789) exited with exit code 0 Note how the cancel log line has a context of .. instead of the qualified table name. What I did was create a big table using Simon's suggested create table a as select generate_series(1,100)::integer as col1; then launch update a set col1 = col1 + 1; When this is finished, autovac starts to vacuum the table; and I run alter table a alter column col1 type bigint; This does not cancel the vacuum (because I'm only cancelling in analyze). When the vacuum is finished, alter table proceeds. When alter table finishes, the ANALYZE corresponding to the vacuum starts; then I start another ALTER TABLE and this one is cancelled. That's the errcontext message that's messed up. I don't really know what's going on ... I suppose the sigsetjmp() in PG_TRY is messing things up, but I'm not sure how to fix it. Any thoughts? -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] vacuum as flags in PGPROC
Alvaro Herrera wrote: Hmm, now it's misbehaving strangely. Nevermind, I think I see what's happening. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] vacuum as flags in PGPROC
Tom Lane wrote: Alvaro Herrera [EMAIL PROTECTED] writes: In the spirit of incremental improvement, here is a patch that turns the couple of bools in PGPROC into a bitmask, and associated fallout. Maybe declare the field as uint8 instead of char? Otherwise, +1. I'm wondering if it's safe to do something like MyProc-vacuumFlags |= PROC_FOR_XID_WRAPAROUND without holding the ProcArrayLock. It seems in practice a type smaller than int (which uint8 always is) is always stored atomically so this shouldn't be a problem. -- Alvaro Herrera http://www.PlanetPostgreSQL.org/ La rebeldÃa es la virtud original del hombre (Arthur Schopenhauer) ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] vacuum as flags in PGPROC
Tom Lane wrote:
Alvaro Herrera [EMAIL PROTECTED] writes:
I'm wondering if it's safe to do something like
MyProc-vacuumFlags |= PROC_FOR_XID_WRAPAROUND
without holding the ProcArrayLock.
This seems a bit itchy.
One thing I'd be worried about is processors that implement that by
fetching the whole word containing the field, setting the bit, and
storing back the whole word. (I believe some RISC processors are likely
to do it like that.) This would mean that it'd work OK only as long as
no other process was concurrently changing any field that happened to be
in the same word, which is the kind of requirement that seems horribly
fragile.
I did it that way (i.e. added locking) and then realized that it
shouldn't really be a problem, because the only one who can be setting
vacuum flags is the process itself. Other processes can only read the
flags.
Maybe the locking is not a problem anyway, but there are two additional
flag sets in analyze and two more in the autovacuum code when it detects
that it's vacuuming a table for Xid wraparound. (The idea is to use
these flags in the deadlock patch Simon posted, so that signals are
automatically sent or not according to the current activity of
autovacuum. This way the signal handler can be kept stupid.)
Also, I forgot to mention it on the first email, but this patch adds
errcontext() lines when an autovacuum/analyze is being aborted. It
works fine, but I'm not seeing code anywhere else that does something
like this.
--
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Index: src/backend/access/transam/twophase.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/transam/twophase.c,v
retrieving revision 1.36
diff -c -p -r1.36 twophase.c
*** src/backend/access/transam/twophase.c 21 Sep 2007 16:32:19 - 1.36
--- src/backend/access/transam/twophase.c 24 Oct 2007 14:59:47 -
*** MarkAsPreparing(TransactionId xid, const
*** 283,290
gxact-proc.databaseId = databaseid;
gxact-proc.roleId = owner;
gxact-proc.inCommit = false;
! gxact-proc.inVacuum = false;
! gxact-proc.isAutovacuum = false;
gxact-proc.lwWaiting = false;
gxact-proc.lwExclusive = false;
gxact-proc.lwWaitLink = NULL;
--- 283,289
gxact-proc.databaseId = databaseid;
gxact-proc.roleId = owner;
gxact-proc.inCommit = false;
! gxact-proc.vacuumFlags = 0;
gxact-proc.lwWaiting = false;
gxact-proc.lwExclusive = false;
gxact-proc.lwWaitLink = NULL;
Index: src/backend/commands/analyze.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/commands/analyze.c,v
retrieving revision 1.109
diff -c -p -r1.109 analyze.c
*** src/backend/commands/analyze.c 24 Sep 2007 03:12:23 - 1.109
--- src/backend/commands/analyze.c 24 Oct 2007 14:59:47 -
***
*** 31,36
--- 31,37
#include parser/parse_relation.h
#include pgstat.h
#include postmaster/autovacuum.h
+ #include storage/proc.h
#include utils/acl.h
#include utils/datum.h
#include utils/lsyscache.h
*** analyze_rel(Oid relid, VacuumStmt *vacst
*** 201,206
--- 202,212
return;
}
+ /* let others know what I'm doing */
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ MyProc-vacuumFlags |= PROC_IN_ANALYZE;
+ LWLockRelease(ProcArrayLock);
+
/* measure elapsed time iff autovacuum logging requires it */
if (IsAutoVacuumWorkerProcess() Log_autovacuum_min_duration = 0)
{
*** analyze_rel(Oid relid, VacuumStmt *vacst
*** 484,489
--- 490,503
RelationGetRelationName(onerel),
pg_rusage_show(ru0;
}
+
+ /*
+ * Reset my PGPROC flag. Note: we need this here, and not in vacuum_rel,
+ * because the vacuum flag is cleared by the end-of-xact code.
+ */
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ MyProc-vacuumFlags = ~PROC_IN_ANALYZE;
+ LWLockRelease(ProcArrayLock);
}
/*
Index: src/backend/commands/vacuum.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/commands/vacuum.c,v
retrieving revision 1.359
diff -c -p -r1.359 vacuum.c
*** src/backend/commands/vacuum.c 20 Sep 2007 17:56:31 - 1.359
--- src/backend/commands/vacuum.c 24 Oct 2007 14:59:47 -
*** vacuum_set_xid_limits(int freeze_min_age
*** 660,668
* fixed-size never-null columns, but these are.
*
* Another reason for doing it this way is that when we are in a lazy
! * VACUUM and have inVacuum set, we mustn't do any updates --- somebody
! * vacuuming pg_class might think they could delete a tuple marked with
! * xmin = our xid.
*
* This routine is shared by full VACUUM, lazy VACUUM, and stand-alone
* ANALYZE.
--- 660,668
* fixed-size never-null
Re: [PATCHES] vacuum as flags in PGPROC
Heikki Linnakangas wrote: Alvaro Herrera wrote: I did it that way (i.e. added locking) and then realized that it shouldn't really be a problem, because the only one who can be setting vacuum flags is the process itself. Other processes can only read the flags. It would still be a problem if there was any other fields that were updated by other processes, adjacent to the vacuum flags. I don't think that's the case, however. Yeah, that's not the case currently. Tom is right in that it's fragile if we ever change the definition so that there is such a flag. Maybe this is solved by adding a comment however. -- Alvaro Herrera http://www.amazon.com/gp/registry/5ZYLFMCVHXC Jude: I wish humans laid eggs Ringlord: Why would you want humans to lay eggs? Jude: So I can eat them ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] vacuum as flags in PGPROC
Heikki Linnakangas [EMAIL PROTECTED] writes: Alvaro Herrera wrote: I did it that way (i.e. added locking) and then realized that it shouldn't really be a problem, because the only one who can be setting vacuum flags is the process itself. Other processes can only read the flags. It would still be a problem if there was any other fields that were updated by other processes, adjacent to the vacuum flags. I don't think that's the case, however. Well, that may not be the case today, but it still seems like an assumption that will come back to bite us someday. And can you imagine trying to debug a misbehavior like that? It's really not worth the risk, given how seldom these flags will be changed. regards, tom lane ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] vacuum as flags in PGPROC
Tom Lane wrote: Heikki Linnakangas [EMAIL PROTECTED] writes: Alvaro Herrera wrote: I did it that way (i.e. added locking) and then realized that it shouldn't really be a problem, because the only one who can be setting vacuum flags is the process itself. Other processes can only read the flags. It would still be a problem if there was any other fields that were updated by other processes, adjacent to the vacuum flags. I don't think that's the case, however. Well, that may not be the case today, but it still seems like an assumption that will come back to bite us someday. And can you imagine trying to debug a misbehavior like that? It's really not worth the risk, given how seldom these flags will be changed. Oh, I totally agree. I wasn't trying to argue to the contrary. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] vacuum as flags in PGPROC
Alvaro Herrera [EMAIL PROTECTED] writes: Also, I forgot to mention it on the first email, but this patch adds errcontext() lines when an autovacuum/analyze is being aborted. It works fine, but I'm not seeing code anywhere else that does something like this. This is a little bit scary because you might be invoking system catalog reads after the transaction has already failed. What would it take to save the names involved before starting the TRY block? I'm not worried about the errcontext() call per se, only about the syscache fetches. regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] vacuum as flags in PGPROC
Alvaro Herrera [EMAIL PROTECTED] writes: In the spirit of incremental improvement, here is a patch that turns the couple of bools in PGPROC into a bitmask, and associated fallout. Maybe declare the field as uint8 instead of char? Otherwise, +1. This patch also contains a change to make a cancelled autovacuum continue with the schedule (indeed to continue with the schedule on any error), rather than aborting completely. I think we'd considered that a bug to be fixed. Are you intending this for 8.3 or 8.4? I don't have a problem with it for 8.3, but someone else might ... regards, tom lane ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
