Re: Database-level collation version tracking

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?