Re: Collation version tracking for macOS

On Sat, 2022-11-26 at 18:27 +1300, Thomas Munro wrote:
> On Thu, Nov 24, 2022 at 5:48 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
> wrote:
> > On Thu, Nov 24, 2022 at 3:07 PM Jeff Davis <pgsql(at)j-davis(dot)com>
> > wrote:
> > > I'd vote for 1 on the grounds that it's easier to document and
> > > understand a single collation version, which comes straight from
> > > ucol_getVersion(). This approach makes it a separate problem to
> > > find
> > > the collation version among whatever libraries the admin can
> > > provide;
> > > but adding some observability into the search should mitigate any
> > > confusion.
> >
> > OK, it sounds like I should code that up next.
>
> Here's the first iteration.

Thank you.

Proposed changes:

* I attached a first pass of some documentation.

* Should be another GUC to turn WARNING into an ERROR. Useful at least
for testing; perhaps too dangerous for production.

* The libraries should be loaded in a more diliberate order. The "*"
should be expanded in a descending fashion so that later versions are
preferred.

* GUCs should be validated.

* Should validate that loaded library has expected version.

* We need to revise or remove pg_collation_actual_version() and
pg_database_collation_actual_version().

* The GUCs are PGC_SUSET, but don't take effect because
icu_library_list_fully_loaded is never reset.

* The extra collations you're adding at bootstrap time are named based
on the library major version. I suppose it might be more "proper" to
name them based on the collation version, but that would be more
verbose, so I won't advocate for that. Just pointing it out.

* It looks hard (or impossible) to mix multiple ICU libraries with the
same major version and different minor versions. That's because,
e.g., libicui18n.so.63.1 links against libicuuc.63 and libicudata.63,
and when you install ICU 63.2, those dependencies get clobbered with
the 63.2 versions. That fails the sanity check I proposed above about
the library version number matching the requested library version
number. And it also just seems wrong -- why would you have minor-
version precision about an ICU library but then only major-version
precision about the ICU dependencies of that library? Doesn't that
defeat the whole purpose of this naming scheme? (Maybe another ICU
bug?).

Minor comments:

* ICU_I18N is defined in make_icu_library_name() but used outside of
it. One solution might be to have it return both library names to the
caller and rename it as make_icu_library_names().

* get_icu_function() could use a clarifying comment or a better name.
Something that communicates that you are looking for the function in
the given library with the given major version number (which may or may
not be needed depending on how the library was compiled).

* typo in comment over make_icu_collator:
s/u_getVersion/ucol_getVersion/

* The return value of make_icu_collator() seems backwards to me,
stylistically. I typically see the false-is-good pattern with integer
returns.

* weird bracketing style in get_icu_collator for the "else"

>   The version rosetta stone functions look like this:
>
> postgres=# select * from pg_icu_library_versions();
>  icu_version | unicode_version | cldr_version
> -------------+-----------------+--------------
>  67.1        | 13.0            | 37.0
>  63.1        | 11.0            | 34.0
>  57.1        | 8.0             | 29.0
> (3 rows)
>
> postgres=# select * from pg_icu_collation_versions('zh');
>  icu_version | uca_version | collator_version
> -------------+-------------+------------------
>  67.1        | 13.0        | 153.14.37
>  63.1        | 11.0        | 153.88.34
>  57.1        | 8.0         | 153.64.29
> (3 rows)

I like these functions.

--
Jeff Davis
PostgreSQL Contributor Team - AWS