On Wed, 2023-08-02 at 01:14 -0400, Isaac Morland wrote:
> > > On Sat, 2023-07-29 at 12:44 -0400, Isaac Morland wrote:
> > > > Essentially, "just" observe efficiently (somehow) that no
> > > > change is
> > > > needed, and skip changing it?
...
> Speaking as someone who uses a lot of stored procedures, many of
> which call one another, I definitely want this optimization.
If we try to avoid the set_config_option() entirely, we'd have to
introduce the invalidation logic into fmgr.c somehow, and there would
be a performance cliff for users who change their session's
search_path.
Instead, in v4 (attached) I introduced a fast path (patch 0004,
experimental) for setting search_path that uses the same low-level
invalidation logic in namespace.c, but avoids most of the overhead of
set_config_option().
(Aside: part of the reason set_config_option() is slow is because of
the lookup in guc_hashtab. That's slower than some other hash lookups
because it does case-folding, which needs to be done in both the hash
function and also the match function. The hash lookups in
SearchPathCache are significantly faster. I also have a patch to change
guc_hashtab to simplehash, but I didn't see much difference.)
New timings (with session's search_path set to a, b):
inc() plain function: 4.1s
inc_secdef() SECURITY DEFINER: 4.3s
inc_wm() SET work_mem = '4GB': 6.3s
inc_ab() SET search_path = a, b: 5.4s
inc_bc() SET search_path = b, c: 5.6s
I reported the numbers a bit differently here. All numbers are using v4
of the patch. The plain function is a baseline; SECURITY DEFINER is for
measuring the overhead of the fmgr_security_definer wrapper; inc_ab()
is for setting the search_path to the same value it's already set to;
and inc_bc() is for setting the search_path to a new value.
There's still a difference between inc() (with no proconfigs) and
inc_ab(), so you can certainly argue that some or all of that can be
avoided if the search_path is unchanged. For instance, adding the new
GUC level causes AtEOXact_GUC to be run, and that does some work. Maybe
it's even possible to avoid the fmgr_security_definer wrapper entirely
(though it would be tricky to do so).
But in general I'd prefer to optimize cases that are going to work
nicely by default for a lot of users (especially switching to a safe
search path), without the need for obscure knowledge about the
performance implications of the session search_path. And to the extent
that we do optimize for the pre-existing search_path, I'd like to
understand the inherent overhead of changing the search path versus
incidental overhead.
--
Jeff Davis
PostgreSQL Contributor Team - AWS
From f45ee8c5ea6517907000d1d3566dcc766ca25732 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Thu, 27 Jul 2023 15:11:42 -0700
Subject: [PATCH v4 1/4] Transform proconfig for faster execution.
Store function config settings as a list of (name, value) pairs to
avoid the need to parse for each function invocation.
---
src/backend/utils/fmgr/fmgr.c | 29 +++++++++++++++-------
src/backend/utils/misc/guc.c | 45 ++++++++++++++++++++++++++++-------
src/include/utils/guc.h | 2 ++
3 files changed, 59 insertions(+), 17 deletions(-)
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index 9208c31fe0..6b2d7d7be3 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -612,7 +612,8 @@ struct fmgr_security_definer_cache
{
FmgrInfo flinfo; /* lookup info for target function */
Oid userid; /* userid to set, or InvalidOid */
- ArrayType *proconfig; /* GUC values to set, or NULL */
+ List *configNames; /* GUC names to set, or NIL */
+ List *configValues; /* GUC values to set, or NIL */
Datum arg; /* passthrough argument for plugin modules */
};
@@ -634,6 +635,8 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
FmgrInfo *save_flinfo;
Oid save_userid;
int save_sec_context;
+ ListCell *lc1;
+ ListCell *lc2;
volatile int save_nestlevel;
PgStat_FunctionCallUsage fcusage;
@@ -666,8 +669,11 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
&isnull);
if (!isnull)
{
+ ArrayType *array;
oldcxt = MemoryContextSwitchTo(fcinfo->flinfo->fn_mcxt);
- fcache->proconfig = DatumGetArrayTypePCopy(datum);
+ array = DatumGetArrayTypeP(datum);
+ TransformGUCArray(array, &fcache->configNames,
+ &fcache->configValues);
MemoryContextSwitchTo(oldcxt);
}
@@ -680,7 +686,7 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
/* GetUserIdAndSecContext is cheap enough that no harm in a wasted call */
GetUserIdAndSecContext(&save_userid, &save_sec_context);
- if (fcache->proconfig) /* Need a new GUC nesting level */
+ if (fcache->configNames != NIL) /* Need a new GUC nesting level */
save_nestlevel = NewGUCNestLevel();
else
save_nestlevel = 0; /* keep compiler quiet */
@@ -689,12 +695,17 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
SetUserIdAndSecContext(fcache->userid,
save_sec_context | SECURITY_LOCAL_USERID_CHANGE);
- if (fcache->proconfig)
+ forboth(lc1, fcache->configNames, lc2, fcache->configValues)
{
- ProcessGUCArray(fcache->proconfig,
- (superuser() ? PGC_SUSET : PGC_USERSET),
- PGC_S_SESSION,
- GUC_ACTION_SAVE);
+ GucContext context = superuser() ? PGC_SUSET : PGC_USERSET;
+ GucSource source = PGC_S_SESSION;
+ GucAction action = GUC_ACTION_SAVE;
+ char *name = lfirst(lc1);
+ char *value = lfirst(lc2);
+
+ (void) set_config_option(name, value,
+ context, source,
+ action, true, 0, false);
}
/* function manager hook */
@@ -737,7 +748,7 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
fcinfo->flinfo = save_flinfo;
- if (fcache->proconfig)
+ if (fcache->configNames != NIL)
AtEOXact_GUC(true, save_nestlevel);
if (OidIsValid(fcache->userid))
SetUserIdAndSecContext(save_userid, save_sec_context);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 5308896c87..fa96b79192 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -6213,16 +6213,13 @@ ParseLongOption(const char *string, char **name, char **value)
*cp = '_';
}
-
/*
- * Handle options fetched from pg_db_role_setting.setconfig,
- * pg_proc.proconfig, etc. Caller must specify proper context/source/action.
- *
- * The array parameter must be an array of TEXT (it must not be NULL).
+ * Transform array of GUC settings into a list of (name, value) pairs. The
+ * list is faster to process in cases where the settings must be applied
+ * repeatedly (e.g. for each function invocation).
*/
void
-ProcessGUCArray(ArrayType *array,
- GucContext context, GucSource source, GucAction action)
+TransformGUCArray(ArrayType *array, List **names, List **values)
{
int i;
@@ -6231,6 +6228,8 @@ ProcessGUCArray(ArrayType *array,
Assert(ARR_NDIM(array) == 1);
Assert(ARR_LBOUND(array)[0] == 1);
+ *names = NIL;
+ *values = NIL;
for (i = 1; i <= ARR_DIMS(array)[0]; i++)
{
Datum d;
@@ -6262,14 +6261,44 @@ ProcessGUCArray(ArrayType *array,
continue;
}
+ *names = lappend(*names, name);
+ *values = lappend(*values, value);
+
+ pfree(s);
+ }
+}
+
+/*
+ * Handle options fetched from pg_db_role_setting.setconfig,
+ * pg_proc.proconfig, etc. Caller must specify proper context/source/action.
+ *
+ * The array parameter must be an array of TEXT (it must not be NULL).
+ */
+void
+ProcessGUCArray(ArrayType *array,
+ GucContext context, GucSource source, GucAction action)
+{
+ List *gucNames;
+ List *gucValues;
+ ListCell *lc1;
+ ListCell *lc2;
+
+ TransformGUCArray(array, &gucNames, &gucValues);
+ forboth(lc1, gucNames, lc2, gucValues)
+ {
+ char *name = lfirst(lc1);
+ char *value = lfirst(lc2);
+
(void) set_config_option(name, value,
context, source,
action, true, 0, false);
pfree(name);
pfree(value);
- pfree(s);
}
+
+ list_free(gucNames);
+ list_free(gucValues);
}
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 223a19f80d..e89083ee0e 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -391,6 +391,8 @@ extern void AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt);
extern char *GetConfigOptionByName(const char *name, const char **varname,
bool missing_ok);
+extern void TransformGUCArray(ArrayType *array, List **configNames,
+ List **configValues);
extern void ProcessGUCArray(ArrayType *array,
GucContext context, GucSource source, GucAction action);
extern ArrayType *GUCArrayAdd(ArrayType *array, const char *name, const char *value);
--
2.34.1
From 313136c308641d2dc47a19330cd134451224918e Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Thu, 27 Jul 2023 15:23:51 -0700
Subject: [PATCH v4 2/4] Optimize check_search_path().
Introduce CheckIdentifierString(), which validates a string list of
identifiers without the work of actually storing them in a list.
This speeds up frequent search_path changes, such as when a function
has search_path set in proconfig. Based on profiling.
---
src/backend/catalog/namespace.c | 13 +-----
src/backend/utils/adt/varlena.c | 70 +++++++++++++++++++++++++++++++++
src/include/utils/varlena.h | 1 +
3 files changed, 73 insertions(+), 11 deletions(-)
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 1f76b5d7f7..f457f778cb 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -4101,19 +4101,13 @@ ResetTempTableNamespace(void)
bool
check_search_path(char **newval, void **extra, GucSource source)
{
- char *rawname;
- List *namelist;
-
- /* Need a modifiable copy of string */
- rawname = pstrdup(*newval);
+ char *rawname = *newval;
/* Parse string into list of identifiers */
- if (!SplitIdentifierString(rawname, ',', &namelist))
+ if (!CheckIdentifierString(rawname, ','))
{
/* syntax error in name list */
GUC_check_errdetail("List syntax is invalid.");
- pfree(rawname);
- list_free(namelist);
return false;
}
@@ -4125,9 +4119,6 @@ check_search_path(char **newval, void **extra, GucSource source)
* requirement is syntactic validity of the identifier list.
*/
- pfree(rawname);
- list_free(namelist);
-
return true;
}
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index b1ec5c32ce..c8d36fd868 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -3427,6 +3427,76 @@ textToQualifiedNameList(text *textval)
return result;
}
+/*
+ * Check that an identifier list is syntactically valid; that is, that there
+ * are no mismatched quotes and that identifiers are separated with the given
+ * separator. Empty identifiers are considered acceptable if (and only if)
+ * quoted.
+ *
+ * Note that an empty string is considered okay here, though not in
+ * textToQualifiedNameList.
+ */
+bool
+CheckIdentifierString(const char *rawstring, char separator)
+{
+ const char *nextp = rawstring;
+ bool done = false;
+
+ while (scanner_isspace(*nextp))
+ nextp++; /* skip leading whitespace */
+
+ if (*nextp == '\0')
+ return true; /* allow empty string */
+
+ /* At the top of the loop, we are at start of a new identifier. */
+ do
+ {
+ if (*nextp == '"')
+ {
+ for (;;)
+ {
+ nextp = strchr(nextp + 1, '"');
+ if (nextp == NULL)
+ return false; /* mismatched quotes */
+ if (nextp[1] != '"')
+ break; /* found end of quoted name */
+ nextp++;
+ }
+
+ nextp++;
+ }
+ else
+ {
+ /* Unquoted name --- extends to separator or whitespace */
+ const char *curname = nextp;
+ while (*nextp && *nextp != separator &&
+ !scanner_isspace(*nextp))
+ nextp++;
+ if (curname == nextp)
+ return false; /* empty unquoted name not allowed */
+ }
+
+ while (scanner_isspace(*nextp))
+ nextp++; /* skip trailing whitespace */
+
+ if (*nextp == separator)
+ {
+ nextp++;
+ while (scanner_isspace(*nextp))
+ nextp++; /* skip leading whitespace for next */
+ /* we expect another name, so done remains false */
+ }
+ else if (*nextp == '\0')
+ done = true;
+ else
+ return false; /* invalid syntax */
+
+ /* Loop back if we didn't reach end of string */
+ } while (!done);
+
+ return true;
+}
+
/*
* SplitIdentifierString --- parse a string containing identifiers
*
diff --git a/src/include/utils/varlena.h b/src/include/utils/varlena.h
index 77f5b24735..4ff0cecc91 100644
--- a/src/include/utils/varlena.h
+++ b/src/include/utils/varlena.h
@@ -27,6 +27,7 @@ extern int varstr_levenshtein_less_equal(const char *source, int slen,
int ins_c, int del_c, int sub_c,
int max_d, bool trusted);
extern List *textToQualifiedNameList(text *textval);
+extern bool CheckIdentifierString(const char *rawstring, char separator);
extern bool SplitIdentifierString(char *rawstring, char separator,
List **namelist);
extern bool SplitDirectoriesString(char *rawstring, char separator,
--
2.34.1
From 8f704c05d8f8538807c8430bc32fba2e043c8fb2 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Wed, 26 Jul 2023 12:55:09 -0700
Subject: [PATCH v4 3/4] Add cache for recomputeNamespacePath().
When search_path is changed to something that was previously set, and
no invalidation happened in between, use the cached list of namespace
OIDs rather than recomputing them. This avoids syscache lookups and
ACL checks.
Important when the search_path changes frequently, such as when set in
proconfig.
---
src/backend/catalog/namespace.c | 349 ++++++++++++++++++++++++++------
1 file changed, 286 insertions(+), 63 deletions(-)
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index f457f778cb..89a1c0bf98 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -41,6 +41,7 @@
#include "catalog/pg_ts_template.h"
#include "catalog/pg_type.h"
#include "commands/dbcommands.h"
+#include "common/hashfn.h"
#include "funcapi.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
@@ -109,11 +110,12 @@
* activeSearchPath is always the actually active path; it points to
* to baseSearchPath which is the list derived from namespace_search_path.
*
- * If baseSearchPathValid is false, then baseSearchPath (and other
- * derived variables) need to be recomputed from namespace_search_path.
- * We mark it invalid upon an assignment to namespace_search_path or receipt
- * of a syscache invalidation event for pg_namespace. The recomputation
- * is done during the next lookup attempt.
+ * If baseSearchPathValid is false, then baseSearchPath (and other derived
+ * variables) need to be recomputed from namespace_search_path, or retrieved
+ * from the search path cache if there haven't been any syscache
+ * invalidations. We mark it invalid upon an assignment to
+ * namespace_search_path or receipt of a syscache invalidation event for
+ * pg_namespace. The recomputation is done during the next lookup attempt.
*
* Any namespaces mentioned in namespace_search_path that are not readable
* by the current user ID are simply left out of baseSearchPath; so
@@ -153,6 +155,35 @@ static Oid namespaceUser = InvalidOid;
/* The above four values are valid only if baseSearchPathValid */
static bool baseSearchPathValid = true;
+static bool searchPathCacheValid = false;
+static MemoryContext SearchPathCacheContext = NULL;
+
+typedef struct SearchPathCacheKey
+{
+ const char *searchPath;
+ Oid roleid;
+} SearchPathCacheKey;
+
+typedef struct SearchPathCacheEntry
+{
+ SearchPathCacheKey *key;
+ List *oidlist; /* namespace OIDs that pass ACL checks */
+ List *finalPath; /* cached final computed search path */
+ Oid firstNS; /* first explicitly-listed namespace */
+ bool temp_missing;
+
+ /*
+ * If true, some namespaces were previously filtered out of finalPath by a
+ * namespace search hook. The next time it's read from the cache, we need
+ * to recreate finalPath from the oidlist.
+ */
+ bool hook_filtered;
+
+ /* needed for simplehash */
+ char status;
+} SearchPathCacheEntry;
+
+static SearchPathCacheEntry *CurrentSearchPathCacheEntry = NULL;
/*
* myTempNamespace is InvalidOid until and unless a TEMP namespace is set up
@@ -193,6 +224,45 @@ static bool MatchNamedCall(HeapTuple proctup, int nargs, List *argnames,
bool include_out_arguments, int pronargs,
int **argnumbers);
+/*
+ * Recomputing the namespace path can be costly when done frequently, such as
+ * when a function has search_path set in proconfig. Add a cache that can be
+ * used when the search_path changes to a value that was previously set, and
+ * no invalidation intervened.
+ *
+ * The cache is a simple hash table in its own memory context, with a key of
+ * search_path and role ID.
+ */
+
+static inline uint32
+nspcachekey_hash(SearchPathCacheKey *key)
+{
+ const unsigned char *bytes = (const unsigned char *)key->searchPath;
+ int blen = strlen(key->searchPath);
+
+ return hash_combine(hash_bytes(bytes, blen),
+ hash_uint32(key->roleid));
+}
+
+static inline bool
+nspcachekey_equal(SearchPathCacheKey *a, SearchPathCacheKey *b)
+{
+ return a->roleid == b->roleid &&
+ strcmp(a->searchPath, b->searchPath) == 0;
+}
+
+#define SH_PREFIX nsphash
+#define SH_ELEMENT_TYPE SearchPathCacheEntry
+#define SH_KEY_TYPE SearchPathCacheKey *
+#define SH_KEY key
+#define SH_HASH_KEY(tb, key) nspcachekey_hash(key)
+#define SH_EQUAL(tb, a, b) nspcachekey_equal(a, b)
+#define SH_SCOPE static inline
+#define SH_DECLARE
+#define SH_DEFINE
+#include "lib/simplehash.h"
+
+static nsphash_hash *SearchPathCache = NULL;
/*
* RangeVarGetRelidExtended
@@ -3370,6 +3440,7 @@ SetTempNamespaceState(Oid tempNamespaceId, Oid tempToastNamespaceId)
*/
baseSearchPathValid = false; /* may need to rebuild list */
+ searchPathCacheValid = false;
}
@@ -3633,28 +3704,19 @@ FindDefaultConversionProc(int32 for_encoding, int32 to_encoding)
}
/*
- * recomputeNamespacePath - recompute path derived variables if needed.
+ * Look up namespace IDs and perform ACL checks. Return newly-allocated list.
*/
-static void
-recomputeNamespacePath(void)
+static List *
+preprocessNamespacePath(const char *searchPath, Oid roleid,
+ bool *temp_missing)
{
- Oid roleid = GetUserId();
char *rawname;
List *namelist;
List *oidlist;
- List *newpath;
ListCell *l;
- bool temp_missing;
- Oid firstNS;
- bool pathChanged;
- MemoryContext oldcxt;
- /* Do nothing if path is already valid. */
- if (baseSearchPathValid && namespaceUser == roleid)
- return;
-
- /* Need a modifiable copy of namespace_search_path string */
- rawname = pstrdup(namespace_search_path);
+ /* Need a modifiable copy */
+ rawname = pstrdup(searchPath);
/* Parse string into list of identifiers */
if (!SplitIdentifierString(rawname, ',', &namelist))
@@ -3671,7 +3733,7 @@ recomputeNamespacePath(void)
* already been accepted.) Don't make duplicate entries, either.
*/
oidlist = NIL;
- temp_missing = false;
+ *temp_missing = false;
foreach(l, namelist)
{
char *curname = (char *) lfirst(l);
@@ -3691,10 +3753,8 @@ recomputeNamespacePath(void)
namespaceId = get_namespace_oid(rname, true);
ReleaseSysCache(tuple);
if (OidIsValid(namespaceId) &&
- !list_member_oid(oidlist, namespaceId) &&
object_aclcheck(NamespaceRelationId, namespaceId, roleid,
- ACL_USAGE) == ACLCHECK_OK &&
- InvokeNamespaceSearchHook(namespaceId, false))
+ ACL_USAGE) == ACLCHECK_OK)
oidlist = lappend_oid(oidlist, namespaceId);
}
}
@@ -3702,16 +3762,12 @@ recomputeNamespacePath(void)
{
/* pg_temp --- substitute temp namespace, if any */
if (OidIsValid(myTempNamespace))
- {
- if (!list_member_oid(oidlist, myTempNamespace) &&
- InvokeNamespaceSearchHook(myTempNamespace, false))
- oidlist = lappend_oid(oidlist, myTempNamespace);
- }
+ oidlist = lappend_oid(oidlist, myTempNamespace);
else
{
/* If it ought to be the creation namespace, set flag */
if (oidlist == NIL)
- temp_missing = true;
+ *temp_missing = true;
}
}
else
@@ -3719,63 +3775,218 @@ recomputeNamespacePath(void)
/* normal namespace reference */
namespaceId = get_namespace_oid(curname, true);
if (OidIsValid(namespaceId) &&
- !list_member_oid(oidlist, namespaceId) &&
object_aclcheck(NamespaceRelationId, namespaceId, roleid,
- ACL_USAGE) == ACLCHECK_OK &&
- InvokeNamespaceSearchHook(namespaceId, false))
+ ACL_USAGE) == ACLCHECK_OK)
oidlist = lappend_oid(oidlist, namespaceId);
}
}
+ pfree(rawname);
+ list_free(namelist);
+
+ return oidlist;
+}
+
+/*
+ * Remove duplicates, run namespace search hooks, and prepend
+ * implicitly-searched namespaces. Return newly-allocated list.
+ *
+ * All of this must happen after retrieving from cache, because we don't know
+ * what might invalidate the result from the last time the hook was
+ * invoked. It may seem that duplicate elimination is not dependent on the
+ * result of the hook, but if a hook returns different results on different
+ * calls for the same namespace ID, then it could affect the order in which
+ * that namespace appears in the final list.
+ */
+static List *
+finalNamespacePath(List *oidlist, Oid *firstNS, bool *hook_filtered)
+{
+ List *finalPath = NIL;
+ ListCell *lc;
+
+ *hook_filtered = false;
+ foreach(lc, oidlist)
+ {
+ Oid namespaceId = lfirst_oid(lc);
+ if (!list_member_oid(finalPath, namespaceId))
+ {
+ if (InvokeNamespaceSearchHook(namespaceId, false))
+ finalPath = lappend_oid(finalPath, namespaceId);
+ else
+ *hook_filtered = true;
+ }
+ }
+
/*
* Remember the first member of the explicit list. (Note: this is
* nominally wrong if temp_missing, but we need it anyway to distinguish
* explicit from implicit mention of pg_catalog.)
*/
- if (oidlist == NIL)
- firstNS = InvalidOid;
+ if (finalPath == NIL)
+ *firstNS = InvalidOid;
else
- firstNS = linitial_oid(oidlist);
+ *firstNS = linitial_oid(finalPath);
/*
* Add any implicitly-searched namespaces to the list. Note these go on
* the front, not the back; also notice that we do not check USAGE
* permissions for these.
*/
- if (!list_member_oid(oidlist, PG_CATALOG_NAMESPACE))
- oidlist = lcons_oid(PG_CATALOG_NAMESPACE, oidlist);
+ if (!list_member_oid(finalPath, PG_CATALOG_NAMESPACE))
+ finalPath = lcons_oid(PG_CATALOG_NAMESPACE, finalPath);
if (OidIsValid(myTempNamespace) &&
- !list_member_oid(oidlist, myTempNamespace))
- oidlist = lcons_oid(myTempNamespace, oidlist);
+ !list_member_oid(finalPath, myTempNamespace))
+ finalPath = lcons_oid(myTempNamespace, finalPath);
+
+ return finalPath;
+}
+
+/*
+ * Retrieve search path information from the cache; or if not there, fill
+ * it. The returned entry is valid until the next call to this function.
+ *
+ * Invalidation takes two forms: The first is ordinary syscache invalidation
+ * clears the whole cache, and every entry must be rebuilt from the string
+ * form (finding OIDs from the namespace names and performing ACL checks). The
+ * second form of invalidation is when an object_access_hook is present, in
+ * which case we rebuild the entry by calling the hook for each OID in the
+ * oidlist, which is much faster than rebuilding from the string.
+ *
+ * We also determine if the new entry's finalPath (which is a list of OIDs) is equal
+ * to the previous one. It's more efficient to do so in this function, because
+ * we can avoid copying the previous list unless there is a syscache
+ * invalidation (in which case we must copy it before resetting
+ * SearchPathCacheContext).
+ */
+static SearchPathCacheEntry *
+cachedNamespacePath(const char *searchPath, Oid roleid, bool *prevPathEqual)
+{
+ MemoryContext oldcxt;
+ SearchPathCacheKey cachekey;
+ SearchPathCacheEntry *entry;
+ bool found;
+ SearchPathCacheEntry *prevEntry = CurrentSearchPathCacheEntry;
+ List *prevPath = prevEntry ? prevEntry->finalPath : NIL;
+ List *prevPathCopy = NIL;
+
+ /* clear the cache and start over when invalidated */
+ if (!searchPathCacheValid)
+ {
+ /* temporarily save prevPath for comparison after reset */
+ prevPathCopy = list_copy(prevPath);
+ prevPath = prevPathCopy;
+
+ MemoryContextReset(SearchPathCacheContext);
+
+ /* arbitrary initial starting size of 16 elements */
+ SearchPathCache = nsphash_create(SearchPathCacheContext, 16, NULL);
+ searchPathCacheValid = true;
+ prevEntry = NULL;
+ }
+
+ if (prevEntry && roleid == prevEntry->key->roleid &&
+ strcmp(searchPath, prevEntry->key->searchPath) == 0)
+ {
+ /*
+ * If search_path has changed and then changed back before
+ * recomputeNamespacePath is called, then the hash lookup will just
+ * find the very same entry that we already have. Optimize this case.
+ */
+ entry = prevEntry;
+ found = true;
+ }
+ else
+ {
+ /* otherwise do a quick lookup */
+ cachekey.searchPath = searchPath;
+ cachekey.roleid = roleid;
+
+ entry = nsphash_insert(SearchPathCache, &cachekey, &found);
+ }
+
+ /* initialize entry, though don't set finalPath (etc.) */
+ if (!found)
+ {
+ SearchPathCacheKey *newkey;
+
+ oldcxt = MemoryContextSwitchTo(SearchPathCacheContext);
+
+ newkey = palloc(sizeof(SearchPathCacheKey));
+ newkey->searchPath = pstrdup(searchPath);
+ newkey->roleid = roleid;
+
+ entry->key = newkey;
+ entry->oidlist = preprocessNamespacePath(searchPath, roleid,
+ &entry->temp_missing);
+ entry->finalPath = finalNamespacePath(entry->oidlist,
+ &entry->firstNS,
+ &entry->hook_filtered);
+ MemoryContextSwitchTo(oldcxt);
+ }
/*
- * We want to detect the case where the effective value of the base search
- * path variables didn't change. As long as we're doing so, we can avoid
- * copying the OID list unnecessarily.
+ * If a hook is set now, or if a previously-set hook filtered out some
+ * namespaces from finalPath, we must re-finalize.
*/
- if (baseCreationNamespace == firstNS &&
- baseTempCreationPending == temp_missing &&
- equal(oidlist, baseSearchPath))
+ if (found && (object_access_hook || entry->hook_filtered))
+ {
+ /*
+ * Don't free the existing entry->finalPath yet, as prevPath may be an
+ * alias for it. Treat it as a copy that will be freed later.
+ */
+ Assert(prevPathCopy == NIL);
+ prevPathCopy = entry->finalPath;
+
+ oldcxt = MemoryContextSwitchTo(SearchPathCacheContext);
+ entry->finalPath = finalNamespacePath(entry->oidlist,
+ &entry->firstNS,
+ &entry->hook_filtered);
+ MemoryContextSwitchTo(oldcxt);
+ }
+
+ *prevPathEqual = equal(prevPath, entry->finalPath);
+ list_free(prevPathCopy);
+
+ CurrentSearchPathCacheEntry = entry;
+
+ return entry;
+}
+
+/*
+ * recomputeNamespacePath - recompute path derived variables if needed.
+ */
+static void
+recomputeNamespacePath(void)
+{
+ Oid roleid = GetUserId();
+ bool prevPathEqual;
+ bool pathChanged;
+ SearchPathCacheEntry *entry;
+
+ /* Do nothing if path is already valid. */
+ if (baseSearchPathValid && namespaceUser == roleid)
+ return;
+
+ entry = cachedNamespacePath(namespace_search_path,
+ roleid, &prevPathEqual);
+
+ if (baseCreationNamespace == entry->firstNS &&
+ baseTempCreationPending == entry->temp_missing &&
+ prevPathEqual)
{
pathChanged = false;
}
else
{
pathChanged = true;
-
- /* Must save OID list in permanent storage. */
- oldcxt = MemoryContextSwitchTo(TopMemoryContext);
- newpath = list_copy(oidlist);
- MemoryContextSwitchTo(oldcxt);
-
- /* Now safe to assign to state variables. */
- list_free(baseSearchPath);
- baseSearchPath = newpath;
- baseCreationNamespace = firstNS;
- baseTempCreationPending = temp_missing;
}
+ /* Now safe to assign to state variables. */
+ baseSearchPath = entry->finalPath;
+ baseCreationNamespace = entry->firstNS;
+ baseTempCreationPending = entry->temp_missing;
+
/* Mark the path valid. */
baseSearchPathValid = true;
namespaceUser = roleid;
@@ -3791,11 +4002,6 @@ recomputeNamespacePath(void)
*/
if (pathChanged)
activePathGeneration++;
-
- /* Clean up. */
- pfree(rawname);
- list_free(namelist);
- list_free(oidlist);
}
/*
@@ -3950,6 +4156,7 @@ InitTempTableNamespace(void)
myTempNamespaceSubID = GetCurrentSubTransactionId();
baseSearchPathValid = false; /* need to rebuild list */
+ searchPathCacheValid = false;
}
/*
@@ -3975,6 +4182,7 @@ AtEOXact_Namespace(bool isCommit, bool parallel)
myTempNamespace = InvalidOid;
myTempToastNamespace = InvalidOid;
baseSearchPathValid = false; /* need to rebuild list */
+ searchPathCacheValid = false;
/*
* Reset the temporary namespace flag in MyProc. We assume that
@@ -4016,6 +4224,7 @@ AtEOSubXact_Namespace(bool isCommit, SubTransactionId mySubid,
myTempNamespace = InvalidOid;
myTempToastNamespace = InvalidOid;
baseSearchPathValid = false; /* need to rebuild list */
+ searchPathCacheValid = false;
/*
* Reset the temporary namespace flag in MyProc. We assume that
@@ -4130,6 +4339,10 @@ assign_search_path(const char *newval, void *extra)
* We mark the path as needing recomputation, but don't do anything until
* it's needed. This avoids trying to do database access during GUC
* initialization, or outside a transaction.
+ *
+ * This does not invalidate the search path cache, so if this value had
+ * been previously set and no syscache invalidations happened,
+ * recomputation may not be necessary.
*/
baseSearchPathValid = false;
}
@@ -4164,6 +4377,10 @@ InitializeSearchPath(void)
}
else
{
+ SearchPathCacheContext = AllocSetContextCreate(
+ TopMemoryContext, "search_path processing cache",
+ ALLOCSET_DEFAULT_SIZES);
+
/*
* In normal mode, arrange for a callback on any syscache invalidation
* of pg_namespace rows.
@@ -4173,6 +4390,7 @@ InitializeSearchPath(void)
(Datum) 0);
/* Force search path to be recomputed on next use */
baseSearchPathValid = false;
+ searchPathCacheValid = false;
}
}
@@ -4183,8 +4401,13 @@ InitializeSearchPath(void)
static void
NamespaceCallback(Datum arg, int cacheid, uint32 hashvalue)
{
- /* Force search path to be recomputed on next use */
+ /*
+ * Force search path to be recomputed on next use, also invalidating the
+ * search path cache (because namespace names, ACLs, or role names may
+ * have changed).
+ */
baseSearchPathValid = false;
+ searchPathCacheValid = false;
}
/*
--
2.34.1
From 08d688a4346a5bb44f702ebbd46f75d5fc7bc01d Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Mon, 7 Aug 2023 11:12:07 -0700
Subject: [PATCH v4 4/4] wip
---
src/backend/utils/fmgr/fmgr.c | 9 +++--
src/backend/utils/misc/guc.c | 63 +++++++++++++++++++++++++++++++++++
src/include/utils/guc.h | 1 +
3 files changed, 70 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index 6b2d7d7be3..40c6137b01 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -703,9 +703,12 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
char *name = lfirst(lc1);
char *value = lfirst(lc2);
- (void) set_config_option(name, value,
- context, source,
- action, true, 0, false);
+ if (!strcmp(name, "search_path"))
+ fast_set_search_path(value);
+ else
+ (void) set_config_option(name, value,
+ context, source,
+ action, true, 0, false);
}
/* function manager hook */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index fa96b79192..a34a81217f 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -46,6 +46,7 @@
#include "utils/builtins.h"
#include "utils/conffiles.h"
#include "utils/float.h"
+#include "utils/guc_hooks.h"
#include "utils/guc_tables.h"
#include "utils/memutils.h"
#include "utils/timestamp.h"
@@ -225,6 +226,7 @@ static bool reporting_enabled; /* true to enable GUC_REPORT */
static int GUCNestLevel = 0; /* 1 when in main transaction */
+struct config_string *search_path_conf = NULL;
static int guc_var_compare(const void *a, const void *b);
static uint32 guc_name_hash(const void *key, Size keysize);
@@ -4135,6 +4137,67 @@ set_config_option_ext(const char *name, const char *value,
return changeVal ? 1 : -1;
}
+/*
+ * optimized version of:
+ * set_config_option("search_path", searchPath,
+ * superuser() ? PGC_SUSET : PGC_USERSET,
+ PGC_S_SESSION, GUC_ACTION_SAVE, true, 0, false);
+ *
+ */
+void
+fast_set_search_path(const char *searchPath)
+{
+ char *newval = guc_strdup(ERROR, searchPath);
+ void *newextra = NULL;
+ Oid srole = GetUserId();
+ GucContext context = superuser() ? PGC_SUSET : PGC_USERSET;
+ GucSource source = PGC_S_SESSION;
+
+ if (search_path_conf == NULL)
+ {
+ struct config_generic *record;
+ record = find_option("search_path", true, false, ERROR);
+ Assert(record != NULL);
+ search_path_conf = (struct config_string *) record;
+ }
+
+ /* Reset variables that might be set by hook */
+ GUC_check_errcode_value = ERRCODE_INVALID_PARAMETER_VALUE;
+ GUC_check_errmsg_string = NULL;
+ GUC_check_errdetail_string = NULL;
+ GUC_check_errhint_string = NULL;
+
+ if (!check_search_path(&newval, NULL, source))
+ {
+ guc_free(newval);
+ ereport(ERROR,
+ (errcode(GUC_check_errcode_value),
+ GUC_check_errmsg_string ?
+ errmsg_internal("%s", GUC_check_errmsg_string) :
+ errmsg("invalid value for parameter \"%s\": \"%s\"",
+ search_path_conf->gen.name, newval ? newval : ""),
+ GUC_check_errdetail_string ?
+ errdetail_internal("%s", GUC_check_errdetail_string) : 0,
+ GUC_check_errhint_string ?
+ errhint("%s", GUC_check_errhint_string) : 0));
+ }
+
+ assign_search_path(newval, newextra);
+ set_string_field(search_path_conf, search_path_conf->variable, newval);
+ set_extra_field(&search_path_conf->gen, &search_path_conf->gen.extra,
+ newextra);
+ set_guc_source(&search_path_conf->gen, source);
+ search_path_conf->gen.scontext = context;
+ search_path_conf->gen.srole = srole;
+
+ /* Perhaps we didn't install newval anywhere */
+ if (newval && !string_field_used(search_path_conf, newval))
+ guc_free(newval);
+ /* Perhaps we didn't install newextra anywhere */
+ if (newextra && !extra_field_used(&search_path_conf->gen, newextra))
+ guc_free(newextra);
+}
+
/*
* Set the fields for source file and line number the setting came from.
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index e89083ee0e..d916ecf2ed 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -387,6 +387,7 @@ extern int set_config_option_ext(const char *name, const char *value,
Oid srole,
GucAction action, bool changeVal, int elevel,
bool is_reload);
+extern void fast_set_search_path(const char *searchPath);
extern void AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt);
extern char *GetConfigOptionByName(const char *name, const char **varname,
bool missing_ok);
--
2.34.1
