Horton

Thread

Commits

  1. Don't accept length of -1 in pg_locale.h APIs.

  2. Allow length=-1 for NUL-terminated input to pg_strncoll(), etc.

  1. Refactor: allow pg_strncoll(), etc., to accept -1 length for NUL-terminated cstrings.

    Jeff Davis <pgsql@j-davis.com> — 2024-08-22T18:00:54Z

    Like ICU, allow -1 length to mean that the input string is NUL-
terminated for pg_strncoll(), pg_strnxfrm(), and pg_strnxfrm_prefix().

This simplifies the API and code a bit.

Along with some other refactoring in this area, we are getting close to
the point where the collation provider can just be a table of methods,
which means we can add an extension hook to provide a different method
table. That still requires more work, I'm just mentioning it here for
context.

Regards,
	Jeff Davis

  2. Re: Refactor: allow pg_strncoll(), etc., to accept -1 length for NUL-terminated cstrings.

    Jeff Davis <pgsql@j-davis.com> — 2024-09-21T00:28:51Z

    On Thu, 2024-08-22 at 11:00 -0700, Jeff Davis wrote:
> Like ICU, allow -1 length to mean that the input string is NUL-
> terminated for pg_strncoll(), pg_strnxfrm(), and
> pg_strnxfrm_prefix().

To better illustrate the direction I'm going, I roughly implemented
some patches that implement collation using a table of methods rather
than lots branching based on the provider.

This more cleanly separates the API for a provider, which will enable
us to use a hook to create a custom provider with arbitrary methods,
that may have nothing to do with ICU or libc. Or, we could go so far as
to implement a "CREATE LOCALE PROVIDER" that would provide the methods
using a handler function, and "datlocprovider" would be an OID rather
than a char.

From a practical perspective, I expect that extensions would use this
to lock down the version of a particular provider rather than implement
a completely arbitrary one. But the API is good for either case, and
offers quite a bit of code cleanup.

There are quite a few loose ends, of course:

  * There is still a lot of branching on the provider for DDL and
catalog access. I'm not sure if we will ever eliminate all of this, or
if we would even want to.

  * I haven't done anything with get_collation_actual_version().
Perhaps that should be a method, too, but it requires some extra
thought if we want this to be useful for "multilib" (having multiple
versions of a provider library at once).

  * I didn't add methods for formatting.c yet.

  * initdb -- should it offer a way to preload a library and then use
that for the provider?

  * I need to allow an arbitrary per-provider context, rather than the
current union designed for the existing providers.

Again, the patches are rough and there's a lot of code churn. I'd like
some feedback on whether people generally like the direction this is
going. If so I will clean up the patch series into smaller, more
reviewable chunks.

Regards,
	Jeff Davis

  3. Re: Refactor: allow pg_strncoll(), etc., to accept -1 length for NUL-terminated cstrings.

    Andres Freund <andres@anarazel.de> — 2026-05-01T16:40:54Z

    Hi,

On 2024-08-22 11:00:54 -0700, Jeff Davis wrote:
> Like ICU, allow -1 length to mean that the input string is NUL-
> terminated for pg_strncoll(), pg_strnxfrm(), and pg_strnxfrm_prefix().
> 
> This simplifies the API and code a bit.

I don't really like this.  I was hacking on a patch that uses compiler
annotations to tell the compiler what range of memory a function access. The
compiler then can use that knowledge to give you both compile-time warnings
and, more importantly, it makes ubsan much more accurate. It'll e.g. often be
able to warn you if a function accesses more memory than its annotation would
suggest, even if the memory is part of a larger memory allocation (something
asan, valgrind etc can't warn about, yet are often the most security critical
issues).  I found a bunch of issues that way already.

But the annotations can't work if the access size is sometimes is -1.

I also don't find this very convincing code-wise. You end up with lots of
branches for -1. You have to support cases where one of the arguments is
specifies as -1 and the other one with a real length, even though that's
presumably a non-existing case.

It seems reasonable to want the more efficient path for zero terminated
strings with libc, but it seems like if we want that, we should add add a
collate_method->strcoll, rather than have a strncoll that's not actually
strncoll but strcoll.

Greetings,

Andres Freund

  4. Re: Refactor: allow pg_strncoll(), etc., to accept -1 length for NUL-terminated cstrings.

    Jeff Davis <pgsql@j-davis.com> — 2026-05-05T20:23:12Z

    On Fri, 2026-05-01 at 12:40 -0400, Andres Freund wrote:
> Hi,
> 
> On 2024-08-22 11:00:54 -0700, Jeff Davis wrote:
> > Like ICU, allow -1 length to mean that the input string is NUL-
> > terminated for pg_strncoll(), pg_strnxfrm(), and
> > pg_strnxfrm_prefix().
> > 
> > This simplifies the API and code a bit.
> 
> I don't really like this.

Agreed. I did this to match up with the ICU API a bit better, but if
it's interfering with useful tools, then the special cases aren't worth
it.

Patch attached. It causes a bit of churn, so one disadvantage is that
it will complicate future backports in this area.

Regards,
	Jeff Davis

  5. Re: Refactor: allow pg_strncoll(), etc., to accept -1 length for NUL-terminated cstrings.

    Jeff Davis <pgsql@j-davis.com> — 2026-05-14T21:58:07Z

    On Tue, 2026-05-05 at 13:23 -0700, Jeff Davis wrote:
> Patch attached. It causes a bit of churn, so one disadvantage is that
> it will complicate future backports in this area.

I plan to commit this soon.

Regards,
	Jeff Davis

  6. Re: Refactor: allow pg_strncoll(), etc., to accept -1 length for NUL-terminated cstrings.

    Andres Freund <andres@anarazel.de> — 2026-05-14T22:17:17Z

    Hi,

On 2026-05-05 13:23:12 -0700, Jeff Davis wrote:
> Agreed. I did this to match up with the ICU API a bit better, but if
> it's interfering with useful tools, then the special cases aren't worth
> it.

> Patch attached.

Thanks!


> It causes a bit of churn, so one disadvantage is that it will complicate
> future backports in this area.

I think it's worth the gain in instrument-ability.  I also suspect it's good
for runtime performance, adding all those branches can't be particularly good.


Greetings,

Andres Freund