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
From 7c5390d9cea9b8b3abb94db26337e10c60fe3756 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 v2 1/6] 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.
Discussion: https://postgr.es/m/54d20e812bd6c3e44c10eddcd757ec494ebf1803.ca...@j-davis.com
---
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..1668236e779 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
+create_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 = create_pg_locale(collid);
+
return cache_entry->locale;
}
--
2.34.1
From a8da929f5961c169b359ad93f31f59b8baac00c8 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 v2 2/6] 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 1668236e779..5a3dccf757b 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 @@ create_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 @@ create_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 c0671c641af3399bfc7a809153af9fa40ed166b2 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Tue, 13 Aug 2024 13:24:07 -0700
Subject: [PATCH v2 3/6] Refactor collation version check into new function.
Will make it easier, in an upcoming commit, to avoid a resource leak
if the version check raises an ERROR. Also adds a nice place for a
comment.
---
src/backend/utils/adt/pg_locale.c | 124 +++++++++++++++++++-----------
1 file changed, 80 insertions(+), 44 deletions(-)
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index 5a3dccf757b..db9d1c01947 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -1519,12 +1519,84 @@ init_database_collation(void)
ReleaseSysCache(tup);
}
+/*
+ * Check that the collation version in the catalog (from the time the
+ * collation was last created or refreshed) matches the version currently
+ * reported by the collation provider.
+ *
+ * A mismatch can indicate that the provider has been updated, which can lead
+ * to index inconsistencies or other problems.
+ */
+static void
+check_collation_version(Oid collid)
+{
+ /* We haven't computed this yet in this session, so do it */
+ HeapTuple tp;
+ Form_pg_collation collform;
+ Datum datum;
+ bool isnull;
+ char *actual_versionstr;
+ char *collversionstr;
+
+ tp = SearchSysCache1(COLLOID, ObjectIdGetDatum(collid));
+ if (!HeapTupleIsValid(tp))
+ elog(ERROR, "cache lookup failed for collation %u", collid);
+ collform = (Form_pg_collation) GETSTRUCT(tp);
+
+ datum = SysCacheGetAttr(COLLOID, tp, Anum_pg_collation_collversion,
+ &isnull);
+ if (isnull)
+ {
+ ReleaseSysCache(tp);
+ return;
+ }
+
+ collversionstr = TextDatumGetCString(datum);
+
+ if (collform->collprovider == COLLPROVIDER_LIBC)
+ datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_collcollate);
+ else
+ datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_colllocale);
+
+ 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))));
+ }
+
+ 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)))));
+
+ ReleaseSysCache(tp);
+}
+
/*
* 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.
+ *
+ * Ensure that no path leaks a collator. That is, create the collator after
+ * any error paths, such as memory allocation.
*/
static pg_locale_t
create_pg_locale(Oid collid)
@@ -1535,7 +1607,6 @@ create_pg_locale(Oid collid)
struct pg_locale_struct result;
pg_locale_t resultp;
Datum datum;
- bool isnull;
tp = SearchSysCache1(COLLOID, ObjectIdGetDatum(collid));
if (!HeapTupleIsValid(tp))
@@ -1584,6 +1655,7 @@ create_pg_locale(Oid collid)
#ifdef USE_ICU
const char *iculocstr;
const char *icurules;
+ bool isnull;
datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_colllocale);
iculocstr = TextDatumGetCString(datum);
@@ -1607,48 +1679,6 @@ create_pg_locale(Oid collid)
#endif /* not USE_ICU */
}
- datum = SysCacheGetAttr(COLLOID, tp, Anum_pg_collation_collversion,
- &isnull);
- if (!isnull)
- {
- char *actual_versionstr;
- char *collversionstr;
-
- collversionstr = TextDatumGetCString(datum);
-
- if (collform->collprovider == COLLPROVIDER_LIBC)
- datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_collcollate);
- else
- datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_colllocale);
-
- 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))));
- }
-
- 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)))));
- }
-
ReleaseSysCache(tp);
/* We'll keep the pg_locale_t structures in TopMemoryContext */
@@ -1693,7 +1723,13 @@ pg_newlocale_from_collation(Oid collid)
cache_entry = collation_cache_insert(CollationCache, collid, &found);
if (!found || !cache_entry->locale)
- cache_entry->locale = create_pg_locale(collid);
+ {
+ pg_locale_t locale = create_pg_locale(collid);
+
+ check_collation_version(collid);
+
+ cache_entry->locale = locale;
+ }
return cache_entry->locale;
}
--
2.34.1
From bc196e948a06b07369fe1623f03f1f549fc48a2c 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 v2 4/6] 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 | 49 +++++++++++++------------------
1 file changed, 21 insertions(+), 28 deletions(-)
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index db9d1c01947..c52d6989802 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -1589,23 +1589,22 @@ check_collation_version(Oid collid)
}
/*
- * 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.
*
* Ensure that no path leaks a collator. That is, create the collator after
* any error paths, such as memory allocation.
*/
static pg_locale_t
-create_pg_locale(Oid collid)
+create_pg_locale(Oid collid, MemoryContext context)
{
/* 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;
tp = SearchSysCache1(COLLOID, ObjectIdGetDatum(collid));
@@ -1613,10 +1612,9 @@ create_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 = MemoryContextAllocZero(context, sizeof(struct pg_locale_struct));
+ result->provider = collform->collprovider;
+ result->deterministic = collform->collisdeterministic;
if (collform->collprovider == COLLPROVIDER_BUILTIN)
{
@@ -1625,13 +1623,12 @@ create_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 = MemoryContextStrdup(context, locstr);
}
else if (collform->collprovider == COLLPROVIDER_LIBC)
{
@@ -1643,12 +1640,12 @@ create_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)
{
@@ -1660,8 +1657,8 @@ create_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)
@@ -1669,8 +1666,8 @@ create_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.locale = MemoryContextStrdup(context, iculocstr);
+ result->info.icu.ucol = make_icu_collator(iculocstr, icurules);
#else /* not USE_ICU */
/* could get here if a collation was created by a build with ICU */
ereport(ERROR,
@@ -1681,11 +1678,7 @@ create_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;
}
/*
@@ -1724,7 +1717,7 @@ pg_newlocale_from_collation(Oid collid)
cache_entry = collation_cache_insert(CollationCache, collid, &found);
if (!found || !cache_entry->locale)
{
- pg_locale_t locale = create_pg_locale(collid);
+ pg_locale_t locale = create_pg_locale(collid, CollationCacheContext);
check_collation_version(collid);
--
2.34.1
From 994d970c648b3ab13bd9120e03a97823628a5255 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 v2 5/6] 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 | 72 ++++++++++++++++++++++++++++++-
src/include/utils/pg_locale.h | 4 +-
2 files changed, 73 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index c52d6989802..47f902b8f59 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,12 @@ 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.
+ */
+static ResourceOwner CollationCacheOwner = NULL;
+
#if defined(WIN32) && defined(LC_MESSAGES)
static char *IsoLocaleName(const char *);
#endif
@@ -174,6 +181,24 @@ static void icu_set_collation_attributes(UCollator *collator, const char *loc,
UErrorCode *status);
#endif
+static void ResOwnerReleasePGLocale(Datum val);
+static void pg_freelocale(pg_locale_t val);
+
+static const ResourceOwnerDesc PGLocaleResourceKind =
+{
+ .name = "pg_locale_t reference",
+ .release_phase = RESOURCE_RELEASE_AFTER_LOCKS,
+ .release_priority = RELEASE_PRIO_LAST,
+ .ReleaseResource = ResOwnerReleasePGLocale,
+ .DebugPrint = NULL /* the default message is fine */
+};
+
+static void
+ResOwnerReleasePGLocale(Datum val)
+{
+ pg_freelocale((pg_locale_t) DatumGetPointer(val));
+}
+
/*
* POSIX doesn't define _l-variants of these functions, but several systems
* have them. We provide our own replacements here.
@@ -1707,6 +1732,7 @@ pg_newlocale_from_collation(Oid collid)
*/
if (CollationCache == NULL)
{
+ CollationCacheOwner = ResourceOwnerCreate(NULL, "collation cache");
CollationCacheContext = AllocSetContextCreate(TopMemoryContext,
"collation cache",
ALLOCSET_DEFAULT_SIZES);
@@ -1717,16 +1743,60 @@ pg_newlocale_from_collation(Oid collid)
cache_entry = collation_cache_insert(CollationCache, collid, &found);
if (!found || !cache_entry->locale)
{
- pg_locale_t locale = create_pg_locale(collid, CollationCacheContext);
+ pg_locale_t locale;
+
+ ResourceOwnerEnlarge(CollationCacheOwner);
+
+ locale = create_pg_locale(collid, CollationCacheContext);
+ ResourceOwnerRemember(CurrentResourceOwner,
+ PointerGetDatum(locale),
+ &PGLocaleResourceKind);
check_collation_version(collid);
+ /* reassign locale from CurrentResourceOwner to CollationCacheOwner */
+ ResourceOwnerForget(CurrentResourceOwner,
+ PointerGetDatum(locale),
+ &PGLocaleResourceKind);
+ ResourceOwnerRemember(CollationCacheOwner,
+ PointerGetDatum(locale),
+ &PGLocaleResourceKind);
+
cache_entry->locale = locale;
}
return cache_entry->locale;
}
+static void
+pg_freelocale(pg_locale_t locale)
+{
+ if (locale->provider == COLLPROVIDER_BUILTIN)
+ {
+ pfree(locale->info.builtin.locale);
+ }
+#ifdef USE_ICU
+ else if (locale->provider == COLLPROVIDER_ICU)
+ {
+ ucol_close(locale->info.icu.ucol);
+ pfree(locale->info.icu.locale);
+ }
+#endif
+ else if (locale->provider == COLLPROVIDER_LIBC)
+ {
+ if (locale->info.lt != NULL)
+ {
+#ifndef WIN32
+ freelocale(locale->info.lt);
+#else
+ _free_locale(locale->info.lt);
+#endif
+ }
+ }
+
+ pfree(locale);
+}
+
/*
* Get provider-specific collation version string for the given collation from
* the operating system/library.
diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h
index 1ca0b7fa74b..57096fe484f 100644
--- a/src/include/utils/pg_locale.h
+++ b/src/include/utils/pg_locale.h
@@ -92,13 +92,13 @@ struct pg_locale_struct
{
struct
{
- const char *locale;
+ char *locale;
} builtin;
locale_t lt;
#ifdef USE_ICU
struct
{
- const char *locale;
+ char *locale;
UCollator *ucol;
} icu;
#endif
--
2.34.1
From 9bba83e8c9e19e442c0e8ad2f90a4fd1d14fe4f1 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 v2 6/6] 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 | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index 47f902b8f59..f5f45f72a13 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"
@@ -1453,6 +1454,18 @@ pg_locale_deterministic(pg_locale_t locale)
return locale->deterministic;
}
+static void
+CollationCacheInvalidate(Datum arg, int cacheid, uint32 hashvalue)
+{
+ ResourceOwnerReleaseAllOfKind(CollationCacheOwner,
+ &PGLocaleResourceKind);
+
+ /* free all memory and reset hash table */
+ MemoryContextReset(CollationCacheContext);
+ CollationCache = collation_cache_create(CollationCacheContext,
+ 16, NULL);
+}
+
/*
* Initialize default_locale with database locale settings.
*/
@@ -1722,14 +1735,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");
@@ -1738,6 +1744,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);
--
2.34.1
