Re: Database-level collation version tracking

On Wed, Feb 09, 2022 at 12:48:35PM +0100, Peter Eisentraut wrote:
> On 08.02.22 13:55, Julien Rouhaud wrote:
> > I'm just saying that without such a lock you can easily trigger the "cache
> > lookup" error, and that's something that's supposed to happen with normal
> > usage I think.  So it should be a better message saying that the database has
> > been concurrently dropped, or actually simply does not exist like it's done in
> > AlterDatabaseOwner() for the same pattern:
> > 
> > [...]
> >     tuple = systable_getnext(scan);
> >     if (!HeapTupleIsValid(tuple))
> >         ereport(ERROR,
> >                 (errcode(ERRCODE_UNDEFINED_DATABASE),
> >                  errmsg("database \"%s\" does not exist", dbname)));
> > [...]
> 
> In my code, the existence of the database is checked by
> 
>     dboid = get_database_oid(stmt->dbname, false);
> 
> This also issues an appropriate user-facing error message if the database
> does not exist.

Yes but if you run a DROP DATABASE concurrently  you will either get a
"database does not exist" or "cache lookup failed" depending on whether the
DROP is processed before or after the get_database_oid().

I agree that it's not worth trying to make it concurrent-drop safe, but I also
thought that throwing plain elog(ERROR) should be avoided when reasonably
doable.  And in that situation we know it can happen in normal situation, so
having a real error message looks like a cost-free improvement.  Now if it's
better to have a cache lookup error even in that situation just for safety or
something ok, it's not like trying to refresh a db collation and having someone
else dropping it at the same time is going to be a common practice anway.

> The flow in AlterDatabaseOwner() is a bit different, it looks up the
> pg_database tuple directly from the name.  I think both are correct.  My
> code has been copied from the analogous code in AlterCollation().

I also think it would be better to have a "collation does not exist" in the
syscache failure message, but same here dropping collation is probably even
less frequent than dropping database, let alone while refreshing the collation
version.