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
From 1b24d08302e268370ebb14e732f4d5f1f1407726 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@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.nmi...@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.c
index 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
