Cleaning up unreferenced table files
Here's a patch for the TODO item "Remove unreferenced table files created by transactions
that were in-progress when the server terminated abruptly."
It adds a new function, CleanupStaleRelFiles, that scans through the data
directory and removes all table files that are not mentioned in pg_class
of the corresponding database. CleanupStaleRelFiles is called after WAL
recovery.
Actually, the patch doesn't currently delete the files, just issues a
warning. Testing is easier if the files don't keep getting deleted :).
The patch also adds a GetTablespacePath function similar to
GetDatabasePath that constructs the path to a tablespace symbolic link.
commands/tablespace.c is modified to use it, in addition to the new
CleanupStaleRelFiles function.
- Heikki
Attachments:
cleanup_stales_20050305.difftext/plain; charset=US-ASCII; name=cleanup_stales_20050305.diffDownload+237-9
Heikki Linnakangas <hlinnaka@iki.fi> writes:
Here's a patch for the TODO item "Remove unreferenced table files created by transactions
that were in-progress when the server terminated abruptly."
xlog.c is a fairly random place to put that functionality. Didn't it
strike any warning bells for you when you had to add so many new
#includes? I'm not entirely sure where this should go, but not there.
regards, tom lane
On Sat, 5 Mar 2005, Tom Lane wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
Here's a patch for the TODO item "Remove unreferenced table files created by transactions
that were in-progress when the server terminated abruptly."xlog.c is a fairly random place to put that functionality. Didn't it
strike any warning bells for you when you had to add so many new
#includes? I'm not entirely sure where this should go, but not there.
Yeah actually it did, but I forgot about it along the way. How about
putting it in a file of its own in backend/catalog? There's some code that
also deals with the data directories. Or straight into backend/storage.
- Heikki
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On Sat, 5 Mar 2005, Tom Lane wrote:
xlog.c is a fairly random place to put that functionality. Didn't it
strike any warning bells for you when you had to add so many new
#includes? I'm not entirely sure where this should go, but not there.
Yeah actually it did, but I forgot about it along the way. How about
putting it in a file of its own in backend/catalog? There's some code that
also deals with the data directories. Or straight into backend/storage.
Actually, you could make some case for putting it in utils/init/ beside
flatfiles.c, which has got much the same sort of issues to deal with.
I think though that we ought to first consider the question of whether
we *want* this functionality. On reflection I'm fairly nervous about
the idea of actually deleting anything during startup --- seems like a
good recipe for turning small failures into large failures. But if
we're not going to delete anything then it's questionable whether we
need to code it like this at all. It'd certainly be easier and safer to
examine these tables after the system is up and running normally.
regards, tom lane
Tom Lane wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On Sat, 5 Mar 2005, Tom Lane wrote:
xlog.c is a fairly random place to put that functionality. Didn't it
strike any warning bells for you when you had to add so many new
#includes? I'm not entirely sure where this should go, but not there.Yeah actually it did, but I forgot about it along the way. How about
putting it in a file of its own in backend/catalog? There's some code that
also deals with the data directories. Or straight into backend/storage.Actually, you could make some case for putting it in utils/init/ beside
flatfiles.c, which has got much the same sort of issues to deal with.I think though that we ought to first consider the question of whether
we *want* this functionality. On reflection I'm fairly nervous about
the idea of actually deleting anything during startup --- seems like a
good recipe for turning small failures into large failures. But if
we're not going to delete anything then it's questionable whether we
need to code it like this at all. It'd certainly be easier and safer to
examine these tables after the system is up and running normally.
Let's discuss this. The patch as submitted checks for unreferenced
files on bootup and reports them in the log file, but does not delete
them. That seems like the proper behavior. I think we delete from
pgsql_tmp on bootup, but we _know_ those aren't referenced.
What other user interface would trigger this if we did it after startup?
Wouldn't we have to lock pg_class against VACUUM while we scan the file
system, and are we sure we do things in pg_class or the file system
first consistently? It seems much more prone to error doing it while
the system is running.
I guess I am happy with just reporting during startup like the patch
does now.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073On Mon, 25 Apr 2005, Bruce Momjian wrote:
Tom Lane wrote:
...
I think though that we ought to first consider the question of whether
we *want* this functionality. On reflection I'm fairly nervous about
the idea of actually deleting anything during startup --- seems like a
good recipe for turning small failures into large failures. But if
we're not going to delete anything then it's questionable whether we
need to code it like this at all. It'd certainly be easier and safer to
examine these tables after the system is up and running normally.Let's discuss this. The patch as submitted checks for unreferenced
files on bootup and reports them in the log file, but does not delete
them. That seems like the proper behavior. I think we delete from
pgsql_tmp on bootup, but we _know_ those aren't referenced.What other user interface would trigger this if we did it after startup?
Wouldn't we have to lock pg_class against VACUUM while we scan the file
system, and are we sure we do things in pg_class or the file system
first consistently? It seems much more prone to error doing it while
the system is running.
I agree.
Also, you can only have stale files after a backend crash, since they are
normally cleaned up at the end of transaction. If it was a separate
program or command, the administrator would have to be aware
of the issue. Otherwise, he wouldn't know he needs to run it after a
crash.
I feel that crashes that leaves behind stale files are rare. You
would need an application that creates/drops tables as part of normal
operation. Some kind of a large batch load might do that: BEGIN, CREATE
TABLE foo, COPY 1 GB of data, COMMIT.
The nasty thing right now is, you might end up with 1 GB of wasted disk
space, and never even know it.
I guess I am happy with just reporting during startup like the patch
does now.
Ok. I'll fix the design issues Tom addressed earlier, add documentation,
and resubmit.
We can come back to this after a release or two, when we have more
confidence in the feature. Maybe we'll also get some feedback on how often
those stale files occur in practice.
- Heikki
On Tue, 26 Apr 2005, Heikki Linnakangas wrote:
On Mon, 25 Apr 2005, Bruce Momjian wrote:
...
I guess I am happy with just reporting during startup like the patch
does now.Ok. I'll fix the design issues Tom addressed earlier, add documentation, and
resubmit.
Here you go.
The new functionality is now separated in a new file
backend/utils/init/cleanup.c.
There was code in many places that constructs the path to a tablespace
directory. I refactored that into a new function called GetTablespacePath
and put it next to GetDatabasePath in catalog.c.
I added a section under the "Routine Database Maintenance Tasks" that
basically gives a heads up that these notifications can appear in the log
after a crash.
- Heikki
Heikki Linnakangas <hlinnaka@iki.fi> writes:
I feel that crashes that leaves behind stale files are rare.
Indeed, and getting more so all the time ... which makes me question
the value of doing anything about this at all.
regards, tom lane
On Tue, 26 Apr 2005, Alvaro Herrera wrote:
You forgot the attachment?
Damn. It happens time after time...
- Heikki
Attachments:
cleanup_stales_20050427.difftext/plain; CHARSET=US-ASCII; NAME=cleanup_stales_20050427.diffDownload+282-23
Import Notes
Reply to msg id not found: 20050427004548.GB6915@dcc.uchile.cl
On Tue, 26 Apr 2005, Tom Lane wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
I feel that crashes that leaves behind stale files are rare.
Indeed, and getting more so all the time ...
How so? Have changes been made in those parts of the code?
which makes me question the value of doing anything about this at all.
What bothers me is that we currently have no means of knowing if it
happens and how often it happens, so we're just guessing that "it's
rare". How rare? We don't know.
If nobody ever runs into this issue in production, and this whole exercise
turns out to be completely unnecessary, at least we'll know. That alone
makes me feel better.
The only drawback of the patch that I can see is the performance impact on
recovery. And I think the time it takes to scan the data directories isn't
much compared to WAL replay.
- Heikki
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On Tue, 26 Apr 2005, Tom Lane wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
I feel that crashes that leaves behind stale files are rare.
Indeed, and getting more so all the time ...
How so? Have changes been made in those parts of the code?
No, just that the overall reliability of Postgres keeps improving.
At the time that TODO entry was created, I don't think we even had the
ability to roll back table creates/drops properly, so there were
scenarios in which unreferenced files could be left behind without even
assuming any software error. And the prevalence of backend crashes was
way higher than it is now, too. So I think a good argument could be
made that the TODO item isn't nearly as important as it was at the time.
If nobody ever runs into this issue in production, and this whole exercise
turns out to be completely unnecessary, at least we'll know. That alone
makes me feel better.
We will know no such thing, unless the patch is made to announce the
problem so intrusively that people are certain to see it *and report it
to us*. Which I don't think will be acceptable.
regards, tom lane
On Wed, 27 Apr 2005, Tom Lane wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On Tue, 26 Apr 2005, Tom Lane wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
I feel that crashes that leaves behind stale files are rare.
Indeed, and getting more so all the time ...
How so? Have changes been made in those parts of the code?
No, just that the overall reliability of Postgres keeps improving.
At the time that TODO entry was created, I don't think we even had the
ability to roll back table creates/drops properly, so there were
scenarios in which unreferenced files could be left behind without even
assuming any software error. And the prevalence of backend crashes was
way higher than it is now, too. So I think a good argument could be
made that the TODO item isn't nearly as important as it was at the time.
*shrug*. I won't push any harder if you feel strongly against the patch.
Should we just remove the TODO item? And/or document the issue?
- Heikki
Tom Lane wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On Tue, 26 Apr 2005, Tom Lane wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
I feel that crashes that leaves behind stale files are rare.
Indeed, and getting more so all the time ...
How so? Have changes been made in those parts of the code?
No, just that the overall reliability of Postgres keeps improving.
At the time that TODO entry was created, I don't think we even had the
ability to roll back table creates/drops properly, so there were
scenarios in which unreferenced files could be left behind without even
assuming any software error. And the prevalence of backend crashes was
way higher than it is now, too. So I think a good argument could be
made that the TODO item isn't nearly as important as it was at the time.If nobody ever runs into this issue in production, and this whole exercise
turns out to be completely unnecessary, at least we'll know. That alone
makes me feel better.We will know no such thing, unless the patch is made to announce the
problem so intrusively that people are certain to see it *and report it
to us*. Which I don't think will be acceptable.
Well, if putting it in the server logs isn't enough, I don't know what
is.
I think we do need the patch, at least to find out if there is an issue
we don't know about. I don't see the hard in it.
Heikki, would you time startup and tell us what percentage of time is
taken by the routines? Or I can do it.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Bruce Momjian <pgman@candle.pha.pa.us> writes:
I think we do need the patch, at least to find out if there is an issue
we don't know about.
My point is that we won't find out anything, because we will have no
idea if people are noticing the log entries at all, much less telling
us about 'em. Of course if they do tell us then we'd know something,
but what I am expecting is a vast silence. We will not be able to tell
the difference between "no problem" and "very infrequent problem" any
better than we can now.
regards, tom lane
Tom Lane wrote:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
I think we do need the patch, at least to find out if there is an issue
we don't know about.My point is that we won't find out anything, because we will have no
idea if people are noticing the log entries at all, much less telling
us about 'em. Of course if they do tell us then we'd know something,
but what I am expecting is a vast silence. We will not be able to tell
the difference between "no problem" and "very infrequent problem" any
better than we can now.
I think people do look at their logs. We are certainly better off
finding it than having no reporting at all.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Uh, you forgot to add cleanup.h.
---------------------------------------------------------------------------
Heikki Linnakangas wrote:
On Tue, 26 Apr 2005, Alvaro Herrera wrote:
You forgot the attachment?
Damn. It happens time after time...
- Heikki
Content-Description:
[ Attachment, skipping... ]
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073How embarrasing...
I hope it's all there now.
On Thu, 28 Apr 2005, Bruce Momjian wrote:
Uh, you forgot to add cleanup.h.
---------------------------------------------------------------------------
Heikki Linnakangas wrote:
On Tue, 26 Apr 2005, Alvaro Herrera wrote:
You forgot the attachment?
Damn. It happens time after time...
- Heikki
Content-Description:
[ Attachment, skipping... ]
-- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
- Heikki
Attachments:
cleanup_stales_20050428.difftext/plain; charset=US-ASCII; name=cleanup_stales_20050428.diffDownload+297-23
Heikki,
Good patch.
...The patch makes no mention of temporary files, which are left there
after a crash.
Temp files are only removed on a normal startup, whereas the patch
invokes removal on both normal startup and crash recovery. That doesn't
make much sense...
Also, temp file deletion happens in the postmaster, not at the end of
recovery in xlog.c.
Could we rationalise those two? One place, one action for both?
I'd rather have this also as an administrator command rather than as
something automatically run after crash recovery. That way I have my
debugging opportunity, but the admin can remove them without restarting
the server.
Same code, just a Function instead...
Best Regards, Simon Riggs
reference from fd.c: (this is not a patch)
/*
* Remove temporary files left over from a prior postmaster session
*
* This should be called during postmaster startup. It will forcibly
* remove any leftover files created by OpenTemporaryFile.
*
* NOTE: we could, but don't, call this during a post-backend-crash
restart
* cycle. The argument for not doing it is that someone might want to
examine
* the temp files for debugging purposes. This does however mean that
* OpenTemporaryFile had better allow for collision with an existing
temp
* file name.
*/
void
RemovePgTempFiles(void)
FYI, his patch is in CVS and now only _reports_ unreferenced files, and
it happens on recovery and normal startup.
---------------------------------------------------------------------------
Simon Riggs wrote:
Heikki,
Good patch.
...The patch makes no mention of temporary files, which are left there
after a crash.Temp files are only removed on a normal startup, whereas the patch
invokes removal on both normal startup and crash recovery. That doesn't
make much sense...Also, temp file deletion happens in the postmaster, not at the end of
recovery in xlog.c.Could we rationalise those two? One place, one action for both?
I'd rather have this also as an administrator command rather than as
something automatically run after crash recovery. That way I have my
debugging opportunity, but the admin can remove them without restarting
the server.Same code, just a Function instead...
Best Regards, Simon Riggs
reference from fd.c: (this is not a patch)
/*
* Remove temporary files left over from a prior postmaster session
*
* This should be called during postmaster startup. It will forcibly
* remove any leftover files created by OpenTemporaryFile.
*
* NOTE: we could, but don't, call this during a post-backend-crash
restart
* cycle. The argument for not doing it is that someone might want to
examine
* the temp files for debugging purposes. This does however mean that
* OpenTemporaryFile had better allow for collision with an existing
temp
* file name.
*/
void
RemovePgTempFiles(void)---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073