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
From 7a007d3573c6bda67729ab45bd58931bfcdaf702 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Tue, 6 Aug 2024 12:44:14 -0700
Subject: [PATCH v1 1/5] Minor refactor of collation cache.
Put collation cache logic in pg_newlocale_from_collation(), and
separate out the slow path for constructing a new pg_locale_t object.
---
src/backend/utils/adt/pg_locale.c | 286 ++++++++++++++----------------
1 file changed, 135 insertions(+), 151 deletions(-)
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index cd3661e7279..1560209ef21 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -1219,46 +1219,6 @@ IsoLocaleName(const char *winlocname)
#endif /* WIN32 && LC_MESSAGES */
-/*
- * Cache mechanism for collation information.
- *
- * Note that we currently lack any way to flush the cache. Since we don't
- * support ALTER COLLATION, this is OK. The worst case is that someone
- * drops a collation, and a useless cache entry hangs around in existing
- * backends.
- */
-static collation_cache_entry *
-lookup_collation_cache(Oid collation)
-{
- collation_cache_entry *cache_entry;
- bool found;
-
- Assert(OidIsValid(collation));
- Assert(collation != DEFAULT_COLLATION_OID);
-
- if (CollationCache == NULL)
- {
- CollationCacheContext = AllocSetContextCreate(TopMemoryContext,
- "collation cache",
- ALLOCSET_DEFAULT_SIZES);
- CollationCache = collation_cache_create(CollationCacheContext,
- 16, NULL);
- }
-
- cache_entry = collation_cache_insert(CollationCache, collation, &found);
- if (!found)
- {
- /*
- * Make sure cache entry is marked invalid, in case we fail before
- * setting things.
- */
- cache_entry->locale = 0;
- }
-
- return cache_entry;
-}
-
-
/*
* Detect whether collation's LC_COLLATE property is C
*/
@@ -1551,149 +1511,173 @@ init_database_collation(void)
}
/*
- * Create a pg_locale_t from a collation OID. Results are cached for the
- * lifetime of the backend. Thus, do not free the result with freelocale().
+ * Create pg_locale_t from collation oid in TopMemoryContext.
*
* For simplicity, we always generate COLLATE + CTYPE even though we
* might only need one of them. Since this is called only once per session,
* it shouldn't cost much.
*/
-pg_locale_t
-pg_newlocale_from_collation(Oid collid)
-{
- collation_cache_entry *cache_entry;
+static pg_locale_t
+init_pg_locale(Oid collid)
+{
+ /* We haven't computed this yet in this session, so do it */
+ HeapTuple tp;
+ Form_pg_collation collform;
+ struct pg_locale_struct result;
+ pg_locale_t resultp;
+ Datum datum;
+ bool isnull;
- /* Callers must pass a valid OID */
- Assert(OidIsValid(collid));
+ tp = SearchSysCache1(COLLOID, ObjectIdGetDatum(collid));
+ if (!HeapTupleIsValid(tp))
+ elog(ERROR, "cache lookup failed for collation %u", collid);
+ collform = (Form_pg_collation) GETSTRUCT(tp);
- if (collid == DEFAULT_COLLATION_OID)
- return &default_locale;
+ /* We'll fill in the result struct locally before allocating memory */
+ memset(&result, 0, sizeof(result));
+ result.provider = collform->collprovider;
+ result.deterministic = collform->collisdeterministic;
- cache_entry = lookup_collation_cache(collid);
+ if (collform->collprovider == COLLPROVIDER_BUILTIN)
+ {
+ const char *locstr;
+
+ datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_colllocale);
+ locstr = TextDatumGetCString(datum);
+
+ result.collate_is_c = true;
+ result.ctype_is_c = (strcmp(locstr, "C") == 0);
- if (cache_entry->locale == 0)
+ builtin_validate_locale(GetDatabaseEncoding(), locstr);
+
+ result.info.builtin.locale = MemoryContextStrdup(TopMemoryContext,
+ locstr);
+ }
+ else if (collform->collprovider == COLLPROVIDER_LIBC)
{
- /* We haven't computed this yet in this session, so do it */
- HeapTuple tp;
- Form_pg_collation collform;
- struct pg_locale_struct result;
- pg_locale_t resultp;
- Datum datum;
- bool isnull;
-
- tp = SearchSysCache1(COLLOID, ObjectIdGetDatum(collid));
- if (!HeapTupleIsValid(tp))
- elog(ERROR, "cache lookup failed for collation %u", collid);
- collform = (Form_pg_collation) GETSTRUCT(tp);
-
- /* We'll fill in the result struct locally before allocating memory */
- memset(&result, 0, sizeof(result));
- result.provider = collform->collprovider;
- result.deterministic = collform->collisdeterministic;
-
- if (collform->collprovider == COLLPROVIDER_BUILTIN)
- {
- const char *locstr;
+ const char *collcollate;
+ const char *collctype;
- datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_colllocale);
- locstr = TextDatumGetCString(datum);
+ datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_collcollate);
+ collcollate = TextDatumGetCString(datum);
+ datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_collctype);
+ collctype = TextDatumGetCString(datum);
- result.collate_is_c = true;
- result.ctype_is_c = (strcmp(locstr, "C") == 0);
+ result.collate_is_c = (strcmp(collcollate, "C") == 0) ||
+ (strcmp(collcollate, "POSIX") == 0);
+ result.ctype_is_c = (strcmp(collctype, "C") == 0) ||
+ (strcmp(collctype, "POSIX") == 0);
- builtin_validate_locale(GetDatabaseEncoding(), locstr);
+ make_libc_collator(collcollate, collctype, &result);
+ }
+ else if (collform->collprovider == COLLPROVIDER_ICU)
+ {
+ const char *iculocstr;
+ const char *icurules;
- result.info.builtin.locale = MemoryContextStrdup(TopMemoryContext,
- locstr);
- }
- else if (collform->collprovider == COLLPROVIDER_LIBC)
- {
- const char *collcollate;
- const char *collctype;
+ datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_colllocale);
+ iculocstr = TextDatumGetCString(datum);
- datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_collcollate);
- collcollate = TextDatumGetCString(datum);
- datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_collctype);
- collctype = TextDatumGetCString(datum);
+ result.collate_is_c = false;
+ result.ctype_is_c = false;
- result.collate_is_c = (strcmp(collcollate, "C") == 0) ||
- (strcmp(collcollate, "POSIX") == 0);
- result.ctype_is_c = (strcmp(collctype, "C") == 0) ||
- (strcmp(collctype, "POSIX") == 0);
+ datum = SysCacheGetAttr(COLLOID, tp, Anum_pg_collation_collicurules, &isnull);
+ if (!isnull)
+ icurules = TextDatumGetCString(datum);
+ else
+ icurules = NULL;
- make_libc_collator(collcollate, collctype, &result);
- }
- else if (collform->collprovider == COLLPROVIDER_ICU)
- {
- const char *iculocstr;
- const char *icurules;
+ make_icu_collator(iculocstr, icurules, &result);
+ }
- datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_colllocale);
- iculocstr = TextDatumGetCString(datum);
+ datum = SysCacheGetAttr(COLLOID, tp, Anum_pg_collation_collversion,
+ &isnull);
+ if (!isnull)
+ {
+ char *actual_versionstr;
+ char *collversionstr;
- result.collate_is_c = false;
- result.ctype_is_c = false;
+ collversionstr = TextDatumGetCString(datum);
- datum = SysCacheGetAttr(COLLOID, tp, Anum_pg_collation_collicurules, &isnull);
- if (!isnull)
- icurules = TextDatumGetCString(datum);
- else
- icurules = NULL;
+ if (collform->collprovider == COLLPROVIDER_LIBC)
+ datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_collcollate);
+ else
+ datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_colllocale);
- make_icu_collator(iculocstr, icurules, &result);
+ actual_versionstr = get_collation_actual_version(collform->collprovider,
+ TextDatumGetCString(datum));
+ if (!actual_versionstr)
+ {
+ /*
+ * This could happen when specifying a version in CREATE COLLATION
+ * but the provider does not support versioning, or manually
+ * creating a mess in the catalogs.
+ */
+ ereport(ERROR,
+ (errmsg("collation \"%s\" has no actual version, but a version was recorded",
+ NameStr(collform->collname))));
}
- datum = SysCacheGetAttr(COLLOID, tp, Anum_pg_collation_collversion,
- &isnull);
- if (!isnull)
- {
- char *actual_versionstr;
- char *collversionstr;
+ if (strcmp(actual_versionstr, collversionstr) != 0)
+ ereport(WARNING,
+ (errmsg("collation \"%s\" has version mismatch",
+ NameStr(collform->collname)),
+ errdetail("The collation in the database was created using version %s, "
+ "but the operating system provides version %s.",
+ collversionstr, actual_versionstr),
+ errhint("Rebuild all objects affected by this collation and run "
+ "ALTER COLLATION %s REFRESH VERSION, "
+ "or build PostgreSQL with the right library version.",
+ quote_qualified_identifier(get_namespace_name(collform->collnamespace),
+ NameStr(collform->collname)))));
+ }
- collversionstr = TextDatumGetCString(datum);
+ ReleaseSysCache(tp);
- if (collform->collprovider == COLLPROVIDER_LIBC)
- datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_collcollate);
- else
- datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_colllocale);
+ /* We'll keep the pg_locale_t structures in TopMemoryContext */
+ resultp = MemoryContextAlloc(TopMemoryContext, sizeof(*resultp));
+ *resultp = result;
- actual_versionstr = get_collation_actual_version(collform->collprovider,
- TextDatumGetCString(datum));
- if (!actual_versionstr)
- {
- /*
- * This could happen when specifying a version in CREATE
- * COLLATION but the provider does not support versioning, or
- * manually creating a mess in the catalogs.
- */
- ereport(ERROR,
- (errmsg("collation \"%s\" has no actual version, but a version was recorded",
- NameStr(collform->collname))));
- }
+ return resultp;
+}
- if (strcmp(actual_versionstr, collversionstr) != 0)
- ereport(WARNING,
- (errmsg("collation \"%s\" has version mismatch",
- NameStr(collform->collname)),
- errdetail("The collation in the database was created using version %s, "
- "but the operating system provides version %s.",
- collversionstr, actual_versionstr),
- errhint("Rebuild all objects affected by this collation and run "
- "ALTER COLLATION %s REFRESH VERSION, "
- "or build PostgreSQL with the right library version.",
- quote_qualified_identifier(get_namespace_name(collform->collnamespace),
- NameStr(collform->collname)))));
- }
+/*
+ * Retrieve a pg_locale_t from a collation OID. Results are cached for the
+ * lifetime of the backend, thus do not free the result.
+ */
+pg_locale_t
+pg_newlocale_from_collation(Oid collid)
+{
+ collation_cache_entry *cache_entry;
+ bool found;
- ReleaseSysCache(tp);
+ /* Callers must pass a valid OID */
+ Assert(OidIsValid(collid));
- /* We'll keep the pg_locale_t structures in TopMemoryContext */
- resultp = MemoryContextAlloc(TopMemoryContext, sizeof(*resultp));
- *resultp = result;
+ if (collid == DEFAULT_COLLATION_OID)
+ return &default_locale;
- cache_entry->locale = resultp;
+ /*
+ * Cache mechanism for collation information.
+ *
+ * Note that we currently lack any way to flush the cache. Since we don't
+ * support ALTER COLLATION, this is OK. The worst case is that someone
+ * drops a collation, and a useless cache entry hangs around in existing
+ * backends.
+ */
+ if (CollationCache == NULL)
+ {
+ CollationCacheContext = AllocSetContextCreate(TopMemoryContext,
+ "collation cache",
+ ALLOCSET_DEFAULT_SIZES);
+ CollationCache = collation_cache_create(CollationCacheContext,
+ 16, NULL);
}
+ cache_entry = collation_cache_insert(CollationCache, collid, &found);
+ if (!found || !cache_entry->locale)
+ cache_entry->locale = init_pg_locale(collid);
+
return cache_entry->locale;
}
--
2.34.1
From 06e1cb3aec40655976fa35e17f055a3ce2057778 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Wed, 7 Aug 2024 11:05:46 -0700
Subject: [PATCH v1 2/5] Tighten up make_libc_collator() and
make_icu_collator().
Return the result rather than using an out parameter, and make it the
caller's responsibility to copy it into the right context. Ensure that
no paths leak a collator.
The function make_icu_collator() doesn't have any external callers, so
change it to be static. Also, when re-opening with rules, close the
previous collator before there's a chance for errors.
In make_libc_collator(), if the first newlocale() succeeds and the
second one fails, close the first locale_t object.
---
src/backend/utils/adt/pg_locale.c | 75 +++++++++++++++++++------------
src/include/utils/pg_locale.h | 4 --
2 files changed, 47 insertions(+), 32 deletions(-)
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index 1560209ef21..20fa4ebe2b2 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -1302,14 +1302,15 @@ report_newlocale_failure(const char *localename)
}
/*
- * Initialize the locale_t field.
+ * Create a locale_t with the given collation and ctype.
*
- * The "C" and "POSIX" locales are not actually handled by libc, so set the
- * locale_t to zero in that case.
+ * The "C" and "POSIX" locales are not actually handled by libc, so return
+ * NULL.
+ *
+ * Ensure that no path leaks a locale_t.
*/
-static void
-make_libc_collator(const char *collate, const char *ctype,
- pg_locale_t result)
+static locale_t
+make_libc_collator(const char *collate, const char *ctype)
{
locale_t loc = 0;
@@ -1348,7 +1349,11 @@ make_libc_collator(const char *collate, const char *ctype,
errno = 0;
loc = newlocale(LC_CTYPE_MASK, ctype, loc1);
if (!loc)
+ {
+ if (loc1)
+ freelocale(loc1);
report_newlocale_failure(ctype);
+ }
}
else
loc = loc1;
@@ -1365,15 +1370,18 @@ make_libc_collator(const char *collate, const char *ctype,
#endif
}
- result->info.lt = loc;
+ return loc;
}
-void
-make_icu_collator(const char *iculocstr,
- const char *icurules,
- struct pg_locale_struct *resultp)
-{
+/*
+ * Create a UCollator with the given locale string and rules.
+ *
+ * Ensure that no path leaks a UCollator.
+ */
#ifdef USE_ICU
+static UCollator *
+make_icu_collator(const char *iculocstr, const char *icurules)
+{
UCollator *collator;
collator = pg_ucol_open(iculocstr);
@@ -1391,14 +1399,14 @@ make_icu_collator(const char *iculocstr,
int32_t length;
default_rules = ucol_getRules(collator, &length);
+ ucol_close(collator);
+
icu_to_uchar(&my_rules, icurules, strlen(icurules));
agg_rules = palloc_array(UChar, u_strlen(default_rules) + u_strlen(my_rules) + 1);
u_strcpy(agg_rules, default_rules);
u_strcat(agg_rules, my_rules);
- ucol_close(collator);
-
status = U_ZERO_ERROR;
collator = ucol_openRules(agg_rules, u_strlen(agg_rules),
UCOL_DEFAULT, UCOL_DEFAULT_STRENGTH, NULL, &status);
@@ -1409,16 +1417,9 @@ make_icu_collator(const char *iculocstr,
iculocstr, icurules, u_errorName(status))));
}
- /* We will leak this string if the caller errors later :-( */
- resultp->info.icu.locale = MemoryContextStrdup(TopMemoryContext, iculocstr);
- resultp->info.icu.ucol = collator;
-#else /* not USE_ICU */
- /* could get here if a collation was created by a build with ICU */
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("ICU is not supported in this build")));
-#endif /* not USE_ICU */
+ return collator;
}
+#endif /* not USE_ICU */
bool
@@ -1436,7 +1437,6 @@ init_database_collation(void)
HeapTuple tup;
Form_pg_database dbform;
Datum datum;
- bool isnull;
/* Fetch our pg_database row normally, via syscache */
tup = SearchSysCache1(DATABASEOID, ObjectIdGetDatum(MyDatabaseId));
@@ -1461,8 +1461,10 @@ init_database_collation(void)
}
else if (dbform->datlocprovider == COLLPROVIDER_ICU)
{
+#ifdef USE_ICU
char *datlocale;
char *icurules;
+ bool isnull;
datum = SysCacheGetAttrNotNull(DATABASEOID, tup, Anum_pg_database_datlocale);
datlocale = TextDatumGetCString(datum);
@@ -1476,7 +1478,14 @@ init_database_collation(void)
else
icurules = NULL;
- make_icu_collator(datlocale, icurules, &default_locale);
+ default_locale.info.icu.ucol = make_icu_collator(datlocale, icurules);
+ default_locale.info.icu.locale = MemoryContextStrdup(TopMemoryContext, datlocale);
+#else /* not USE_ICU */
+ /* could get here if a collation was created by a build with ICU */
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ICU is not supported in this build")));
+#endif /* not USE_ICU */
}
else
{
@@ -1495,7 +1504,7 @@ init_database_collation(void)
default_locale.ctype_is_c = (strcmp(datctype, "C") == 0) ||
(strcmp(datctype, "POSIX") == 0);
- make_libc_collator(datcollate, datctype, &default_locale);
+ default_locale.info.lt = make_libc_collator(datcollate, datctype);
}
default_locale.provider = dbform->datlocprovider;
@@ -1568,10 +1577,11 @@ init_pg_locale(Oid collid)
result.ctype_is_c = (strcmp(collctype, "C") == 0) ||
(strcmp(collctype, "POSIX") == 0);
- make_libc_collator(collcollate, collctype, &result);
+ result.info.lt = make_libc_collator(collcollate, collctype);
}
else if (collform->collprovider == COLLPROVIDER_ICU)
{
+#ifdef USE_ICU
const char *iculocstr;
const char *icurules;
@@ -1587,7 +1597,14 @@ init_pg_locale(Oid collid)
else
icurules = NULL;
- make_icu_collator(iculocstr, icurules, &result);
+ result.info.icu.ucol = make_icu_collator(iculocstr, icurules);
+ result.info.icu.locale = MemoryContextStrdup(TopMemoryContext, iculocstr);
+#else /* not USE_ICU */
+ /* could get here if a collation was created by a build with ICU */
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ICU is not supported in this build")));
+#endif /* not USE_ICU */
}
datum = SysCacheGetAttr(COLLOID, tp, Anum_pg_collation_collversion,
@@ -2530,6 +2547,8 @@ builtin_validate_locale(int encoding, const char *locale)
/*
* Wrapper around ucol_open() to handle API differences for older ICU
* versions.
+ *
+ * Ensure that no path leaks a UCollator.
*/
static UCollator *
pg_ucol_open(const char *loc_str)
diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h
index f41d33975be..1ca0b7fa74b 100644
--- a/src/include/utils/pg_locale.h
+++ b/src/include/utils/pg_locale.h
@@ -107,10 +107,6 @@ struct pg_locale_struct
typedef struct pg_locale_struct *pg_locale_t;
-extern void make_icu_collator(const char *iculocstr,
- const char *icurules,
- struct pg_locale_struct *resultp);
-
extern bool pg_locale_deterministic(pg_locale_t locale);
extern void init_database_collation(void);
extern pg_locale_t pg_newlocale_from_collation(Oid collid);
--
2.34.1
From e3985ae3574655878ae53f63390a2f4c7167fffc Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Wed, 7 Aug 2024 11:31:30 -0700
Subject: [PATCH v1 3/5] For collation cache, use CollationCacheContext for all
memory.
Commit 005c6b833f introduced CollationCacheContext for the hash table,
but the pg_locale_t objects were still allocated in
TopMemoryContext. Change to use CollationCacheContext.
This change required a bit of refactoring to make the caller of
init_pg_locale() responsible for copying to the right context, so that
init_database_collation() can still use TopMemoryContext.
---
src/backend/utils/adt/pg_locale.c | 79 ++++++++++++++++++++-----------
1 file changed, 52 insertions(+), 27 deletions(-)
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index 20fa4ebe2b2..59fc99bd12b 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -1520,11 +1520,11 @@ init_database_collation(void)
}
/*
- * Create pg_locale_t from collation oid in TopMemoryContext.
+ * Create pg_locale_t from collation oid.
*
- * For simplicity, we always generate COLLATE + CTYPE even though we
- * might only need one of them. Since this is called only once per session,
- * it shouldn't cost much.
+ * For simplicity, we always generate COLLATE + CTYPE even though we might
+ * only need one of them. Since this is called only once per session, it
+ * shouldn't cost much.
*/
static pg_locale_t
init_pg_locale(Oid collid)
@@ -1532,8 +1532,7 @@ init_pg_locale(Oid collid)
/* We haven't computed this yet in this session, so do it */
HeapTuple tp;
Form_pg_collation collform;
- struct pg_locale_struct result;
- pg_locale_t resultp;
+ pg_locale_t result;
Datum datum;
bool isnull;
@@ -1542,10 +1541,9 @@ init_pg_locale(Oid collid)
elog(ERROR, "cache lookup failed for collation %u", collid);
collform = (Form_pg_collation) GETSTRUCT(tp);
- /* We'll fill in the result struct locally before allocating memory */
- memset(&result, 0, sizeof(result));
- result.provider = collform->collprovider;
- result.deterministic = collform->collisdeterministic;
+ result = palloc0_object(struct pg_locale_struct);
+ result->provider = collform->collprovider;
+ result->deterministic = collform->collisdeterministic;
if (collform->collprovider == COLLPROVIDER_BUILTIN)
{
@@ -1554,13 +1552,12 @@ init_pg_locale(Oid collid)
datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_colllocale);
locstr = TextDatumGetCString(datum);
- result.collate_is_c = true;
- result.ctype_is_c = (strcmp(locstr, "C") == 0);
+ result->collate_is_c = true;
+ result->ctype_is_c = (strcmp(locstr, "C") == 0);
builtin_validate_locale(GetDatabaseEncoding(), locstr);
- result.info.builtin.locale = MemoryContextStrdup(TopMemoryContext,
- locstr);
+ result->info.builtin.locale = pstrdup(locstr);
}
else if (collform->collprovider == COLLPROVIDER_LIBC)
{
@@ -1572,12 +1569,12 @@ init_pg_locale(Oid collid)
datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_collctype);
collctype = TextDatumGetCString(datum);
- result.collate_is_c = (strcmp(collcollate, "C") == 0) ||
+ result->collate_is_c = (strcmp(collcollate, "C") == 0) ||
(strcmp(collcollate, "POSIX") == 0);
- result.ctype_is_c = (strcmp(collctype, "C") == 0) ||
+ result->ctype_is_c = (strcmp(collctype, "C") == 0) ||
(strcmp(collctype, "POSIX") == 0);
- result.info.lt = make_libc_collator(collcollate, collctype);
+ result->info.lt = make_libc_collator(collcollate, collctype);
}
else if (collform->collprovider == COLLPROVIDER_ICU)
{
@@ -1588,8 +1585,8 @@ init_pg_locale(Oid collid)
datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_colllocale);
iculocstr = TextDatumGetCString(datum);
- result.collate_is_c = false;
- result.ctype_is_c = false;
+ result->collate_is_c = false;
+ result->ctype_is_c = false;
datum = SysCacheGetAttr(COLLOID, tp, Anum_pg_collation_collicurules, &isnull);
if (!isnull)
@@ -1597,8 +1594,8 @@ init_pg_locale(Oid collid)
else
icurules = NULL;
- result.info.icu.ucol = make_icu_collator(iculocstr, icurules);
- result.info.icu.locale = MemoryContextStrdup(TopMemoryContext, iculocstr);
+ result->info.icu.ucol = make_icu_collator(iculocstr, icurules);
+ result->info.icu.locale = pstrdup(iculocstr);
#else /* not USE_ICU */
/* could get here if a collation was created by a build with ICU */
ereport(ERROR,
@@ -1651,11 +1648,7 @@ init_pg_locale(Oid collid)
ReleaseSysCache(tp);
- /* We'll keep the pg_locale_t structures in TopMemoryContext */
- resultp = MemoryContextAlloc(TopMemoryContext, sizeof(*resultp));
- *resultp = result;
-
- return resultp;
+ return result;
}
/*
@@ -1693,7 +1686,39 @@ pg_newlocale_from_collation(Oid collid)
cache_entry = collation_cache_insert(CollationCache, collid, &found);
if (!found || !cache_entry->locale)
- cache_entry->locale = init_pg_locale(collid);
+ {
+ MemoryContext oldcontext;
+ pg_locale_t tmplocale;
+ pg_locale_t locale;
+
+ tmplocale = init_pg_locale(collid);
+
+ oldcontext = MemoryContextSwitchTo(CollationCacheContext);
+
+ locale = palloc0_object(struct pg_locale_struct);
+ *locale = *tmplocale;
+
+ /* deep copy */
+ if (locale->provider == COLLPROVIDER_BUILTIN)
+ {
+ locale->info.builtin.locale = pstrdup(locale->info.builtin.locale);
+ }
+#ifdef USE_ICU
+ else if (locale->provider == COLLPROVIDER_ICU)
+ {
+ locale->info.icu.locale = pstrdup(locale->info.icu.locale);
+ }
+#endif
+ else if (locale->provider != COLLPROVIDER_LIBC)
+ {
+ /* shouldn't happen */
+ PGLOCALE_SUPPORT_ERROR(locale->provider);
+ }
+
+ MemoryContextSwitchTo(oldcontext);
+
+ cache_entry->locale = locale;
+ }
return cache_entry->locale;
}
--
2.34.1
From 9ef14874e3ea19edbed8c7aa3d855424ea330937 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Wed, 7 Aug 2024 12:25:37 -0700
Subject: [PATCH v1 4/5] Use resource owners to track locale_t and ICU collator
objects.
Rather than rely on careful ordering of operations in error paths,
track collator objects with CurrentResourceOwner soon after they are
allocated. Then, move them to the new CollationCacheOwner when the
pg_locale_t is successfully allocated, right after copying the memory
to CollationCacheContext.
---
src/backend/utils/adt/pg_locale.c | 94 ++++++++++++++++++++++++++++++-
1 file changed, 91 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index 59fc99bd12b..2aaf11af09b 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -66,6 +66,7 @@
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/pg_locale.h"
+#include "utils/resowner.h"
#include "utils/syscache.h"
#ifdef USE_ICU
@@ -151,6 +152,16 @@ typedef struct
static MemoryContext CollationCacheContext = NULL;
static collation_cache_hash *CollationCache = NULL;
+/*
+ * Collator objects (UCollator for ICU or locale_t for libc) are allocated in
+ * an external library, so track them using a resource owner. While still
+ * initializing the pg_locale_t structure, the collators are owned by
+ * CurrentResourceOwner, so that they are released if an error is encountered
+ * partway through. After the pg_locale_t is fully initialized, the collators
+ * are transferred to CollationCacheOwner.
+ */
+static ResourceOwner CollationCacheOwner = NULL;
+
#if defined(WIN32) && defined(LC_MESSAGES)
static char *IsoLocaleName(const char *);
#endif
@@ -174,6 +185,51 @@ static void icu_set_collation_attributes(UCollator *collator, const char *loc,
UErrorCode *status);
#endif
+static void ResOwnerReleaseLocaleT(Datum val);
+
+#ifdef USE_ICU
+static void ResOwnerReleaseICUCollator(Datum val);
+
+static const ResourceOwnerDesc ICUCollatorResourceKind =
+{
+ .name = "ICU collator reference",
+ .release_phase = RESOURCE_RELEASE_AFTER_LOCKS,
+ .release_priority = RELEASE_PRIO_LAST,
+ .ReleaseResource = ResOwnerReleaseICUCollator,
+ .DebugPrint = NULL /* the default message is fine */
+};
+
+static void
+ResOwnerReleaseICUCollator(Datum val)
+{
+ UCollator *ucol = (UCollator *) DatumGetPointer(val);
+
+ ucol_close(ucol);
+}
+
+#endif
+
+static const ResourceOwnerDesc LocaleTResourceKind =
+{
+ .name = "locale_t reference",
+ .release_phase = RESOURCE_RELEASE_AFTER_LOCKS,
+ .release_priority = RELEASE_PRIO_LAST,
+ .ReleaseResource = ResOwnerReleaseLocaleT,
+ .DebugPrint = NULL /* the default message is fine */
+};
+
+static void
+ResOwnerReleaseLocaleT(Datum val)
+{
+ locale_t loc = (locale_t) DatumGetPointer(val);
+
+#ifndef WIN32
+ freelocale(loc);
+#else
+ _free_locale(loc);
+#endif
+}
+
/*
* POSIX doesn't define _l-variants of these functions, but several systems
* have them. We provide our own replacements here.
@@ -1574,7 +1630,12 @@ init_pg_locale(Oid collid)
result->ctype_is_c = (strcmp(collctype, "C") == 0) ||
(strcmp(collctype, "POSIX") == 0);
+ ResourceOwnerEnlarge(CurrentResourceOwner);
result->info.lt = make_libc_collator(collcollate, collctype);
+ if (result->info.lt != NULL)
+ ResourceOwnerRemember(CurrentResourceOwner,
+ PointerGetDatum(result->info.lt),
+ &LocaleTResourceKind);
}
else if (collform->collprovider == COLLPROVIDER_ICU)
{
@@ -1594,7 +1655,11 @@ init_pg_locale(Oid collid)
else
icurules = NULL;
+ ResourceOwnerEnlarge(CurrentResourceOwner);
result->info.icu.ucol = make_icu_collator(iculocstr, icurules);
+ Assert(result->info.icu.ucol != NULL);
+ ResourceOwnerRemember(CurrentResourceOwner, PointerGetDatum(result->info.icu.ucol),
+ &ICUCollatorResourceKind);
result->info.icu.locale = pstrdup(iculocstr);
#else /* not USE_ICU */
/* could get here if a collation was created by a build with ICU */
@@ -1677,6 +1742,7 @@ pg_newlocale_from_collation(Oid collid)
*/
if (CollationCache == NULL)
{
+ CollationCacheOwner = ResourceOwnerCreate(NULL, "collation cache");
CollationCacheContext = AllocSetContextCreate(TopMemoryContext,
"collation cache",
ALLOCSET_DEFAULT_SIZES);
@@ -1691,14 +1757,20 @@ pg_newlocale_from_collation(Oid collid)
pg_locale_t tmplocale;
pg_locale_t locale;
+ ResourceOwnerEnlarge(CollationCacheOwner);
+
tmplocale = init_pg_locale(collid);
+ /*
+ * Deep copy the memory into CollationCacheContext and reassign the
+ * collators to CollationCacheOwner.
+ */
+
oldcontext = MemoryContextSwitchTo(CollationCacheContext);
locale = palloc0_object(struct pg_locale_struct);
*locale = *tmplocale;
- /* deep copy */
if (locale->provider == COLLPROVIDER_BUILTIN)
{
locale->info.builtin.locale = pstrdup(locale->info.builtin.locale);
@@ -1707,13 +1779,29 @@ pg_newlocale_from_collation(Oid collid)
else if (locale->provider == COLLPROVIDER_ICU)
{
locale->info.icu.locale = pstrdup(locale->info.icu.locale);
+ Assert(locale->info.icu.ucol != NULL);
+ ResourceOwnerForget(CurrentResourceOwner,
+ PointerGetDatum(locale->info.icu.ucol),
+ &ICUCollatorResourceKind);
+ ResourceOwnerRemember(CollationCacheOwner,
+ PointerGetDatum(locale->info.icu.ucol),
+ &ICUCollatorResourceKind);
}
#endif
- else if (locale->provider != COLLPROVIDER_LIBC)
+ else if (locale->provider == COLLPROVIDER_LIBC)
{
+ /* may be NULL if locale is "C" or "POSIX" */
+ if (locale->info.lt != NULL)
+ {
+ ResourceOwnerForget(CurrentResourceOwner, PointerGetDatum(locale->info.lt),
+ &LocaleTResourceKind);
+ ResourceOwnerRemember(CollationCacheOwner, PointerGetDatum(locale->info.lt),
+ &LocaleTResourceKind);
+ }
+ }
+ else
/* shouldn't happen */
PGLOCALE_SUPPORT_ERROR(locale->provider);
- }
MemoryContextSwitchTo(oldcontext);
--
2.34.1
From 7e04f81d825bcddf2c5ec3e7a156f30c8d1b67eb Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Wed, 7 Aug 2024 13:03:32 -0700
Subject: [PATCH v1 5/5] Invalidate collation cache when appropriate.
Previously, DROP COLLATION could leave a cache entry around. That's
not normally a problem, but can be if oid wraparound causes the same
oid to be reused for a different collation.
---
src/backend/utils/adt/pg_locale.c | 36 ++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 10 deletions(-)
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index 2aaf11af09b..7c5abb5eca3 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -63,6 +63,7 @@
#include "utils/builtins.h"
#include "utils/formatting.h"
#include "utils/guc_hooks.h"
+#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/pg_locale.h"
@@ -158,7 +159,8 @@ static collation_cache_hash *CollationCache = NULL;
* initializing the pg_locale_t structure, the collators are owned by
* CurrentResourceOwner, so that they are released if an error is encountered
* partway through. After the pg_locale_t is fully initialized, the collators
- * are transferred to CollationCacheOwner.
+ * are transferred to CollationCacheOwner, which lasts until the next cache
+ * invalidation.
*/
static ResourceOwner CollationCacheOwner = NULL;
@@ -1484,6 +1486,23 @@ pg_locale_deterministic(pg_locale_t locale)
return locale->deterministic;
}
+static void
+CollationCacheInvalidate(Datum arg, int cacheid, uint32 hashvalue)
+{
+ /* free all memory and reset hash table */
+ MemoryContextReset(CollationCacheContext);
+ CollationCache = collation_cache_create(CollationCacheContext,
+ 16, NULL);
+
+ /* release ICU collator and locale_t objects */
+#ifdef USE_ICU
+ ResourceOwnerReleaseAllOfKind(CollationCacheOwner,
+ &ICUCollatorResourceKind);
+#endif
+ ResourceOwnerReleaseAllOfKind(CollationCacheOwner,
+ &LocaleTResourceKind);
+}
+
/*
* Initialize default_locale with database locale settings.
*/
@@ -1732,14 +1751,7 @@ pg_newlocale_from_collation(Oid collid)
if (collid == DEFAULT_COLLATION_OID)
return &default_locale;
- /*
- * Cache mechanism for collation information.
- *
- * Note that we currently lack any way to flush the cache. Since we don't
- * support ALTER COLLATION, this is OK. The worst case is that someone
- * drops a collation, and a useless cache entry hangs around in existing
- * backends.
- */
+ /* cache mechanism for collation information */
if (CollationCache == NULL)
{
CollationCacheOwner = ResourceOwnerCreate(NULL, "collation cache");
@@ -1748,6 +1760,9 @@ pg_newlocale_from_collation(Oid collid)
ALLOCSET_DEFAULT_SIZES);
CollationCache = collation_cache_create(CollationCacheContext,
16, NULL);
+ CacheRegisterSyscacheCallback(COLLOID,
+ CollationCacheInvalidate,
+ (Datum) 0);
}
cache_entry = collation_cache_insert(CollationCache, collid, &found);
@@ -1763,7 +1778,8 @@ pg_newlocale_from_collation(Oid collid)
/*
* Deep copy the memory into CollationCacheContext and reassign the
- * collators to CollationCacheOwner.
+ * collators to CollationCacheOwner, which last until the next cache
+ * invalidation.
*/
oldcontext = MemoryContextSwitchTo(CollationCacheContext);
--
2.34.1
