On Fri, Feb 11, 2022 at 12:07:02PM +0100, Peter Eisentraut wrote:
> On 10.02.22 12:08, Julien Rouhaud wrote:
> > > + 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.
>
> I think accurate would be something like "objects using the default
> collation", since objects using a specific collation are not meant, even if
> they use the same provider.
Technically is the objects explicitly use the same collation as the default
collation they should be impacted the same way, but agreed.
> > > +/*
> > > + * 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.
>
> Hmm, I was looking at RenameDatabase() and AlterDatabaseOwner(), which
> return ObjectAddress.
Apparently I managed to only check AlterDatabase and AlterDatabaseSet, which
both return an Oid. Maybe we could also update those two to also return an
ObjectAddress, for consistency?