Patch to remove sort files, temp tables, unreferenced files
The following patch does full referential checking of sort files, temp
tables, and table files. It checks references of every file and temp
table, and reports or removes it. The code only removes files it is
certain about.
A typical output report is:
test=> VACUUM;
NOTICE: Unusual file found in temporary sort directory. This file is not
normally created by PostgreSQL and can be removed by the
administrator using 'rm':
/usr/var/local/postgres/data/base/18617/pg_sorttemp/x
NOTICE: Unusual file found in database directory. This file is not
normally created by PostgreSQL and can be removed by the
administrator using 'rm':
/usr/var/local/postgres/data/base/18617/x
NOTICE: Core file found in database directory. If you don't need it
for debugging, the administrator can remove it using 'rm':
/usr/var/local/postgres/data/base/18617/core
NOTICE: Unreferenced table file found in database directory. This
could have been left from a database crash. If no one was
using the database during VACUUM, the file can be safely
removed by the administrator using 'rm':
/usr/var/local/postgres/data/base/18617/33333
Most of the work is done as part of a full database VACUUM. Postmaster
startup also cleans all sort files.
It uses a SnapshotAny read of pg_class to find referenced files. It
does not use Oid ranges anymore.
I also had to add ShmemPIDAdd() and redo ShmemPIDLookup() because the
ShmemPIDLookup() API was weird.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026Attachments:
/pgpatches/cleantext/plainDownload+418-59
NOTICE: Unreferenced table file found in database directory. This
could have been left from a database crash. If no one was
using the database during VACUUM, the file can be safely
removed by the administrator using 'rm':
/usr/var/local/postgres/data/base/18617/33333
I think this wording is overly cautious. I will change it to:
NOTICE: Unreferenced table file found in database directory. This
could have been left from a database crash. This file can be
safely removed by the administrator using 'rm':
/usr/var/local/postgres/data/base/18617/33333
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
The following patch does full referential checking of sort files, temp
tables, and table files. It checks references of every file and temp
table, and reports or removes it. The code only removes files it is
certain about.
Plus some that it's not certain about.
I still think that this whole business is dangerous and of unproven
usefulness. Removing sorttemp files at postmaster start is OK ---
there's no question that they are temp, and there's no question (after
we have the postmaster lock) that no one needs them anymore. Anything
else is just asking for trouble.
I am not comforted by the fact that the code doesn't remove data files
itself, but only recommends that the dbadmin do it; just how closely
do you think the average DBA will examine such recommendations? The
first time someone removes a file he needed because "the system told me
to", your patch will have done damage outweighing the total positive
effect it could ever have anywhere.
Now, as to the lesser matter of exactly how many holes there are in this
particular version:
1. Removing sorttemp files during vacuum is not safe; you cannot know
that they don't belong to any running backend. (No, the fact that you
looked in PROC and didn't find the PID doesn't prove anything; the same
PID could have been reassigned to a new backend since you looked.)
2. Removing temp tables during vacuum is not safe either, first because
of the fact that the PID check is unsafe, and second because it could
cause vacuum to fail entirely (suppose the owning backend terminates and
removes the temp table just before you do?).
3. Warning about relation nodes that you didn't find in a previous scan
of pg_class is obviously unsafe; they could have been created since you
looked in pg_class.
You can't really get around any of these problems by doing things in a
different order, either; that'd just shift the vulnerabilities. The
only way to do it safely would be to acquire locks that would prevent
other backends from creating/deleting files while you make the checks.
That's not reasonable.
In short, the only parts of this patch that I think are acceptable are
the changes to move sorttemp files into their own directory and to
remove sorttemp files during postmaster start.
BTW, while I approve of moving sorttemp files into their own
subdirectory, it's sheer folly to give them names therein that look
so much like regular relation file names. They should still be named
something like "sorttempNNN".
regards, tom lane
Bruce Momjian <pgman@candle.pha.pa.us> writes:
The following patch does full referential checking of sort files, temp
tables, and table files. It checks references of every file and temp
table, and reports or removes it. The code only removes files it is
certain about.Plus some that it's not certain about.
I still think that this whole business is dangerous and of unproven
usefulness. Removing sorttemp files at postmaster start is OK ---
there's no question that they are temp, and there's no question (after
we have the postmaster lock) that no one needs them anymore. Anything
else is just asking for trouble.
We can't just throw up our hands and say we can't account for this
left-over stuff. We have to come up with some solution.
I am not comforted by the fact that the code doesn't remove data files
itself, but only recommends that the dbadmin do it; just how closely
do you think the average DBA will examine such recommendations? The
first time someone removes a file he needed because "the system told me
to", your patch will have done damage outweighing the total positive
effect it could ever have anywhere.Now, as to the lesser matter of exactly how many holes there are in this
particular version:1. Removing sorttemp files during vacuum is not safe; you cannot know
that they don't belong to any running backend. (No, the fact that you
looked in PROC and didn't find the PID doesn't prove anything; the same
PID could have been reassigned to a new backend since you looked.)
But I have the directory entry before I look for the PID. Seems safe.
2. Removing temp tables during vacuum is not safe either, first because
of the fact that the PID check is unsafe, and second because it could
cause vacuum to fail entirely (suppose the owning backend terminates and
removes the temp table just before you do?).
Yes, I can't remove temp tables for running backends. The table is
removed before the PID is removed from the hash. I will have to do some
more checking to see that the pg_class entry actually points to a real
data file.
3. Warning about relation nodes that you didn't find in a previous scan
of pg_class is obviously unsafe; they could have been created since you
looked in pg_class.
Actually that is why I was doing the OID range checking but you didn't
like that either. I had the code checking from the lowest OID on each
backend startup to the current OID counter and skipping those. Should I
try adding that again.
You can't really get around any of these problems by doing things in a
different order, either; that'd just shift the vulnerabilities. The
only way to do it safely would be to acquire locks that would prevent
other backends from creating/deleting files while you make the checks.
That's not reasonable.In short, the only parts of this patch that I think are acceptable are
the changes to move sorttemp files into their own directory and to
remove sorttemp files during postmaster start.BTW, while I approve of moving sorttemp files into their own
subdirectory, it's sheer folly to give them names therein that look
so much like regular relation file names. They should still be named
something like "sorttempNNN".
They are in their own directory. Naming as backend_pid.serial number seems
OK to me.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026Bruce Momjian <pgman@candle.pha.pa.us> writes:
We can't just throw up our hands and say we can't account for this
left-over stuff. We have to come up with some solution.
No we don't. I'm not convinced that we have a problem at all anymore.
(Show me field reports from 7.1; older versions are irrelevant to this
discussion.) But I am convinced that a half-baked solution would be
worse than not doing anything.
1. Removing sorttemp files during vacuum is not safe; you cannot know
that they don't belong to any running backend. (No, the fact that you
looked in PROC and didn't find the PID doesn't prove anything; the same
PID could have been reassigned to a new backend since you looked.)
But I have the directory entry before I look for the PID. Seems safe.
No. PIDs wrap around, therefore sorttemp file names recycle. You might
be able to establish that the backend that originally created the file
is gone, but it's still possible that by the time you actually do the
unlink, you are cutting the knees off a new backend that has re-used the
file name.
Basically, you cannot do any of this safely without a lock. If you
think you can, then you don't understand the problem.
I don't think there are any suitable locks in the system at the moment,
and in any case I don't like the loss of concurrency that would occur
if we interlocked process start/stop, sorttemp file
creation/destruction, etc, to the extent that would be needed to make
the world safe for on-line temp file removal. This cure looks much
worse than the disease to me...
BTW, while I approve of moving sorttemp files into their own
subdirectory, it's sheer folly to give them names therein that look
so much like regular relation file names. They should still be named
something like "sorttempNNN".
They are in their own directory. Naming as backend_pid.serial number seems
OK to me.
The code won't get confused, but humans might. Keep in mind also that
the reason for having a separate directory is so that a DBA can symlink
the directory to someplace else. What happens if he symlinks it to a
directory that also contains real data files? Far better to make sure
that temp file names cannot conflict with relation file names, whether
they're in the same directory or not.
regards, tom lane
BTW, while I approve of moving sorttemp files into their own
subdirectory, it's sheer folly to give them names therein that look
so much like regular relation file names. They should still be named
something like "sorttempNNN".They are in their own directory. Naming as backend_pid.serial number seems
OK to me.
I know remember I was going to call the file pid_323.2 so people knew it
was a pid. I will add that.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026Bruce Momjian <pgman@candle.pha.pa.us> writes:
We can't just throw up our hands and say we can't account for this
left-over stuff. We have to come up with some solution.No we don't. I'm not convinced that we have a problem at all anymore.
(Show me field reports from 7.1; older versions are irrelevant to this
discussion.) But I am convinced that a half-baked solution would be
worse than not doing anything.
We already know that crashes will leave these files around, and as we
start using file versioning for CLUSTER and DROP COLUMN, I expect those
orphaned files to continue.
Basically, if I can, I would like to harden our code to report or remove
this cruft. Most commercial database have such tools. Seems VACUUM is
the proper place for us to do it.
However, I agree if I can't make it 100% reliable, it is worse than
useless.
Let me keep trying and we can rip out parts that aren't 100%.
1. Removing sorttemp files during vacuum is not safe; you cannot know
that they don't belong to any running backend. (No, the fact that you
looked in PROC and didn't find the PID doesn't prove anything; the same
PID could have been reassigned to a new backend since you looked.)But I have the directory entry before I look for the PID. Seems safe.
No. PIDs wrap around, therefore sorttemp file names recycle. You might
be able to establish that the backend that originally created the file
is gone, but it's still possible that by the time you actually do the
unlink, you are cutting the knees off a new backend that has re-used the
file name.Basically, you cannot do any of this safely without a lock. If you
think you can, then you don't understand the problem.
You are correct. I didn't realized that after I check the shared memory
for the pid, another backend could start with the pid I was checking and
create a sort file. The sort code doesn't so O_EXCL, so it would
succeed in creating it, and I would have removed it.
I don't think there are any suitable locks in the system at the moment,
and in any case I don't like the loss of concurrency that would occur
if we interlocked process start/stop, sorttemp file
creation/destruction, etc, to the extent that would be needed to make
the world safe for on-line temp file removal. This cure looks much
worse than the disease to me...
OK, my solution was to do this:
else if (ShmemPIDLookup(pid, true) == NULL)
{
snprintf(temp_path, MAXPGPATH,
"%s/%s/%s", cwd, SORT_TEMP_DIR, temp_de->d_name);
/* Make sure no pid gets created and starts using this file */
SpinAcquire(ShmemIndexLock);
if (ShmemPIDLookup(pid, false) == NULL)
unlink(temp_path);
SpinRelease(ShmemIndexLock);
}
I added a boolean flag to ShmemPIDLookup() to control whether the
function does locking. Basically, I do my own locking around the unlink
if I have found a orphaned file. This prevents another backend from
being created under me with the pid I am checking. This code only runs
if I have already found an orphan.
BTW, while I approve of moving sorttemp files into their own
subdirectory, it's sheer folly to give them names therein that look
so much like regular relation file names. They should still be named
something like "sorttempNNN".They are in their own directory. Naming as backend_pid.serial number seems
OK to me.The code won't get confused, but humans might. Keep in mind also that
the reason for having a separate directory is so that a DBA can symlink
the directory to someplace else. What happens if he symlinks it to a
directory that also contains real data files? Far better to make sure
that temp file names cannot conflict with relation file names, whether
they're in the same directory or not.
See attached patch. pid_###.###.
Also, I re-added the startOid/nextoid code to prevent me from looking at
any pg_class changes caused by running backends.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026Attachments:
/pgpatches/cleantext/plainDownload+486-62
Bruce Momjian <pgman@candle.pha.pa.us> writes:
No we don't. I'm not convinced that we have a problem at all anymore.
We already know that crashes will leave these files around, and as we
start using file versioning for CLUSTER and DROP COLUMN, I expect those
orphaned files to continue.
I don't. The intention (not yet implemented, but Vadim's referred to it
repeatedly) is that relation file creation/deletion will be logged in
WAL, and so it can be redone or undone after a crash. That seems a much
more secure approach than anything you've proposed in this thread.
Temp files won't be logged in WAL, but a startup-time cleanup seems
sufficient to deal with them.
You are correct. I didn't realized that after I check the shared memory
for the pid, another backend could start with the pid I was checking and
create a sort file. The sort code doesn't do O_EXCL,
... quite deliberately ... you might want to add a comment to that
effect in OpenTemporaryFile.
regards, tom lane
You are correct. I didn't realized that after I check the shared memory
for the pid, another backend could start with the pid I was checking and
create a sort file. The sort code doesn't do O_EXCL,... quite deliberately ... you might want to add a comment to that
effect in OpenTemporaryFile.
Done.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026OK, Tom doesn't like the majority of my patch. I think it has value.
Would others like to comment?
Bruce Momjian <pgman@candle.pha.pa.us> writes:
No we don't. I'm not convinced that we have a problem at all anymore.
We already know that crashes will leave these files around, and as we
start using file versioning for CLUSTER and DROP COLUMN, I expect those
orphaned files to continue.I don't. The intention (not yet implemented, but Vadim's referred to it
repeatedly) is that relation file creation/deletion will be logged in
WAL, and so it can be redone or undone after a crash. That seems a much
more secure approach than anything you've proposed in this thread.Temp files won't be logged in WAL, but a startup-time cleanup seems
sufficient to deal with them.You are correct. I didn't realized that after I check the shared memory
for the pid, another backend could start with the pid I was checking and
create a sort file. The sort code doesn't do O_EXCL,... quite deliberately ... you might want to add a comment to that
effect in OpenTemporaryFile.regards, tom lane
---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
I agree with Tom's arguments ...
On Wed, 30 May 2001, Bruce Momjian wrote:
OK, Tom doesn't like the majority of my patch. I think it has value.
Would others like to comment?Bruce Momjian <pgman@candle.pha.pa.us> writes:
No we don't. I'm not convinced that we have a problem at all anymore.
We already know that crashes will leave these files around, and as we
start using file versioning for CLUSTER and DROP COLUMN, I expect those
orphaned files to continue.I don't. The intention (not yet implemented, but Vadim's referred to it
repeatedly) is that relation file creation/deletion will be logged in
WAL, and so it can be redone or undone after a crash. That seems a much
more secure approach than anything you've proposed in this thread.Temp files won't be logged in WAL, but a startup-time cleanup seems
sufficient to deal with them.You are correct. I didn't realized that after I check the shared memory
for the pid, another backend could start with the pid I was checking and
create a sort file. The sort code doesn't do O_EXCL,... quite deliberately ... you might want to add a comment to that
effect in OpenTemporaryFile.regards, tom lane
---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?-- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster
Marc G. Fournier ICQ#7615664 IRC Nick: Scrappy
Systems Administrator @ hub.org
primary: scrappy@hub.org secondary: scrappy@{freebsd|postgresql}.org
Bruce Momjian <pgman@candle.pha.pa.us> writes:
No we don't. I'm not convinced that we have a problem at all anymore.
We already know that crashes will leave these files around, and as we
start using file versioning for CLUSTER and DROP COLUMN, I expect those
orphaned files to continue.I don't. The intention (not yet implemented, but Vadim's referred to it
repeatedly) is that relation file creation/deletion will be logged in
WAL, and so it can be redone or undone after a crash. That seems a much
more secure approach than anything you've proposed in this thread.
I prefer a WAL cleanup myself, especially if it can be done for 7.2, but
how do we do that without fsync'ing the WAL every time we create a file?
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026Bruce Momjian <pgman@candle.pha.pa.us> writes:
I don't. The intention (not yet implemented, but Vadim's referred to it
repeatedly) is that relation file creation/deletion will be logged in
WAL, and so it can be redone or undone after a crash. That seems a much
more secure approach than anything you've proposed in this thread.
I prefer a WAL cleanup myself, especially if it can be done for 7.2, but
how do we do that without fsync'ing the WAL every time we create a file?
Yes, we'd need to fsync WAL at the file creation/deletion points. So?
regards, tom lane
Bruce Momjian <pgman@candle.pha.pa.us> writes:
I don't. The intention (not yet implemented, but Vadim's referred to it
repeatedly) is that relation file creation/deletion will be logged in
WAL, and so it can be redone or undone after a crash. That seems a much
more secure approach than anything you've proposed in this thread.I prefer a WAL cleanup myself, especially if it can be done for 7.2, but
how do we do that without fsync'ing the WAL every time we create a file?Yes, we'd need to fsync WAL at the file creation/deletion points. So?
I guess we don't do creation/deletion too much.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026OK, seems I am the only one that is concerned about orphaned files, so,
Tom, would you let me know which parts of the patch you want applied or
apply the parts you want yourself. Thanks.
Bruce Momjian <pgman@candle.pha.pa.us> writes:
I don't. The intention (not yet implemented, but Vadim's referred to it
repeatedly) is that relation file creation/deletion will be logged in
WAL, and so it can be redone or undone after a crash. That seems a much
more secure approach than anything you've proposed in this thread.I prefer a WAL cleanup myself, especially if it can be done for 7.2, but
how do we do that without fsync'ing the WAL every time we create a file?Yes, we'd need to fsync WAL at the file creation/deletion points. So?
regards, tom lane
---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026