agora inbox for pgsql-committers@postgresql.org
help / color / mirror / Atom feedFrom: Jeff Davis <pgsql@j-davis.com>
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: Jeff Davis <jdavis@postgresql.org>
Cc: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Perform provider-specific initialization in new functions.
Date: Mon, 13 Apr 2026 14:08:20 -0700
Message-ID: <9e74ecd519ac5a1a2a5fefec86d261037f3db311.camel@j-davis.com> (raw)
In-Reply-To: <2FCB31FC-43E9-45F6-9AF4-4E199132B4E5@yandex-team.ru>
References: <E1tINJx-000sMi-Vr@gemulon.postgresql.org>
<D18AD72A-5004-4EF8-AF80-10732AF677FA@yandex-team.ru>
<dba69ec2334f9cff04d102de243ea912a3166586.camel@j-davis.com>
<2FCB31FC-43E9-45F6-9AF4-4E199132B4E5@yandex-team.ru>
On Sat, 2026-03-07 at 16:36 +0500, Andrey Borodin wrote:
> > 1. Your fix addresses it, and would also add some safety against
> > other edge cases we haven't caught yet. The only time it would take
> > effect is for very early initialization
>
> Maybe let's sprinkle with asserts like "I'm walsender"? Or at least
> "I'm not a user backend"?
That seems like excessive coupling that's hard to explain.
> > , but there is nonzero risk of
> > inconsistency because the same value would get a different hash
> > before
> > and after CheckMyDatabase().
>
> Sounds scary, actually. I heard of several corruptions that started
> with
> bogus cache entries.
Yeah, I'd prefer not take this approach.
> > 2. We could hardcode texthashfunc() to use C_COLLATION_OID. That
> > wouldn't match the column collation, but it would avoid the crash,
> > and
> > might technically still be fine: the default collation is always
> > deterministic, and all deterministic collations have the same
> > equality
> > semantics as "C". Even if the proper hashtext() is used somewhere
> > else,
> > then it uses "C" hashing semantics for all deterministic
> > collations.
> > The problem here is that we'd like to allow the default collation
> > to be
> > nondeterministic in the future (Peter has mentioned this a few
> > times),
> > so relying on this assumption is fragile.
Attached. I think this is the least-invasive patch to apply to master
now, because it doesn't change any assumptions.
The assumption "any deterministic collation will do" is still the same,
it just chooses C_COLLATION_OID rather than DEFAULT_COLLATION_OID. That
has two benefits:
1. Fixes your issue, because C_COLLATION_OID is always available.
2. Faster than a default collation based on libc or ICU.
Note that you may still have other problems trying to do interesting
things before CheckMyDatabase(), so I'm not necessarily endorsing that,
but this patch seems good regardless.
>
I don't see a reason to backport this, but if someone else does then I
could be convinced.
Thoughts?
Regards,
Jeff Davis
Attachments:
[text/x-patch] v1-0001-catcache.c-always-use-C_COLLATION_OID.patch (2.7K, ../9e74ecd519ac5a1a2a5fefec86d261037f3db311.camel@j-davis.com/2-v1-0001-catcache.c-always-use-C_COLLATION_OID.patch)
download | inline diff:
From 193a85477460fb1c46b60d39a78ac9d849f58c72 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Mon, 13 Apr 2026 12:09:40 -0700
Subject: [PATCH v1] catcache.c: always use C_COLLATION_OID.
Previously, texthashfast/texteqfast used DEFAULT_COLLATION_OID. As the
comments stated, that was arbitrary anyway -- if the collation
actually mattered, it should use the column's actual collation. (In
the catalog, some text columns are the default collation and some are
"C".)
When any deterministic collation will do, it's best to consistently
use the simplest and fastest one, so this commit chooses
C_COLLATION_OID.
The original report was to allow the catalog cache to be used in early
code paths before CheckMyDatabase(), such as GUC processing in the
walsender. Using C_COLLATION_OID solves that problem as well, because
it's always available.
Reported-by: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/D18AD72A-5004-4EF8-AF80-10732AF677FA@yandex-team.ru
---
src/backend/utils/cache/catcache.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 87ed5506460..a8e7bf649d2 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -205,6 +205,10 @@ nameeqfast(Datum a, Datum b)
char *ca = NameStr(*DatumGetName(a));
char *cb = NameStr(*DatumGetName(b));
+ /*
+ * Catalogs only use deterministic collations, so ignore column collation
+ * and use fast path.
+ */
return strncmp(ca, cb, NAMEDATALEN) == 0;
}
@@ -213,6 +217,10 @@ namehashfast(Datum datum)
{
char *key = NameStr(*DatumGetName(datum));
+ /*
+ * Catalogs only use deterministic collations, so ignore column collation
+ * and use fast path.
+ */
return hash_bytes((unsigned char *) key, strlen(key));
}
@@ -244,17 +252,20 @@ static bool
texteqfast(Datum a, Datum b)
{
/*
- * The use of DEFAULT_COLLATION_OID is fairly arbitrary here. We just
- * want to take the fast "deterministic" path in texteq().
+ * Catalogs only use deterministic collations, so ignore column collation
+ * and use "C" locale for efficiency.
*/
- return DatumGetBool(DirectFunctionCall2Coll(texteq, DEFAULT_COLLATION_OID, a, b));
+ return DatumGetBool(DirectFunctionCall2Coll(texteq, C_COLLATION_OID, a, b));
}
static uint32
texthashfast(Datum datum)
{
- /* analogously here as in texteqfast() */
- return DatumGetInt32(DirectFunctionCall1Coll(hashtext, DEFAULT_COLLATION_OID, datum));
+ /*
+ * Catalogs only use deterministic collations, so ignore column collation
+ * and use "C" locale for efficiency.
+ */
+ return DatumGetInt32(DirectFunctionCall1Coll(hashtext, C_COLLATION_OID, datum));
}
static bool
--
2.43.0
view thread (8+ messages) latest in thread
Message-ID: <9e74ecd519ac5a1a2a5fefec86d261037f3db311.camel@j-davis.com>
Permalink: ../9e74ecd519ac5a1a2a5fefec86d261037f3db311.camel@j-davis.com/
Also on: postgresql.org/message-id/9e74ecd519ac5a1a2a5fefec86d261037f3db311.camel@j-davis.com
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-committers@postgresql.org
Cc: pgsql@j-davis.com, x4mmm@yandex-team.ru, jdavis@postgresql.org, pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Perform provider-specific initialization in new functions.
In-Reply-To: <9e74ecd519ac5a1a2a5fefec86d261037f3db311.camel@j-davis.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox