Thread: Autovacuum and XID wraparound
Folks, Per Neil Conway, here's some doc patches re: the autovacuum daemon's behavior. Should this be back-patched to 8.2x? Cheers, D -- David Fetter <david@fetter.org> http://fetter.org/ phone: +1 415 235 3778 AIM: dfetter666 Skype: davidfetter Remember to vote! Consider donating to PostgreSQL: http://www.postgresql.org/about/donate
David Fetter <david@fetter.org> writes:
> Per Neil Conway, here's some doc patches re: the autovacuum daemon's
> behavior. Should this be back-patched to 8.2x?
This fact is already documented in at least three places; do we really
need two more? The proposed addition to postgresql.conf seems
particularly over-the-top, since there is no entry in that file that
even pretends to offer a complete description of the associated
behavior.
regards, tom lane
On Sun, May 13, 2007 at 10:06:40PM -0400, Tom Lane wrote: > David Fetter <david@fetter.org> writes: > > Per Neil Conway, here's some doc patches re: the autovacuum > > daemon's behavior. Should this be back-patched to 8.2x? > > This fact is already documented in at least three places; do we > really need two more? Yes. > The proposed addition to postgresql.conf seems particularly > over-the-top, since there is no entry in that file that even > pretends to offer a complete description of the associated behavior. I think that a boolean that doesn't do what you expect booleans to do, i.e. turn the thing all the way off, is worth a mention. Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ phone: +1 415 235 3778 AIM: dfetter666 Skype: davidfetter Remember to vote! Consider donating to PostgreSQL: http://www.postgresql.org/about/donate
On Sun, 2007-13-05 at 22:06 -0400, Tom Lane wrote: > This fact is already documented in at least three places; do we really > need two more? I think we need to at least modify the documentation for the autovacuum GUC parameter, which currently states only that it "controls whether the server should run the autovacuum launcher daemon" -- this is not strictly true, and in any case, it isn't the whole story. > The proposed addition to postgresql.conf seems particularly > over-the-top I agree that this information doesn't really belong in postgresql.conf. -Neil
David Fetter wrote: > On Sun, May 13, 2007 at 10:06:40PM -0400, Tom Lane wrote: > > David Fetter <david@fetter.org> writes: > > > Per Neil Conway, here's some doc patches re: the autovacuum > > > daemon's behavior. Should this be back-patched to 8.2x? > > > > This fact is already documented in at least three places; do we > > really need two more? > > Yes. > > > The proposed addition to postgresql.conf seems particularly > > over-the-top, since there is no entry in that file that even > > pretends to offer a complete description of the associated behavior. > > I think that a boolean that doesn't do what you expect booleans to do, > i.e. turn the thing all the way off, is worth a mention. I agree with Tom. I don't think the current behavior is a major issue for users for it to be mentioned more than it already is, though if you want to move one of those, we can do that. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
neilc@samurai.com (Neil Conway) writes: > On Sun, 2007-13-05 at 22:06 -0400, Tom Lane wrote: >> This fact is already documented in at least three places; do we really >> need two more? > > I think we need to at least modify the documentation for the autovacuum > GUC parameter, which currently states only that it "controls whether the > server should run the autovacuum launcher daemon" -- this is not > strictly true, and in any case, it isn't the whole story. > >> The proposed addition to postgresql.conf seems particularly >> over-the-top > > I agree that this information doesn't really belong in postgresql.conf. Question... (note: this does not strictly fit into the purview of the .patches list) Would the following 'maintenance' regimen be truly safe against XID wraparound: - Most tables are being vacuumed regularly, so that pg_class.relfrozenxid is kept "safe." - There are some tables that periodically get TRUNCATEd so that, in principle, they never need to be vacuumed. Is it actually true that we'd never need to vacuum those tables (assuming 8.2+)? I suppose it would be rather cheap to VACUUM immediately after the TRUNCATE... The application is one where we might use partitioning, rolling from table to table every so often, with the expectation that we'll TRUNCATE the eldest data often enough that we shouldn't need to VACUUM any of the partitions. -- let name="cbbrowne" and tld="linuxdatabases.info" in name ^ "@" ^ tld;; http://linuxdatabases.info/info/linuxxian.html Why are men like blenders? You need one, but you're not quite sure why.
Chris Browne wrote: > Would the following 'maintenance' regimen be truly safe against XID > wraparound: > > - Most tables are being vacuumed regularly, so that > pg_class.relfrozenxid is kept "safe." > > - There are some tables that periodically get TRUNCATEd so that, in > principle, they never need to be vacuumed. > > Is it actually true that we'd never need to vacuum those tables > (assuming 8.2+)? I suppose it would be rather cheap to VACUUM > immediately after the TRUNCATE... You'd need to vacuum after the truncate. It would be pretty cheap, the tables being empty. I suppose it would be pretty trivial to set the relfrozenxid to RecentXmin or something during TRUNCATE. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
> I suppose it would be pretty trivial to set the relfrozenxid to
> RecentXmin or something during TRUNCATE.
I had the idea we were doing that already --- at least I'm pretty sure I
remember it being discussed. But I see it's not being done in HEAD.
regards, tom lane
On Mon, 2007-14-05 at 16:22 -0400, Bruce Momjian wrote: > I agree with Tom. I don't think the current behavior is a major issue > for users for it to be mentioned more than it already is Are you really suggesting that we shouldn't modify config.sgml to note that "autovacuum = off" does not actually imply that "the autovacuum daemon is disabled"? ISTM that plainly violates the principle of least surprise -- it is almost the definition of what an entry in config.sgml *should* include. > though if you want to move one of those, we can do that. So the change would be okay if we also removed one of the other mentions in an unrelated section of the manual? I don't see the logic. -Neil
Neil Conway wrote: > On Mon, 2007-14-05 at 16:22 -0400, Bruce Momjian wrote: > > I agree with Tom. I don't think the current behavior is a major issue > > for users for it to be mentioned more than it already is > > Are you really suggesting that we shouldn't modify config.sgml to note > that "autovacuum = off" does not actually imply that "the autovacuum > daemon is disabled"? ISTM that plainly violates the principle of least > surprise -- it is almost the definition of what an entry in config.sgml > *should* include. I agree, the note should be added there (but it should be a short one and refer the reader someplace else for more complete details). Was there a doc patch proposed already? I seem to have missed it. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
On Tue, 2007-15-05 at 09:07 -0400, Alvaro Herrera wrote: > I agree, the note should be added there (but it should be a short one > and refer the reader someplace else for more complete details). I've applied the attached patch to HEAD and REL8_2_STABLE. -Neil
Attachment
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > I suppose it would be pretty trivial to set the relfrozenxid to > > RecentXmin or something during TRUNCATE. > > I had the idea we were doing that already --- at least I'm pretty sure I > remember it being discussed. But I see it's not being done in HEAD. Patch to do it attached. I am thinking we can do something similar in CLUSTER as well. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Attachment
On Tue, May 15, 2007 at 06:13:47PM -0400, Alvaro Herrera wrote: > Tom Lane wrote: > > Alvaro Herrera <alvherre@commandprompt.com> writes: > > > I suppose it would be pretty trivial to set the relfrozenxid to > > > RecentXmin or something during TRUNCATE. > > > > I had the idea we were doing that already --- at least I'm pretty sure I > > remember it being discussed. But I see it's not being done in HEAD. > > Patch to do it attached. I am thinking we can do something similar in > CLUSTER as well. Actually, it already happens for CLUSTER because cluster calls heap_create_with_catalog, which calls AddNewRelationTuple. See backend/catalog/heap.c line 716. -- Jim Nasby decibel@decibel.org EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)
Jim C. Nasby wrote: > On Tue, May 15, 2007 at 06:13:47PM -0400, Alvaro Herrera wrote: > > Tom Lane wrote: > > > Alvaro Herrera <alvherre@commandprompt.com> writes: > > > > I suppose it would be pretty trivial to set the relfrozenxid to > > > > RecentXmin or something during TRUNCATE. > > > > > > I had the idea we were doing that already --- at least I'm pretty sure I > > > remember it being discussed. But I see it's not being done in HEAD. > > > > Patch to do it attached. I am thinking we can do something similar in > > CLUSTER as well. > > Actually, it already happens for CLUSTER because cluster calls > heap_create_with_catalog, which calls AddNewRelationTuple. See > backend/catalog/heap.c line 716. Right, but that heap is dropped later, and only the relfilenode remains, because they are swapped. In any case the change is a very small patch, which I attach but I haven't tested. This only works if the new rewriteheap stuff actually changes Xids to follow OldestXmin, i.e. all tuples that have older Xmin/Xmax are frozen (or marked with the current Xid). Heikki, can you confirm that this is the case? -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Attachment
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane wrote:
>> I had the idea we were doing that already --- at least I'm pretty sure I
>> remember it being discussed. But I see it's not being done in HEAD.
> Patch to do it attached. I am thinking we can do something similar in
> CLUSTER as well.
Umm ... you'd have to be a lot more conservative in CLUSTER now that
it's MVCC-safe. I don't say that CLUSTER can't push up relfrozenxid,
but there's something wrong if CLUSTER and TRUNCATE are trying to
push it up the same amount.
regards, tom lane
Alvaro Herrera wrote: > In any case the change is a very small patch, which I attach but I > haven't tested. This only works if the new rewriteheap stuff actually > changes Xids to follow OldestXmin, i.e. all tuples that have older > Xmin/Xmax are frozen (or marked with the current Xid). Heikki, can you > confirm that this is the case? No, CLUSTER doesn't freeze tuples. It could do that easily, it didn't occur to me when I wrote it. It would make it harder to debug, though, should we have any problems with it in the future. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > Tom Lane wrote: > >> I had the idea we were doing that already --- at least I'm pretty sure I > >> remember it being discussed. But I see it's not being done in HEAD. > > > Patch to do it attached. I am thinking we can do something similar in > > CLUSTER as well. > > Umm ... you'd have to be a lot more conservative in CLUSTER now that > it's MVCC-safe. I don't say that CLUSTER can't push up relfrozenxid, > but there's something wrong if CLUSTER and TRUNCATE are trying to > push it up the same amount. No, TRUNCATE will use RecentXmin while the CLUSTER patch I posted uses OldestXmin, which is what the HeapTupleSatisfiesUpdate test was using. However, given that Heikki just confirmed that CLUSTER does not freeze tuples, it's not really possible to do this, so I'll drop the CLUSTER patch for now. This means that people using CLUSTER to compact tables won't have the benefit of advancing relfrozenxid, so they will have to run VACUUM on those tables at some point anyway, even though there will be no dead tuples :-( -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > However, given that Heikki just confirmed that CLUSTER does not freeze > tuples, it's not really possible to do this, so I'll drop the CLUSTER > patch for now. > > This means that people using CLUSTER to compact tables won't have the > benefit of advancing relfrozenxid, so they will have to run VACUUM on > those tables at some point anyway, even though there will be no dead > tuples :-( Now that I think this a bit more, I think CLUSTER should freeze the tuples. I was worried about losing valuable debug information by doing that, but thinking a bit more that's not a big concern: we wouldn't freeze tuples newer than recent xmin. The update chain logic is the most risky part of the code, and we wouldn't lose the xmin and xmax of tuples that are part of update chains. Patch attached. You'll need the changes to cluster.c to set the relfrozenid as well. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com Index: src/backend/access/heap/rewriteheap.c =================================================================== RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/access/heap/rewriteheap.c,v retrieving revision 1.3 diff -c -r1.3 rewriteheap.c *** src/backend/access/heap/rewriteheap.c 17 Apr 2007 21:29:31 -0000 1.3 --- src/backend/access/heap/rewriteheap.c 16 May 2007 13:42:23 -0000 *************** *** 538,543 **** --- 538,545 ---- OffsetNumber newoff; HeapTuple heaptup; + heap_freeze_tuple(tup->t_data, state->rs_oldest_xmin, NULL); + /* * If the new tuple is too big for storage or contains already toasted * out-of-line attributes from some other relation, invoke the toaster.
Heikki Linnakangas wrote: > Alvaro Herrera wrote: > >However, given that Heikki just confirmed that CLUSTER does not freeze > >tuples, it's not really possible to do this, so I'll drop the CLUSTER > >patch for now. > > > >This means that people using CLUSTER to compact tables won't have the > >benefit of advancing relfrozenxid, so they will have to run VACUUM on > >those tables at some point anyway, even though there will be no dead > >tuples :-( > > Now that I think this a bit more, I think CLUSTER should freeze the > tuples. I was worried about losing valuable debug information by doing > that, but thinking a bit more that's not a big concern: we wouldn't > freeze tuples newer than recent xmin. The update chain logic is the most > risky part of the code, and we wouldn't lose the xmin and xmax of tuples > that are part of update chains. > > Patch attached. You'll need the changes to cluster.c to set the > relfrozenid as well. Thanks, committed. (I changed NULL for InvalidBuffer). -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote: > Tom Lane wrote: > > Alvaro Herrera <alvherre@commandprompt.com> writes: > > > I suppose it would be pretty trivial to set the relfrozenxid to > > > RecentXmin or something during TRUNCATE. > > > > I had the idea we were doing that already --- at least I'm pretty sure I > > remember it being discussed. But I see it's not being done in HEAD. > > Patch to do it attached. I am thinking we can do something similar in > CLUSTER as well. Applied. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Heikki Linnakangas <heikki@enterprisedb.com> writes:
> Now that I think this a bit more, I think CLUSTER should freeze the
> tuples.
I disagree with that...
regards, tom lane
Tom Lane wrote: > Heikki Linnakangas <heikki@enterprisedb.com> writes: >> Now that I think this a bit more, I think CLUSTER should freeze the >> tuples. > > I disagree with that... Ok. Why? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki@enterprisedb.com> writes:
> Tom Lane wrote:
>> Heikki Linnakangas <heikki@enterprisedb.com> writes:
>>> Now that I think this a bit more, I think CLUSTER should freeze the
>>> tuples.
>>
>> I disagree with that...
> Ok. Why?
It's removing potentially-important data without any notice or recourse
to the user. There seems to be a contingent around here that thinks
that as soon as xmin is older than GlobalXmin it is no longer of
interest to anyone, but I have lost count of how often I have found it
invaluable for forensic purposes. I have resisted having VACUUM freeze
tuples before they've reached a quite-respectable age, and I object to
having CLUSTER do it either.
I could maybe accept a CLUSTER FREEZE option to do this, but that's not
what's in the patch.
regards, tom lane
Tom Lane wrote: > It's removing potentially-important data without any notice or recourse > to the user. There seems to be a contingent around here that thinks > that as soon as xmin is older than GlobalXmin it is no longer of > interest to anyone, but I have lost count of how often I have found it > invaluable for forensic purposes. I have resisted having VACUUM freeze > tuples before they've reached a quite-respectable age, and I object to > having CLUSTER do it either. How about freezing anything older than vacuum_freeze_min_age, just like VACUUM does? > I could maybe accept a CLUSTER FREEZE option to do this, but that's not > what's in the patch. I wouldn't like to add more options to CLUSTER, people are already confused about the similar VACUUM options. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki@enterprisedb.com> writes:
> Tom Lane wrote:
>> ... I have resisted having VACUUM freeze
>> tuples before they've reached a quite-respectable age, and I object to
>> having CLUSTER do it either.
> How about freezing anything older than vacuum_freeze_min_age, just like
> VACUUM does?
I suppose that'd be OK, but is it likely to be worth the trouble?
>> I could maybe accept a CLUSTER FREEZE option to do this, but that's not
>> what's in the patch.
> I wouldn't like to add more options to CLUSTER, people are already
> confused about the similar VACUUM options.
Agreed, I wasn't thrilled with that idea.
regards, tom lane
Tom Lane wrote: > Heikki Linnakangas <heikki@enterprisedb.com> writes: > > Tom Lane wrote: > >> ... I have resisted having VACUUM freeze > >> tuples before they've reached a quite-respectable age, and I object to > >> having CLUSTER do it either. > > > How about freezing anything older than vacuum_freeze_min_age, just like > > VACUUM does? > > I suppose that'd be OK, but is it likely to be worth the trouble? I think so, because it means that people using CLUSTER to keep the size of tables in line instead of VACUUM, would not need the otherwise mandatory VACUUM. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane wrote:
>> Heikki Linnakangas <heikki@enterprisedb.com> writes:
>>> How about freezing anything older than vacuum_freeze_min_age, just like
>>> VACUUM does?
>>
>> I suppose that'd be OK, but is it likely to be worth the trouble?
> I think so, because it means that people using CLUSTER to keep the size
> of tables in line instead of VACUUM, would not need the otherwise
> mandatory VACUUM.
Fair enough. Who will fix the already-applied patch?
regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > Tom Lane wrote: > >> Heikki Linnakangas <heikki@enterprisedb.com> writes: > >>> How about freezing anything older than vacuum_freeze_min_age, just like > >>> VACUUM does? > >> > >> I suppose that'd be OK, but is it likely to be worth the trouble? > > > I think so, because it means that people using CLUSTER to keep the size > > of tables in line instead of VACUUM, would not need the otherwise > > mandatory VACUUM. > > Fair enough. Who will fix the already-applied patch? I'm on it. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > Tom Lane wrote: > >> Heikki Linnakangas <heikki@enterprisedb.com> writes: > >>> How about freezing anything older than vacuum_freeze_min_age, just like > >>> VACUUM does? > >> > >> I suppose that'd be OK, but is it likely to be worth the trouble? > > > I think so, because it means that people using CLUSTER to keep the size > > of tables in line instead of VACUUM, would not need the otherwise > > mandatory VACUUM. > > Fair enough. Who will fix the already-applied patch? Here is my proposed patch. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachment
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Here is my proposed patch.
Actually, the original patch in this series was fairly horrid, and
things haven't been made better by the subsequent changes. It lacked
any comment explaining what it was doing; failed to comment on the way
it was abusing heap_freeze_tuple (the latter thinks it's getting a tuple
that's in a disk buffer); and IMHO puts the heap_freeze_tuple call in
the wrong place anyway. raw_heap_insert has no business editorializing
on the tuple it's given. It'd be better to have the call in
rewrite_heap_tuple, which is already busy messing with the tuple's
visibility info. Perhaps like this, in addition to your changes:
*** src/backend/access/heap/rewriteheap.c~ Wed May 16 19:22:55 2007
--- src/backend/access/heap/rewriteheap.c Wed May 16 19:54:46 2007
***************
*** 292,298 ****
/*
* Add a tuple to the new heap.
*
! * Visibility information is copied from the original tuple.
*
* state opaque state as returned by begin_heap_rewrite
* old_tuple original tuple in the old heap
--- 292,300 ----
/*
* Add a tuple to the new heap.
*
! * Visibility information is copied from the original tuple, except that
! * we "freeze" very-old tuples. Note that since we scribble on new_tuple,
! * it had better be temp storage not a pointer to the original tuple.
*
* state opaque state as returned by begin_heap_rewrite
* old_tuple original tuple in the old heap
***************
*** 324,329 ****
--- 326,342 ----
old_tuple->t_data->t_infomask & HEAP_XACT_MASK;
/*
+ * While we have our hands on the tuple, we may as well freeze any
+ * very-old xmin or xmax, so that future VACUUM effort can be saved.
+ *
+ * Note we abuse heap_freeze_tuple() a bit here, since it's expecting
+ * to be given a pointer to a tuple in a disk buffer. It happens
+ * though that we can get the right things to happen by passing
+ * InvalidBuffer for the buffer.
+ */
+ heap_freeze_tuple(new_tuple->t_data, state->rs_oldest_xmin, InvalidBuffer);
+
+ /*
* Invalid ctid means that ctid should point to the tuple itself.
* We'll override it later if the tuple is part of an update chain.
*/
***************
*** 537,544 ****
Size len;
OffsetNumber newoff;
HeapTuple heaptup;
-
- heap_freeze_tuple(tup->t_data, state->rs_oldest_xmin, InvalidBuffer);
/*
* If the new tuple is too big for storage or contains already toasted
--- 550,555 ----
regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > Here is my proposed patch. > > Actually, the original patch in this series was fairly horrid, and > things haven't been made better by the subsequent changes. It lacked > any comment explaining what it was doing; failed to comment on the way > it was abusing heap_freeze_tuple (the latter thinks it's getting a tuple > that's in a disk buffer); and IMHO puts the heap_freeze_tuple call in > the wrong place anyway. raw_heap_insert has no business editorializing > on the tuple it's given. It'd be better to have the call in > rewrite_heap_tuple, which is already busy messing with the tuple's > visibility info. Perhaps like this, in addition to your changes: Applied, thanks. Let me know if there is still something you don't like about the current state of cluster or truncate. The part about actually advancing relfrozenxid is still not done though ... -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
> The part about actually advancing relfrozenxid is still not done though ...
Right. Are you intending to make that happen?
regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > The part about actually advancing relfrozenxid is still not done though ... > > Right. Are you intending to make that happen? Yes. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > The part about actually advancing relfrozenxid is still not done though ... > > Right. Are you intending to make that happen? See attached patch. I'm intending to apply this sometime tomorrow. Note that affecting CLUSTER changes code used by the ALTER TABLE rewriting stuff as well; so with this patch, the latter also advances relfrozenxid. I could have chosen to keep it as it was, but instead I chose RecentXmin as the new freeze point. This is correct, because when the table is rewritten, all the tuples are marked the rewriting transaction's Xid, so for all purposes it behaves like a new table. Curiously enough, TOAST tables are handled as fallout of other code, so we don't need to do anything additional for this to work. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support