Re: ICU for global collation

On 15.08.22 14:06, Marina Polyakova wrote:
> 1.1) It looks like there's a bug in the function get_db_infos
> (src/bin/pg_upgrade/info.c), where the version of the old cluster is
> always checked:
>
> if (GET_MAJOR_VERSION(old_cluster.major_version) < 1500)
>     snprintf(query + strlen(query), sizeof(query) - strlen(query),
>              "'c' AS datlocprovider, NULL AS daticulocale, ");
> else
>     snprintf(query + strlen(query), sizeof(query) - strlen(query),
>              "datlocprovider, daticulocale, ");
>
> With the simple patch
>
> diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c
> index
> df374ce4b362b4c6c87fc1fd0e476e5d6d353d9e..53ea348e211d3ac38334292bc16cb814bc13bb87
> 100644
> --- a/src/bin/pg_upgrade/info.c
> +++ b/src/bin/pg_upgrade/info.c
> @@ -319,7 +319,7 @@ get_db_infos(ClusterInfo *cluster)
>
>      snprintf(query, sizeof(query),
>               "SELECT d.oid, d.datname, d.encoding, d.datcollate,
> d.datctype, ");
> -    if (GET_MAJOR_VERSION(old_cluster.major_version) < 1500)
> +    if (GET_MAJOR_VERSION(cluster->major_version) < 1500)
>          snprintf(query + strlen(query), sizeof(query) - strlen(query),
>                   "'c' AS datlocprovider, NULL AS daticulocale, ");
>      else

fixed

> 1.2) It looks like the mentioned asserion in dbcommands.c conflicts with
> the following lines earlier:
>
> if (dbiculocale == NULL)
>     dbiculocale = src_iculocale;

fixed

> I'm wondering if this is not a fully-supported feature (because createdb
> creates an SQL command with LC_COLLATE and LC_CTYPE options instead of
> LOCALE option) or is it a bug in CREATE DATABASE?.. From
> src/backend/commands/dbcommands.c:
>
> if (dblocprovider == COLLPROVIDER_ICU && !dbiculocale)
> {
>     if (dlocale && dlocale->arg)
>         dbiculocale = defGetString(dlocale);
> }

I think this piece of code was left over from some earlier attempts to
specify the libc locale and the icu locale with one option, which never
really worked well. The CREATE DATABASE man page does not mention that
LOCALE provides the default for ICU_LOCALE. Hence, I think we should
delete this.