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

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