Horton

Thread

Commits

  1. Tighten up make_libc_collator() and make_icu_collator().

  1. [18] Fix a few issues with the collation cache

    Jeff Davis <pgsql@j-davis.com> — 2024-08-08T19:24:28Z

    The collation cache, which maps collation oids to pg_locale_t objects,
has a few longstanding issues:

1. Using TopMemoryContext too much, with potential for leaks on error
paths.

2. Creating collators (UCollator for ICU or locale_t for libc) that can
leak on some error paths. For instance, the following will leak the
result of newlocale():

  create collation c2 (provider=libc,
    locale='C.utf8', version='bogus');

3. Not invalidating the cache. Collations don't change in a way that
matters during the lifetime of a backend, so the main problem is DROP
COLLATION. That can leave dangling entries in the cache until the
backend exits, and perhaps be a problem if there's OID wraparound.

The patches make use of resource owners for problems #2 and #3. There
aren't a lot of examples where resource owners are used in this way, so
I'd appreciate feedback on whether this is a reasonable thing to do or
not. Does it feel over-engineered? We can solve these problems by
rearranging the code to avoid problem #2 and iterating through the hash
table for problem #3, but using resource owners felt cleaner to me.

These problems exist in all supported branches, but the fixes are
somewhat invasive so I'm not inclined to backport them unless someone
thinks the problems are serious enough.

Regards,
	Jeff Davis

  2. Re: [18] Fix a few issues with the collation cache

    Jeff Davis <pgsql@j-davis.com> — 2024-08-14T23:30:23Z

    On Thu, 2024-08-08 at 12:24 -0700, Jeff Davis wrote:
> The collation cache, which maps collation oids to pg_locale_t
> objects,
> has a few longstanding issues:

Here's a patch set v2.

I changed it so that the pg_locale_t itself a resource kind, rather
than having separate locale_t and UCollator resource kinds. That
requires a bit more care to make sure that the pg_locale_t can be
initialized without leaking the locale_t or UCollator, but worked out
to be simpler overall.

A potential benefit of these changes is that, for eventual support of
multi-lib or an extension hook, improvements in the API here will make
things less error-prone.

Regards,
	Jeff Davis

  3. Re: [18] Fix a few issues with the collation cache

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

    On Wed, 2024-08-14 at 16:30 -0700, Jeff Davis wrote:
> On Thu, 2024-08-08 at 12:24 -0700, Jeff Davis wrote:
> > The collation cache, which maps collation oids to pg_locale_t
> > objects,
> > has a few longstanding issues:
> 
> Here's a patch set v2.

Updated and rebased.

Regards,
	Jeff Davis

  4. Re: [18] Fix a few issues with the collation cache

    Michael Paquier <michael@paquier.xyz> — 2024-12-10T06:44:06Z

    On Fri, Sep 20, 2024 at 05:28:48PM -0700, Jeff Davis wrote:
> Updated and rebased.

The patch has been failing to apply for a couple of weeks now.  Could
you rebase please?
--
Michael

  5. Re: [18] Fix a few issues with the collation cache

    Jeff Davis <pgsql@j-davis.com> — 2024-12-10T23:34:50Z

    On Tue, 2024-12-10 at 15:44 +0900, Michael Paquier wrote:
> On Fri, Sep 20, 2024 at 05:28:48PM -0700, Jeff Davis wrote:
> > Updated and rebased.
> 
> The patch has been failing to apply for a couple of weeks now.  Could
> you rebase please?

I committed some of the patches and fixed problem #1.

The way I used ResourceOwners to fix problems #2 and #3 is a bit
awkward. I'm not sure if it's worth the ceremony to try to avoid leaks
during OOM. And other paths that leak could be fixed more simply by
freeing it directly rather than relying on the resowner mechanism.
I think I'll withdraw this patch and submit a separate patch to do it
the simpler way.

Regards,
	Jeff Davis

  6. Re: [18] Fix a few issues with the collation cache

    Michael Paquier <michael@paquier.xyz> — 2024-12-10T23:45:23Z

    On Tue, Dec 10, 2024 at 03:34:50PM -0800, Jeff Davis wrote:
> I committed some of the patches and fixed problem #1.
> 
> The way I used ResourceOwners to fix problems #2 and #3 is a bit
> awkward. I'm not sure if it's worth the ceremony to try to avoid leaks
> during OOM. And other paths that leak could be fixed more simply by
> freeing it directly rather than relying on the resowner mechanism.
> I think I'll withdraw this patch and submit a separate patch to do it
> the simpler way.

Okay, thanks for the update.
--
Michael