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

> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026