Thread: [18] Fix a few issues with the collation cache

[18] Fix a few issues with the collation cache

From
Jeff Davis
Date:
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


Attachment

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

From
Jeff Davis
Date:
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