agora inbox for pgsql-committers@postgresql.orghelp / color / mirror / Atom feed
pgsql: Perform provider-specific initialization in new functions. 8+ messages / 3 participants [nested] [flat]
* pgsql: Perform provider-specific initialization in new functions. @ 2024-12-03 07:27 Jeff Davis <jdavis@postgresql.org> 0 siblings, 1 reply; 8+ messages in thread From: Jeff Davis @ 2024-12-03 07:27 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Perform provider-specific initialization in new functions. Reviewed-by: Andreas Karlsson Discussion: https://postgr.es/m/4548a168-62cd-457b-8d06-9ba7b985c477@proxel.se Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/1ba0782ce90cb4261098de59b49ae5cb2326566b Modified Files -------------- src/backend/utils/adt/Makefile | 1 + src/backend/utils/adt/meson.build | 1 + src/backend/utils/adt/pg_locale.c | 162 +++++------------------------- src/backend/utils/adt/pg_locale_builtin.c | 70 +++++++++++++ src/backend/utils/adt/pg_locale_icu.c | 97 +++++++++++++++++- src/backend/utils/adt/pg_locale_libc.c | 74 +++++++++++++- 6 files changed, 259 insertions(+), 146 deletions(-) ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Perform provider-specific initialization in new functions. @ 2026-03-06 15:24 Andrey Borodin <x4mmm@yandex-team.ru> parent: Jeff Davis <jdavis@postgresql.org> 0 siblings, 1 reply; 8+ messages in thread From: Andrey Borodin @ 2026-03-06 15:24 UTC (permalink / raw) To: Jeff Davis <jdavis@postgresql.org>; +Cc: pgsql-committers@lists.postgresql.org > On 3 Dec 2024, at 12:27, Jeff Davis <jdavis@postgresql.org> wrote: > > Perform provider-specific initialization in new functions. > > Reviewed-by: Andreas Karlsson > Discussion: https://postgr.es/m/4548a168-62cd-457b-8d06-9ba7b985c477@proxel.se Hi Jeff! I'm toying with my WAL compression patch. This test is segfaulting for me: src/test/recovery % PROVE_TESTS="t/001_stream_rep.pl" PGOPTIONS="-c wal_compression=lz4" make check in src/test/recovery/tmp_check/log/001_stream_rep_primary.log I observe: 2026-03-06 20:03:01.433 +05 [24424] 001_stream_rep.pl LOG: statement: GRANT pg_read_all_settings TO repl_role; 2026-03-06 20:03:01.441 +05 [24318] LOG: client backend (PID 24426) was terminated by signal 11: Segmentation fault: 11 2026-03-06 20:03:01.441 +05 [24318] LOG: terminating any other active server processes I do not expect it to pass, it would never pass with given arguments. But I do not expect it to segfault either. I bisected the problem, some tome ago this test would fail with something like "permission denied to change wal_compression". And that seems good. Later it became "Cannot find collation for ..." And after 1ba0782ce90cb4261098de59b49ae5cb2326566b it crashes. wal_compression is PGC_SUSET, so when a non-superuser sets it via the startup packet (PGC_BACKEND context), set_config_with_handle must call pg_parameter_aclcheck -> SearchSysCache1(PARAMETERACLNAME, ...) -> hashtext -> pg_newlocale_from_collation(DEFAULT_COLLATION_OID). I do not know if it's expected, so I decided to report this problem, just in case. I can suggest something in a line with attached, but it's kind of point-in-the-sky fix. Best regards, Andrey Borodin. Attachments: [application/octet-stream] fix.diff (1.1K, ../../D18AD72A-5004-4EF8-AF80-10732AF677FA@yandex-team.ru/2-fix.diff) download | inline diff: diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c index ac324ecaad2..938b9b03344 100644 --- a/src/backend/utils/adt/pg_locale.c +++ b/src/backend/utils/adt/pg_locale.c @@ -1192,7 +1192,27 @@ pg_newlocale_from_collation(Oid collid) bool found; if (collid == DEFAULT_COLLATION_OID) + { + /* + * default_locale is set by init_database_collation(), which is called + * from CheckMyDatabase(). Physical walsenders and other early-init + * code paths skip CheckMyDatabase, so default_locale can still be NULL + * when catalog hash lookups (e.g. pg_parameter_acl) reach here. + * Return a minimal deterministic C-locale struct so callers like + * hashtext() don't crash; GUC names are ASCII, so locale is irrelevant. + */ + if (unlikely(default_locale == NULL)) + { + static const struct pg_locale_struct nodb_locale = { + .provider = COLLPROVIDER_LIBC, + .deterministic = true, + .collate_is_c = true, + .ctype_is_c = true, + }; + return (pg_locale_t) &nodb_locale; + } return default_locale; + } /* * Some callers expect C_COLLATION_OID to succeed even without catalog ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Perform provider-specific initialization in new functions. @ 2026-03-06 20:01 Jeff Davis <pgsql@j-davis.com> parent: Andrey Borodin <x4mmm@yandex-team.ru> 0 siblings, 1 reply; 8+ messages in thread From: Jeff Davis @ 2026-03-06 20:01 UTC (permalink / raw) To: Andrey Borodin <x4mmm@yandex-team.ru>; Jeff Davis <jdavis@postgresql.org>; +Cc: pgsql-committers@lists.postgresql.org On Fri, 2026-03-06 at 20:24 +0500, Andrey Borodin wrote: > I'm toying with my WAL compression patch. This test is segfaulting > for me Thank you for the report! > wal_compression is PGC_SUSET, so when a non-superuser sets it via the > startup > packet (PGC_BACKEND context), set_config_with_handle must call > pg_parameter_aclcheck -> SearchSysCache1(PARAMETERACLNAME, ...) -> > hashtext -> > pg_newlocale_from_collation(DEFAULT_COLLATION_OID). It seems like the real problem here is in catcache.c:texthashfast(), which unconditionally passes DEFAULT_COLLATION_OID, despite the fact that pg_parameter_acl.parname has collation "C". namehashfast() uses C-like semantics, which is OK because all name columns in the catalog have collation "C". But TEXT columns in the catalog are about a mix of DEFAULT_COLLATION_OID and C_COLLATION_OID. There are a few possible fixes: 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, but there is nonzero risk of inconsistency because the same value would get a different hash before and after CheckMyDatabase(). 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. 3. We could try to include collation information in the cachinfo or somewhere and pass it down to find the right hash function. This feels like a better fix, but there could be other areas we miss that are using a catalog TEXT field with the default collation. Also it's more invasive. We could decide to do your approach for now (in master and REL_18_STABLE), and then leave #3 for the future (master only). Thoughts? Regards, Jeff Davis ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Perform provider-specific initialization in new functions. @ 2026-03-07 11:36 Andrey Borodin <x4mmm@yandex-team.ru> parent: Jeff Davis <pgsql@j-davis.com> 0 siblings, 1 reply; 8+ messages in thread From: Andrey Borodin @ 2026-03-07 11:36 UTC (permalink / raw) To: Jeff Davis <pgsql@j-davis.com>; +Cc: Jeff Davis <jdavis@postgresql.org>; pgsql-committers@lists.postgresql.org Thanks for the swift reply! > On 7 Mar 2026, at 01:01, Jeff Davis <pgsql@j-davis.com> 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"? > , 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. > 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. > > 3. We could try to include collation information in the cachinfo or > somewhere and pass it down to find the > right hash function. IMO anything less cannot be correct in a long run. Allowing random hash function is a minefield. > This feels > like a better fix, but there could be other areas we miss that are > using a catalog TEXT field with the default collation. Also it's more > invasive. > > We could decide to do your approach for now (in master and > REL_18_STABLE), and then leave #3 for the future (master only). The approach sounds fine for me. Can we be sure hash function does not change after CheckMyDatabase()? Possible coredump by authenticated user does not sound like big issue. And I never saw any report about it in the wild. Best regards, Andrey Borodin. ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Perform provider-specific initialization in new functions. @ 2026-04-13 21:08 Jeff Davis <pgsql@j-davis.com> parent: Andrey Borodin <x4mmm@yandex-team.ru> 0 siblings, 1 reply; 8+ messages in thread From: Jeff Davis @ 2026-04-13 21:08 UTC (permalink / raw) To: Andrey Borodin <x4mmm@yandex-team.ru>; +Cc: Jeff Davis <jdavis@postgresql.org>; pgsql-committers@lists.postgresql.org 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 ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Perform provider-specific initialization in new functions. @ 2026-04-16 18:42 Jeff Davis <pgsql@j-davis.com> parent: Jeff Davis <pgsql@j-davis.com> 0 siblings, 1 reply; 8+ messages in thread From: Jeff Davis @ 2026-04-16 18:42 UTC (permalink / raw) To: Andrey Borodin <x4mmm@yandex-team.ru>; +Cc: Jeff Davis <jdavis@postgresql.org>; pgsql-committers@lists.postgresql.org On Mon, 2026-04-13 at 14:08 -0700, Jeff Davis wrote: > 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. I didn't detect any meaningful improvement here, probably because most catalog cache lookups use columns with type NAME not TEXT. But this still seems like a good idea on the grounds that, if we are choosing an arbitrary deterministic collation for some internal purpose, we should consistently choose the simplest and fastest one. > > I don't see a reason to backport this, but if someone else does then > I > could be convinced. I plan to commit this soon. I don't plan to backport unless someone sees a reason that it should be backported (and if so, how far?). Regards, Jeff Davis ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Perform provider-specific initialization in new functions. @ 2026-04-22 04:48 Jeff Davis <pgsql@j-davis.com> parent: Jeff Davis <pgsql@j-davis.com> 0 siblings, 1 reply; 8+ messages in thread From: Jeff Davis @ 2026-04-22 04:48 UTC (permalink / raw) To: Andrey Borodin <x4mmm@yandex-team.ru>; +Cc: Jeff Davis <jdavis@postgresql.org>; pgsql-committers@lists.postgresql.org On Thu, 2026-04-16 at 11:42 -0700, Jeff Davis wrote: > I plan to commit this soon. > > I don't plan to backport unless someone sees a reason that it should > be > backported (and if so, how far?). Actually, this does need to be backported, a NULL pointer dereference is easily reproducible on master and v18: PGOPTIONS="-c zero_damaged_pages=on" \ pg_receivewal -D archive -U repl On 17 the symptom is slightly different but the fix is the same. I attached a new patch, and only the commit message is different, which I plan to backport to 17. There's another bug, though. Even with the patch applied, if you do the same pg_receivewal command immediately after starting the server (without any other connections), you get: FATAL: cannot read pg_class without having selected a database The path is similar: it's trying to do pg_parameter_aclcheck, but is unable to open pg_parameter_acl at all because it can't read pg_class. It seems to work if you connect another backend first, where it does some initialization first, through I haven't worked out the details. I think it goes back to when parameter ACLs were introduced in a0ffa885e47, so CC Mark Dilger. Regards, Jeff Davis Attachments: [text/x-patch] v2-0001-catcache.c-always-use-C_COLLATION_OID.patch (3.2K, ../../4524ed61a015d3496fc008644dcb999bb31916a7.camel@j-davis.com/2-v2-0001-catcache.c-always-use-C_COLLATION_OID.patch) download | inline diff: From 39f0bc866b092a5def31184bf7f8ce7b7fcd7f89 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 v2] catcache.c: always use C_COLLATION_OID. The problem report was about setting GUCs in the startup packet when initiating a replication connection. Setting the GUC required an ACL check, which performed a catalog lookup on pg_parameter_acl.parname (type TEXT). The catalog cache was hardwired to use DEFAULT_COLLATION_OID for text attributes, but walsender never calls CheckMyDatabase(), so the default collation was uninitialized and it caused a NULL pointer dereference. As the comments stated, using DEFAULT_COLLATION_OID 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".) Fix by using C_COLLATION_OID, which doesn't require any initialization and is always available. When any deterministic collation will do, it's best to consistently use the simplest and fastest one, so this is a good idea anyway. There may be other problems in this general area, so this should not be considered a complete fix. But this is an independently good change and solves the immediate problem. Reported-by: Andrey Borodin <x4mmm@yandex-team.ru> Discussion: https://postgr.es/m/D18AD72A-5004-4EF8-AF80-10732AF677FA@yandex-team.ru Backpatch-through: 17 --- 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 ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Perform provider-specific initialization in new functions. @ 2026-04-22 19:24 Jeff Davis <pgsql@j-davis.com> parent: Jeff Davis <pgsql@j-davis.com> 0 siblings, 0 replies; 8+ messages in thread From: Jeff Davis @ 2026-04-22 19:24 UTC (permalink / raw) To: Andrey Borodin <x4mmm@yandex-team.ru>; +Cc: Jeff Davis <jdavis@postgresql.org>; pgsql-committers@lists.postgresql.org On Tue, 2026-04-21 at 21:48 -0700, Jeff Davis wrote: > I attached a new patch, and only the commit message is different, > which > I plan to backport to 17. Committed. > There's another bug, though. Even with the patch applied, if you do > the > same pg_receivewal command immediately after starting the server > (without any other connections), you get: > > FATAL: cannot read pg_class without having selected a database This is a separate issue so I moved the discussion here: https://www.postgresql.org/message-id/d8f8e11f06d692fff89e6be0f22732d30cf695a0.camel@j-davis.com Any other loose ends from this discussion? Regards, Jeff Davis ^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2026-04-22 19:24 UTC | newest] Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-12-03 07:27 pgsql: Perform provider-specific initialization in new functions. Jeff Davis <jdavis@postgresql.org> 2026-03-06 15:24 ` Andrey Borodin <x4mmm@yandex-team.ru> 2026-03-06 20:01 ` Jeff Davis <pgsql@j-davis.com> 2026-03-07 11:36 ` Andrey Borodin <x4mmm@yandex-team.ru> 2026-04-13 21:08 ` Jeff Davis <pgsql@j-davis.com> 2026-04-16 18:42 ` Jeff Davis <pgsql@j-davis.com> 2026-04-22 04:48 ` Jeff Davis <pgsql@j-davis.com> 2026-04-22 19:24 ` Jeff Davis <pgsql@j-davis.com>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox