From: Jeff Davis <pgsql@j-davis.com>
To: Noah Misch <noah@leadboat.com>
Cc: Nathan Bossart <nathandbossart@gmail.com>
Cc: Isaac Morland <isaac.morland@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Faster "SET search_path"
Date: Mon, 08 Jul 2024 16:39:21 -0700
Message-ID: <cbb4ba82320a66c1e4a7c8fb3aa189ed9fd3908a.camel@j-davis.com> (raw)
In-Reply-To: <20240630223047.1f.nmisch@google.com>
References: <CAMsGm5fZ2xiVDLYH4-_Ld8bfe1qwY23ZLgPc7zBpH8d1WUw70A@mail.gmail.com>
<f555c41554c7f5ff2699c085836ea94beac09bad.camel@j-davis.com>
<20230802050720.GB866741@nathanxps13>
<CAMsGm5dt6szoTe5oAzk5nfbHHOEpMDVhLTOGWCJGCQ3Wq8+J0w@mail.gmail.com>
<fec7f3bea50112323d9b6e700b5e2762a14af809.camel@j-davis.com>
<20230807223908.GB1284531@nathanxps13>
<3b2d57ead15798adcbf6a53a15e35cdbadb2c147.camel@j-davis.com>
<5bb4d9c21cc918e5d05021bf0d5e0e7d34a0377c.camel@j-davis.com>
<abf4ce8804e0e05dff8c1725ae6a8ed28b7d66e0.camel@j-davis.com>
<695668acc40b937258985669f3d55b94cce82c3c.camel@j-davis.com>
<20240630223047.1f.nmisch@google.com>
On Sun, 2024-06-30 at 15:30 -0700, Noah Misch wrote:
> You're caching the result of object_aclcheck(NamespaceRelationId,
> ...), so
> pg_auth_members changes
Thank you for the report.
Question: to check for changes to pg_auth_members, it seems like either
AUTHMEMROLEMEM or AUTHMEMMEMROLE work, and to use both would be
redundant. Am I missing something, or should I just pick one
arbitrarily (or by some convention)?
> and pg_database changes also need to invalidate this
> cache. (pg_database affects the ACL_CREATE_TEMP case in
> pg_namespace_aclmask_ext()
I am having trouble finding an actual problem with ACL_CREATE_TEMP.
search_path ACL checks are normally bypassed for the temp namespace, so
it can only happen when the actual temp namespace name (e.g.
pg_temp_NNN) is explicitly included. In that case, the mask is
ACL_USAGE, so the two branches in pg_namespace_aclmask_ext() are
equivalent, right?
This question is not terribly important for the fix, because
invalidating for each pg_database change is still necessary as you
point out in here:
> and affects ROLE_PG_DATABASE_OWNER membership.)
Another good catch, thank you.
Patch attached. Contains a bit of cleanup and is intended for 17+18.
Regards,
Jeff Davis
Attachments:v1-0001-Fix-missing-invalidations-for-search_path-cache.patch (text/x-patch, 2.6K) download
From 1b24d08302e268370ebb14e732f4d5f1f1407726 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Mon, 8 Jul 2024 15:52:50 -0700
Subject: [PATCH v1] Fix missing invalidations for search_path cache.
Reported-by: Noah Misch
Discussion: https://postgr.es/m/20240630223047.1f.nmisch@google.com
Backpatch-through: 17
---
src/backend/catalog/namespace.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.cindex a2510cf80c6..43b707699d7 100644--- a/src/backend/catalog/namespace.c+++ b/src/backend/catalog/namespace.c@@ -230,7 +230,7 @@ static void AccessTempTableNamespace(bool force);
static void InitTempTableNamespace(void);
static void RemoveTempRelations(Oid tempNamespaceId);
static void RemoveTempRelationsCallback(int code, Datum arg);
-static void NamespaceCallback(Datum arg, int cacheid, uint32 hashvalue);+static void InvalidationCallback(Datum arg, int cacheid, uint32 hashvalue);
static bool MatchNamedCall(HeapTuple proctup, int nargs, List *argnames,
bool include_out_arguments, int pronargs,
int **argnumbers);
@@ -4749,15 +4749,29 @@ InitializeSearchPath(void)
/*
* In normal mode, arrange for a callback on any syscache invalidation
- * of pg_namespace or pg_authid rows. (Changing a role name may affect- * the meaning of the special string $user.)+ * that will affect the search_path cache.
*/
++ /* namespace name or ACLs may have changed */
CacheRegisterSyscacheCallback(NAMESPACEOID,
- NamespaceCallback,+ InvalidationCallback,
(Datum) 0);
++ /* role name may affect the meaning of "$user" */
CacheRegisterSyscacheCallback(AUTHOID,
- NamespaceCallback,+ InvalidationCallback,+ (Datum) 0);++ /* role membership may affect ACLs */+ CacheRegisterSyscacheCallback(AUTHMEMROLEMEM,+ InvalidationCallback,
(Datum) 0);
++ /* database owner may affect ACLs */+ CacheRegisterSyscacheCallback(DATABASEOID,+ InvalidationCallback,+ (Datum) 0);+
/* Force search path to be recomputed on next use */
baseSearchPathValid = false;
searchPathCacheValid = false;
@@ -4765,11 +4779,11 @@ InitializeSearchPath(void)
}
/*
- * NamespaceCallback+ * InvalidationCallback
* Syscache inval callback function
*/
static void
-NamespaceCallback(Datum arg, int cacheid, uint32 hashvalue)+InvalidationCallback(Datum arg, int cacheid, uint32 hashvalue)
{
/*
* Force search path to be recomputed on next use, also invalidating the
--
2.34.1
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-hackers@postgresql.org
Cc: pgsql@j-davis.com, noah@leadboat.com, nathandbossart@gmail.com, isaac.morland@gmail.com
Subject: Re: Faster "SET search_path"
In-Reply-To: <cbb4ba82320a66c1e4a7c8fb3aa189ed9fd3908a.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 DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox