Re: Patch to remove sort files, temp tables, unreferenced files

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