Re: Rework of collation code, extensibility

On Sat, Dec 17, 2022 at 7:14 PM Jeff Davis <pgsql@j-davis.com> wrote:

> Attached is a new patch series. I think there are enough changes that
> this has become more of a "rework" of the collation code rather than
> just a refactoring. This is a continuation of some prior work[1][2] in
> a new thread given its new scope.
>
> Benefits:
>
> 1. Clearer division of responsibilities.
> 2. More consistent between libc and ICU providers.
> 3. Hooks that allow extensions to replace collation provider libraries.
> 4. New tests for the collation provider library hooks.
>
> There are a lot of changes, and still some loose ends, but I believe a
> few of these patches are close to ready.
>
> This set of changes does not express an opinion on how we might want to
> support multiple provider libraries in core; but whatever we choose, it
> should be easier to accomplish. Right now, the hooks have limited
> information on which to make the choice for a specific version of a
> collation provider library, but that's because there's limited
> information in the catalog. If the discussion here[3] concludes in
> adding collation provider library or library version information to the
> catalog, we can add additional parameters to the hooks.
>
> [1]
>
> https://postgr.es/m/99aa79cceefd1fe84fda23510494b8fbb7ad1e70.camel@j-davis.com
> [2]
>
> https://postgr.es/m/c4fda90ec6a7568a896f243a38eb273c3b5c3d93.camel@j-davis.com
> [3]
>
> https://postgr.es/m/CA+hUKGLEqMhnpZrgAcisoUeYFGz8W6EWdhtK2h-4QN0iOSFRqw@mail.gmail.com
>
>
> --
> Jeff Davis
> PostgreSQL Contributor Team - AWS
>
> Hi,
For pg_strxfrm_libc in v4-0002-Add-pg_strxfrm-and-pg_strxfrm_prefix.patch:

+#ifdef HAVE_LOCALE_T
+       if (locale)
+               return strxfrm_l(dest, src, destsize, locale->info.lt);
+       else
+#endif
+               return strxfrm(dest, src, destsize);

It seems the `else` is not needed (since when the if branch is taken, we
return from the func).

+       /* nul-terminate arguments */

nul-terminate -> null-terminate

For pg_strnxfrm(), I think `result` can be removed - we directly return the
result from pg_strnxfrm_libc or pg_strnxfrm_icu.

Cheers