Re: [bug fix] Suppress "autovacuum: found orphan temp table" message - Mailing list pgsql-hackers

On 2014-07-22 09:39:13 -0400, Robert Haas wrote:
> On Tue, Jul 22, 2014 at 6:23 AM, Andres Freund <andres@2ndquadrant.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'spretty darn unlikely to  hit it right now because the wraparound case pretty much implies that  nothing has used
thatbackendid 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