Re: Database-level collation version tracking - Mailing list pgsql-hackers

On 07.02.22 11:29, Julien Rouhaud wrote:
> - there should be a mention to the need for a catversion bump in the message
>    comment

done

> - there is no test

Suggestions where to put it?  We don't really have tests for the 
collation-level versioning either, do we?

> - it's missing some updates in create_database.sgml, and psql tab completion
>    for CREATE DATABASE with the new collation_version defelem.

Added to create_database.sgml, but not to psql.  We don't have 
completion for the collation option either, since it's only meant to be 
used by pg_upgrade, not interactively.

> - that's not really something new with this patch, but should we output the
>    collation version info or mismatch info in \l / \dO?

It's a possibility.  Perhaps there is a question of performance if we 
show it in \l and people have tons of databases and they have to make a 
locale call for each one.  As you say, it's more an independent feature, 
but it's worth looking into.

> +       if (!actual_versionstr)
> +           ereport(ERROR,
> +                   (errmsg("database \"%s\" has no actual collation version, but a version was specified",
> +                           name)));-
> 
> this means you can't connect on such a database anymore.  The level is probably
> ok for collation version check, but for db isn't that too much?

Right, changed to warning.

> +Oid
> +AlterDatabaseRefreshColl(AlterDatabaseRefreshCollStmt *stmt)
> +{
> +   Relation    rel;
> +   Oid         dboid;
> +   HeapTuple   tup;
> +   Datum       datum;
> +   bool        isnull;
> +   char       *oldversion;
> +   char       *newversion;
> +
> +   rel = table_open(DatabaseRelationId, RowExclusiveLock);
> +   dboid = get_database_oid(stmt->dbname, false);
> +
> +   if (!pg_database_ownercheck(dboid, GetUserId()))
> +       aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_DATABASE,
> +                      stmt->dbname);
> +
> +   tup = SearchSysCacheCopy1(DATABASEOID, ObjectIdGetDatum(dboid));
> +   if (!HeapTupleIsValid(tup))
> +       elog(ERROR, "cache lookup failed for database %u", dboid);
> 
> Is that ok to not obtain a lock on the database when refreshing the collation?

That code takes a RowExclusiveLock on pg_database.  Did you have 
something else in mind?

> +   /*
> +    * Check collation version.  See similar code in
> +    * pg_newlocale_from_collation().
> +    */
> +   datum = SysCacheGetAttr(DATABASEOID, tup, Anum_pg_database_datcollversion,
> +                           &isnull);
> +   if (!isnull)
> +   {
> 
> This (and pg_newlocale_from_collation()) reports a problem if a recorded
> collation version is found but there's no reported collation version.
> Shouldn't it also complain if it's the opposite?  It's otherwise a backdoor to
> make sure there won't be any check about the version anymore, and while it can
> probably happen if you mess with the catalogs it still doesn't look great.

get_collation_actual_version() always returns either null or not null 
for a given installation.  So the situation that the stored version is 
null and the actual version is not null can only happen as part of a 
software upgrade.  In that case, all uses of collations after an upgrade 
would immediately start complaining about missing versions, which seems 
like a bad experience.  Users can explicitly opt in to version tracking 
by running REFRESH VERSION once.

> +       /*
> +        * template0 shouldn't have any collation-dependent objects, so unset
> +        * the collation version.  This avoids warnings when making a new
> +        * database from it.
> +        */
> +       "UPDATE pg_database SET datcollversion = NULL WHERE datname = 'template0';\n\n",
> 
> I'm not opposed, but shouldn't there indeed be a warning in case of discrepancy
> in the source database (whether template or not)?
> 
> # update pg_database set datcollversion = 'meh' where datname in ('postgres', 'template1');
> UPDATE 2
> 
> # create database test1 template postgres;
> CREATE DATABASE
> 
> # create database test2 template template1;
> CREATE DATABASE
> 
> # \c test2
> WARNING:  database "test2" has a collation version mismatch

I don't understand what the complaint is here.  It seems to work ok?