Re: Temp table handling after anti-wraparound shutdown (Was: BUG #15840)

Hi,

On 2019-06-08 08:59:37 +0900, Michael Paquier wrote:
> On Fri, Jun 07, 2019 at 03:58:43PM -0700, Andres Freund wrote:
> > Do we need to move the orphan temp cleanup code into database vacuums or
> > such?
>
> When entering into the vacuum() code path for an autovacuum, only one
> relation at a time is processed, and we have prior that extra
> processing related to toast relations when selecting the relations to
> work on, or potentially delete orphaned temp tables. For a manual
> vacuum, we finish by deciding which relation to process in
> get_all_vacuum_rels(), so the localized processing is a bit different
> than what's done in do_autovacuum() when scanning pg_class for
> relations.

Yea, I know. I didn't mean that we should only handle orphan cleanup
only within database wide vacuums, just *also* there. ISTM that'd mean
that at least some of the code ought to be in vacuum.c, and then also
called by autovacuum.c.

> Technically, I think that it would work to give up on the gathering of
> the orphaned OIDs in a gathering and let them be gathered in the list
> of items to vacuum, and then put the deletion logic down to
> vacuum_rel().

I don't think it makes much sense to go there. The API would probably
look pretty bad.

I was more thinking that we'd move the check for orphaned-ness into a
separate function (maybe IsOrphanedRelation()), and move the code to
drop orphan relations into a separate function (maybe
DropOrphanRelations()). That'd limit the amount of code duplication for
doing this both in autovacuum and all-database vacuums quite
considerably.

A more aggressive approach would be to teach vac_update_datfrozenxid()
to ignore orphaned temp tables - perhaps even by heap_inplace'ing an
orphaned table's relfrozenxid/relminmxid with InvalidTransactionId. We'd
not want to do that in do_autovacuum() - otherwise the schema won't get
cleaned up, but for database widevacuums that seems like it could be
good approach.

Random observation while re-reading this code: Having do_autovacuum()
and ExecVacuum() both go through vacuum() seems like it adds too much
complexity to be worth it. Like half of the file is only concerned with
checks related to that.

Greetings,

Andres Freund