Re: [bug fix] Suppress "autovacuum: found orphan temp table" message

On 2014-07-22 09:39:13 -0400, Robert Haas wrote:
> On Tue, Jul 22, 2014 at 6:23 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> Yes, so nobody can convince serious customers that the current behavior
> >> makes real sense.
> >
> > I think you're making lots of noise over a trivial log message.
> >
> >> Could you please reconsider this?
> >
> > No. Just removing a warning isn't the way to solve this. If you want to
> > improve things you'll actually need to improve things not just stick
> > your head into the sand.
>
> I've studied this area of the code before, and I would actually
> proposed to fix this in the opposite way - namely, adopt the logic
> currently used for the wraparound case, which drops the temp table,
> even if wraparound is not an issue.

I'm absolutely not opposed to fixing this for real. I doubt it's
backpatching material, but that's something to judge when we know what
to do.

> The current code seems to be
> laboring under the impression that there's some benefit to keeping the
> temporary relation around, which, as far as I can see, there isn't.
> Now, you could perhaps argue that it's useful for forensics, but that
> presumes that the situation where a backend fails to crash without
> cleaning up its temporary relations is exciting enough to merit an
> investigation, which I believe to be false.

I think the picture here changed with the introduction of the
restart_after_crash GUC - before it it was pretty hard to investigate
anything that involved crashes. So I'm ok with changing things
around. But I think it's more complex than just doing the if
(wraparound) in do_autovacuum().

a) There very well could be a backend reconnecting to that
backendId. Then we potentially might try to remove the temp schema
from two backends - I'm not sure that's always going to end up going
well. There's already a race window, but it's pretty darn unlikely to
hit it right now because the wraparound case pretty much implies that
nothing has used that backendid slot for a while.
I guess we could do something like:

LockDatabaseObject(tempschema);
if (SearchSysCacheExists1)
/* bailout */
performDeletion(...);

b) I think at the very least we also need to call RemovePgTempFiles()
during crash restart.

> RemoveTempRelationsCallback just does this:
>
> AbortOutOfAnyTransaction();
> StartTransactionCommand();
> RemoveTempRelations(myTempNamespace);
> CommitTransactionCommand();
>
> RemoveTempRelations uses:
>
> deleteWhatDependsOn(&object, false);

> These are pretty high-level operations, and there are all kinds of
> reasons they could fail.

One could argue, without being very convincing from my pov, that that's
a reason not to always do it from autovacuum because it'll prevent
vacuum from progressing past the borked temporary relation.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services