public inbox for [email protected]help / color / mirror / Atom feed
GUC parameter ACLs and physical walsender 6+ messages / 3 participants [nested] [flat]
* GUC parameter ACLs and physical walsender @ 2026-04-22 19:18 Jeff Davis <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Jeff Davis @ 2026-04-22 19:18 UTC (permalink / raw) To: pgsql-hackers; +Cc: Andrey Borodin <[email protected]>; Mark Dilger <[email protected]> Moving discussion from: https://www.postgresql.org/message-id/[email protected] because this is a separate issue. If you specify a SUSET GUC setting when connecting as non-superuser for physical replication: PGOPTIONS="-c wal_compression=on" \ pg_receivewal -D archive -U repl you get: FATAL: cannot read pg_class without having selected a database but only if you connect immediately after the server starts. If you do something else first, like an ordinary connection and "SELECT 1", and then start the replication connection, you get (after commit dbf217c1c7): FATAL: permission denied to set parameter "wal_compression" as expected. The problem goes back to a0ffa885e47. It seems to be because pg_parameter_acl is not nailed in cache. I attached a quick patch to do so (which turns it into the "expected permission denied" error). But I'm not sure if that's the right fix, or if it would be a complete fix. I also don't think that would be backportable, but perhaps? Regards, Jeff Davis Attachments: [text/x-patch] v1-0001-Nail-pg_parameter_acl-in-relcache.patch (4.4K, 2-v1-0001-Nail-pg_parameter_acl-in-relcache.patch) download | inline diff: From 5cf20081de5172ec9a1d768b0584d40bfbf9cb12 Mon Sep 17 00:00:00 2001 From: Jeff Davis <[email protected]> Date: Wed, 22 Apr 2026 11:50:12 -0700 Subject: [PATCH v1] Nail pg_parameter_acl in relcache. Previously, a parameter specified in the startup packet for a physical replication connection could encounter an error trying to perform an ACL check for the setting. --- src/backend/utils/cache/catcache.c | 2 ++ src/backend/utils/cache/relcache.c | 19 ++++++++++++++----- src/include/catalog/pg_parameter_acl.h | 2 +- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c index a8e7bf649d2..6fb35dedf95 100644 --- a/src/backend/utils/cache/catcache.c +++ b/src/backend/utils/cache/catcache.c @@ -1335,6 +1335,8 @@ IndexScanOK(CatCache *cache) case AUTHOID: case AUTHMEMMEMROLE: case DATABASEOID: + case PARAMETERACLNAME: + case PARAMETERACLOID: /* * Protect authentication lookups occurring before relcache has diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index e19f0d3e51c..10d481518eb 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -56,6 +56,7 @@ #include "catalog/pg_proc.h" #include "catalog/pg_publication.h" #include "catalog/pg_rewrite.h" +#include "catalog/pg_parameter_acl.h" #include "catalog/pg_shseclabel.h" #include "catalog/pg_statistic_ext.h" #include "catalog/pg_subscription.h" @@ -120,6 +121,7 @@ static const FormData_pg_attribute Desc_pg_auth_members[Natts_pg_auth_members] = static const FormData_pg_attribute Desc_pg_index[Natts_pg_index] = {Schema_pg_index}; static const FormData_pg_attribute Desc_pg_shseclabel[Natts_pg_shseclabel] = {Schema_pg_shseclabel}; static const FormData_pg_attribute Desc_pg_subscription[Natts_pg_subscription] = {Schema_pg_subscription}; +static const FormData_pg_attribute Desc_pg_parameter_acl[Natts_pg_parameter_acl] = {Schema_pg_parameter_acl}; /* * Hash tables that index the relation cache @@ -4074,8 +4076,10 @@ RelationCacheInitializePhase2(void) Natts_pg_shseclabel, Desc_pg_shseclabel); formrdesc("pg_subscription", SubscriptionRelation_Rowtype_Id, true, Natts_pg_subscription, Desc_pg_subscription); + formrdesc("pg_parameter_acl", ParameterAclRelation_Rowtype_Id, true, + Natts_pg_parameter_acl, Desc_pg_parameter_acl); -#define NUM_CRITICAL_SHARED_RELS 5 /* fix if you change list above */ +#define NUM_CRITICAL_SHARED_RELS 6 /* fix if you change list above */ } MemoryContextSwitchTo(oldcxt); @@ -4196,9 +4200,10 @@ RelationCacheInitializePhase3(void) * non-shared catalogs at all. Autovacuum calls InitPostgres with a * database OID, so it instead depends on DatabaseOidIndexId. We also * need to nail up some indexes on pg_authid and pg_auth_members for use - * during client authentication. SharedSecLabelObjectIndexId isn't - * critical for the core system, but authentication hooks might be - * interested in it. + * during client authentication. We need indexes on pg_parameter_acl for + * ACL checks on settings specified in the startup packet for a physical + * replication connection. SharedSecLabelObjectIndexId isn't critical for + * the core system, but authentication hooks might be interested in it. */ if (!criticalSharedRelcachesBuilt) { @@ -4214,8 +4219,12 @@ RelationCacheInitializePhase3(void) AuthMemRelationId); load_critical_index(SharedSecLabelObjectIndexId, SharedSecLabelRelationId); + load_critical_index(ParameterAclParnameIndexId, + ParameterAclRelationId); + load_critical_index(ParameterAclOidIndexId, + ParameterAclRelationId); -#define NUM_CRITICAL_SHARED_INDEXES 6 /* fix if you change list above */ +#define NUM_CRITICAL_SHARED_INDEXES 8 /* fix if you change list above */ criticalSharedRelcachesBuilt = true; } diff --git a/src/include/catalog/pg_parameter_acl.h b/src/include/catalog/pg_parameter_acl.h index a26b05a9bf2..902e2666069 100644 --- a/src/include/catalog/pg_parameter_acl.h +++ b/src/include/catalog/pg_parameter_acl.h @@ -29,7 +29,7 @@ */ BEGIN_CATALOG_STRUCT -CATALOG(pg_parameter_acl,6243,ParameterAclRelationId) BKI_SHARED_RELATION +CATALOG(pg_parameter_acl,6243,ParameterAclRelationId) BKI_SHARED_RELATION BKI_ROWTYPE_OID(2173,ParameterAclRelation_Rowtype_Id) BKI_SCHEMA_MACRO { Oid oid; /* oid */ -- 2.43.0 ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: GUC parameter ACLs and physical walsender @ 2026-04-23 03:27 John Naylor <[email protected]> parent: Jeff Davis <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: John Naylor @ 2026-04-23 03:27 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: pgsql-hackers; Andrey Borodin <[email protected]>; Mark Dilger <[email protected]> On Thu, Apr 23, 2026 at 2:19 AM Jeff Davis <[email protected]> wrote: > It seems to be because pg_parameter_acl is not nailed in cache. I > attached a quick patch to do so (which turns it into the "expected > permission denied" error). But I'm not sure if that's the right fix, or > if it would be a complete fix. I also don't think that would be > backportable, but perhaps? I think in existing installations AddNewRelationType() would have picked some oid already, so fixing rowtype_oid at initdb time would only work in the master branch. -- John Naylor Amazon Web Services ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: GUC parameter ACLs and physical walsender @ 2026-04-23 17:57 Mark Dilger <[email protected]> parent: John Naylor <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Mark Dilger @ 2026-04-23 17:57 UTC (permalink / raw) To: John Naylor <[email protected]>; +Cc: Jeff Davis <[email protected]>; pgsql-hackers; Andrey Borodin <[email protected]> On Wed, Apr 22, 2026 at 8:27 PM John Naylor <[email protected]> wrote: > On Thu, Apr 23, 2026 at 2:19 AM Jeff Davis <[email protected]> wrote: > > It seems to be because pg_parameter_acl is not nailed in cache. I > > attached a quick patch to do so (which turns it into the "expected > > permission denied" error). But I'm not sure if that's the right fix, or > > if it would be a complete fix. I also don't think that would be > > backportable, but perhaps? > > I think in existing installations AddNewRelationType() would have > picked some oid already, so fixing rowtype_oid at initdb time would > only work in the master branch. > John is right that the hardcoded BKI_ROWTYPE_OID(2173) makes this non-backportable as-is. On existing installations (PG 15-18), AddNewRelationType() assigned rowtype OID 10097 to pg_parameter_acl during initdb. Jeff's patch has formrdesc set rd_att->tdtypeid = 2173, but that field is never corrected — Phase3 overwrites rd_rel (including reltype) via memcpy, but rd_att->tdtypeid stays at the formrdesc value. The comment in formrdesc says as much: "this data had better be right because it will never be replaced." On assert-enabled builds, every backend crashes on the first connection: TRAP: failed Assert("relation->rd_att->tdtypeid == relp->reltype"), File: "relcache.c", Line: 4294 On non-assert builds, the mismatch has no observable effect. The wrong typeId gets stamped into pg_parameter_acl tuple headers by heap_form_tuple, but nothing reads it back -- all consumers of HeapTupleHeaderGetTypeId are in the executor, PL/pgSQL, and record-type processing, not catalog operations. For backports, formrdesc would need to use something other than the hardcoded OID -- either look up the real rowtype OID, or pass InvalidOid and teach the Assert to tolerate it for late-nailed relations. To demonstrate, using an existing cluster whose initdb was done without the nailing patch (rowtype 10097), then started with patched binaries: after applying Jeff's v1-0001-Nail-pg_parameter_acl-in-relcache.patch to master, which has dbf217c1c7 already applied: --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -1965,6 +1965,12 @@ formrdesc(const char *relationName, Oid relationReltype, relation->rd_att->tdtypeid = relationReltype; relation->rd_att->tdtypmod = -1; /* just to be sure */ + elog(LOG, "formrdesc \"%s\": relid=%u reltype=%u tdtypeid=%u", + relationName, + TupleDescAttr(relation->rd_att, 0)->attrelid, + relationReltype, + relation->rd_att->tdtypeid); + /* * initialize tuple desc info */ @@ -4283,6 +4289,14 @@ RelationCacheInitializePhase3(void) * data correctly to start with, because it may already have been * copied into one or more catcache entries.) */ + elog(LOG, "RelationCacheInitializePhase3 \"%s\": relid=%u " + "formrdesc tdtypeid=%u, pg_class reltype=%u%s", + RelationGetRelationName(relation), + RelationGetRelid(relation), + relation->rd_att->tdtypeid, + relp->reltype, + (relation->rd_att->tdtypeid != relp->reltype) + ? " MISMATCH" : ""); Assert(relation->rd_att->tdtypeid == relp->reltype); Assert(relation->rd_att->tdtypmod == -1); On an existing cluster with patched binaries it shows: formrdesc "pg_parameter_acl": relid=0 reltype=2173 tdtypeid=2173 RelationCacheInitializePhase3 "pg_parameter_acl": relid=6243 formrdesc tdtypeid=2173, pg_class reltype=10097 MISMATCH -- *Mark Dilger* ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: GUC parameter ACLs and physical walsender @ 2026-04-23 21:06 Jeff Davis <[email protected]> parent: Mark Dilger <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Jeff Davis @ 2026-04-23 21:06 UTC (permalink / raw) To: Mark Dilger <[email protected]>; John Naylor <[email protected]>; +Cc: pgsql-hackers; Andrey Borodin <[email protected]> On Thu, 2026-04-23 at 10:57 -0700, Mark Dilger wrote: > John is right that the hardcoded BKI_ROWTYPE_OID(2173) makes this > non-backportable as-is. Right, but that leaves the questions: (a) Is this the right fix for master? (b) Is there anything we can do in the back branches, or we just leave it as fix going forward only? Regards, Jeff Davis ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: GUC parameter ACLs and physical walsender @ 2026-04-23 23:41 Mark Dilger <[email protected]> parent: Jeff Davis <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Mark Dilger @ 2026-04-23 23:41 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: John Naylor <[email protected]>; pgsql-hackers; Andrey Borodin <[email protected]> On Thu, Apr 23, 2026 at 2:06 PM Jeff Davis <[email protected]> wrote: > On Thu, 2026-04-23 at 10:57 -0700, Mark Dilger wrote: > > John is right that the hardcoded BKI_ROWTYPE_OID(2173) makes this > > non-backportable as-is. > > Right, but that leaves the questions: > > (a) Is this the right fix for master? > Yes. This approach has no problem in master that I can see. > (b) Is there anything we can do in the back branches, or we just leave > it as fix going forward only? > I don't see a solution. We could try to replace the error message with something better, but even that seems hard to phrase. Replacing "cannot read pg_class without having selected a database" with, say, "permission denied" would also be confusing for a role which does have the privilege but just can't verify it. -- *Mark Dilger* ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: GUC parameter ACLs and physical walsender @ 2026-04-24 19:30 Jeff Davis <[email protected]> parent: Mark Dilger <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Jeff Davis @ 2026-04-24 19:30 UTC (permalink / raw) To: Mark Dilger <[email protected]>; +Cc: John Naylor <[email protected]>; pgsql-hackers; Andrey Borodin <[email protected]> On Thu, 2026-04-23 at 16:41 -0700, Mark Dilger wrote: > On Thu, Apr 23, 2026 at 2:06 PM Jeff Davis <[email protected]> wrote: > > (a) Is this the right fix for master? > > Yes. This approach has no problem in master that I can see. > > > (b) Is there anything we can do in the back branches, or we just > > leave > > it as fix going forward only? > > I don't see a solution. OK. I'll wait a couple days to see if others have input on these two points, and then I can commit this to master only. Ideally before 19beta1, because it would bump the catversion. That would leave a bug in the back branches, but not a very serious one as far as I can tell. Regards, Jeff Davis ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-04-24 19:30 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-04-22 19:18 GUC parameter ACLs and physical walsender Jeff Davis <[email protected]> 2026-04-23 03:27 ` John Naylor <[email protected]> 2026-04-23 17:57 ` Mark Dilger <[email protected]> 2026-04-23 21:06 ` Jeff Davis <[email protected]> 2026-04-23 23:41 ` Mark Dilger <[email protected]> 2026-04-24 19:30 ` Jeff Davis <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox