Re: Fun fact about autovacuum and orphan temp tables - Mailing list pgsql-hackers

On Wed, Nov 16, 2016 at 11:14 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Hm. Thinking about that again, having a GUC to control if orphaned
> temp tables in autovacuum is an overkill (who is going to go into this
> level of tuning!?) and that we had better do something more aggressive
> as there have been cases of users complaining about dangling temp
> tables. I suspect the use case where people would like to have a look
> at orphaned temp tables after a backend crash is not that wide, at
> least a method would be to disable autovacuum after a crash if such a
> monitoring is necessary. Tom has already stated upthread that the
> patch to remove wildly locks is not acceptable, and he's clearly
> right.
>
> So the best move would be really to make the removal of orphaned temp
> tables more aggressive, and not bother about having a GUC to control
> that. The patch sent in
> https://www.postgresql.org/message-id/CAB7nPqSRYwaz1i12mPkH06_roO3ifgCgR88_aeX1OEg2r4OaNw@mail.gmail.com
> does so, so I am marking the CF entry as ready for committer for this
> patch to attract some committer's attention on the matter.

The whole point of the review process is to get an opinion from
somebody other than the original author on the patch in question.  If
you write a patch and then mark your own patch Ready for Committer,
you're defeating the entire purpose of this process.  I think that's
what you did here, I think you have done it on other occasions in the
not-too-distant patch, and I wish you'd stop.  I understand perfectly
well that there are times when a committer needs to involve themselves
directly in a patch even when nobody else has reviewed it, because
that just has to happen, and I try to keep an eye out for such
scenarios, but it's frustrating to clear time to work on the RfC
backlog and then discover patches that have just been marked that way
without discussion or agreement.

Now, on the substance of this issue, I think your proposed change to
clean up temporary tables immediately rather than waiting until they
become old enough to threaten wraparound is a clear improvement, and
IMHO we should definitely do it.  However, I think your proposed
documentation changes are pretty well inadequate.  If somebody
actually does want to get at the temporary tables of a crashed
backend, they will need to do a lot more than what you mention in that
update.  Even if they set autovacuum=off, they will be vulnerable to
having those tables removed by another backend with the same backend
ID that reinitializes the temporary schema.  And even if that doesn't
happen, they won't be able to select from those tables because they'll
get an error about reading temporary tables of another session.  To
fix that they'd have to move the temporary tables into some other
schema AND change the filenames on disk.  And then on top of that, by
the time anybody even found this in the documentation, it would be far
too late to shut off autovacuum in the first place because it runs
every minute (unless you have raised autovacuum_naptime, which you
shouldn't).  I think we just shouldn't document this.  The proposed
behavior change is basically switching from an extremely conservative
behavior with surprising consequences to what people would expect
anyway, and anyone who needs to dig information out of another
session's temporary tables is going to need to know a lot more about
the server internals than we normally explain in the documentation.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company