Re: Database-level collation version tracking

On Thu, Feb 10, 2022 at 09:57:59AM +0100, Peter Eisentraut wrote:
> New patch that fixes all reported issues, I think:
> 
> - Added test for ALTER DATABASE / REFRESH COLLATION VERSION
> 
> - Rewrote AlterDatabaseRefreshCollVersion() with better locking
> 
> - Added version checking in createdb()

Thanks!  All issues are indeed fixed.  I just have a few additional comments:



> From 290ebb9ca743a2272181f435d5ea76d8a7280a0a Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter@eisentraut.org>
> Date: Thu, 10 Feb 2022 09:44:20 +0100
> Subject: [PATCH v4] Database-level collation version tracking



> +     * collation version was specified explicitly as an statement option; that

typo, should be "as a statement"

> +        actual_versionstr = get_collation_actual_version(COLLPROVIDER_LIBC, dbcollate);
> +        if (!actual_versionstr)
> +            ereport(ERROR,
> +                    (errmsg("template database \"%s\" has a collation version, but no actual collation version could
bedetermined",
 
> +                            dbtemplate)));
> +
> +        if (strcmp(actual_versionstr, src_collversion) != 0)
> +            ereport(ERROR,
> +                    (errmsg("template database \"%s\" has a collation version mismatch",
> +                            dbtemplate),
> +                     errdetail("The template database was created using collation version %s, "
> +                               "but the operating system provides version %s.",
> +                               src_collversion, actual_versionstr),
> +                     errhint("Rebuild all objects affected by collation in the template database and run "
> +                             "ALTER DATABASE %s REFRESH COLLATION VERSION, "
> +                             "or build PostgreSQL with the right library version.",
> +                             quote_identifier(dbtemplate))));

After a second read I think the messages are slightly ambiguous.  What do you
think about specifying the problematic collation name and provider?

For now we only support libc default collation so users will probably have to
reindex almost everything on that database (not sure if the versioning is more
fine grained on Windows), but we should probably still specify "affected by
libc collation" in the errhint so they have a chance to avoid unnecessary
reindex.

And this will hopefully become more important to have those information, when
ICU default collations will land :)

> +/*
> + * ALTER DATABASE name REFRESH COLLATION VERSION
> + */
> +ObjectAddress
> +AlterDatabaseRefreshColl(AlterDatabaseRefreshCollStmt *stmt)

I'm wondering why you changed this function to return an ObjectAddress rather
than an Oid?  There's no event trigger support for ALTER DATABASE, and the rest
of similar utility commands also returns Oid.

Other than that it all looks good to me!