Horton

Re: Assert failure due to "drop schema pg_temp_3 cascade" for temporary tables and \d+ is not showing any info after drooping temp table schema

Robert Haas <robertmhaas@gmail.com>

View thread View on postgr.es
From: Robert Haas <robertmhaas@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Mahendra Singh <mahi6run@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2020-01-06T17:25:19Z
Lists: pgsql-bugs, pgsql-hackers 
On Sun, Jan 5, 2020 at 8:42 PM Michael Paquier <michael@paquier.xyz> wrote:
> The behavior of the code in 246a6c8 has changed so as a non-existing
> temporary namespace is considered as not in use, in which case
> autovacuum would consider this relation to be orphaned, and it would
> then try to drop it.  Anyway, just a revert of the patch is not a good
> idea either, because keeping around the old behavior allows any user
> to create orphaned relations that autovacuum would just ignore in
> 9.4~10, leading to ultimately a forced shutdown of the instance as no
> cleanup can happen if this goes unnoticed.  This also puts pg_class
> into an inconsistent state as pg_class entries would include
> references to a namespace that does not exist for sessions still
> holding its own references to myTempNamespace/myTempToastNamespace.

I'm not arguing for a revert of 246a6c8.  I think we should just change this:

                ereport(LOG,
                                (errmsg("autovacuum: dropping orphan
temp table \"%s.%s.%s\"",
                                                get_database_name(MyDatabaseId),

get_namespace_name(classForm->relnamespace),
                                                NameStr(classForm->relname))));

To look more like:

char *nspname = get_namespace_name(classForm->relnamespace);
if (nspname != NULL)
   ereport(..."autovacuum: dropping orphan temp table \"%s.%s.%s\"...)
else
   ereport(..."autovacuum: dropping orphan temp table with OID %u"....)

If we do that, then I think we can just revert
a052f6cbb84e5630d50b68586cecc127e64be639 completely. As a side
benefit, this would also provide some insurance against other
possibly-problematic situations, like a corrupted pg_class row with a
garbage value in the relnamespace field, which is something I've seen
multiple times in the field.

I can't quite understand your comments about why we shouldn't do that,
but the reported bug is just a null pointer reference. Incredibly,
autovacuum.c seems to have been using get_namespace_name() without a
null check since 2006, so it's not really the fault of your patch as I
had originally thought. I wonder how in the world we've managed to get
away with it for as long as we have.

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

Commits

  1. Avoid failure if autovacuum tries to access a just-dropped temp namespace.

  2. Revert "Forbid DROP SCHEMA on temporary namespaces"

  3. Forbid DROP SCHEMA on temporary namespaces

  4. Make autovacuum more aggressive to remove orphaned temp tables

  5. Code review for early drop of orphaned temp relations in autovacuum.