On Tue, 2023-12-05 at 11:58 -0800, Jeff Davis wrote:
> Also, I forward-declared config_generic in guc.h to eliminate the
> cast.
Looking more closely, I fixed an issue related to placeholder configs.
We can't return a handle to a placeholder, because it's not stable, so
in that case it falls back to using the name.
My apologies for the churn on this (mostly) simple patch. I think this
version is correct; I intend to commit soon.
Regards,
Jeff Davis
From 386100494f3bb977e5b9e52fa603898587b4d976 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Mon, 4 Dec 2023 16:20:05 -0800
Subject: [PATCH v3] Cache opaque handle for GUC option to avoid repeasted
lookups.
When setting GUCs from proconfig, performance is important, and hash
lookups in the GUC table are significant.
Per suggestion from Robert Haas.
Discussion: https://postgr.es/m/ca+tgmoypkxhr3hod9syk2xwcauvpa0+ba0xpnwwbcyxtklk...@mail.gmail.com
Reviewed-by: John Naylor
---
src/backend/utils/fmgr/fmgr.c | 39 ++++++++++++++++-----
src/backend/utils/misc/guc.c | 66 ++++++++++++++++++++++++++++++++---
src/include/utils/guc.h | 8 +++++
3 files changed, 101 insertions(+), 12 deletions(-)
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index 9dfdf890c5..442d7b8269 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -613,6 +613,7 @@ struct fmgr_security_definer_cache
FmgrInfo flinfo; /* lookup info for target function */
Oid userid; /* userid to set, or InvalidOid */
List *configNames; /* GUC names to set, or NIL */
+ List *configHandles; /* GUC handles to set, or NIL */
List *configValues; /* GUC values to set, or NIL */
Datum arg; /* passthrough argument for plugin modules */
};
@@ -635,8 +636,9 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
FmgrInfo *save_flinfo;
Oid save_userid;
int save_sec_context;
- ListCell *lc1;
- ListCell *lc2;
+ ListCell *lc1,
+ *lc2,
+ *lc3;
volatile int save_nestlevel;
PgStat_FunctionCallUsage fcusage;
@@ -670,11 +672,23 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
if (!isnull)
{
ArrayType *array;
+ ListCell *lc;
oldcxt = MemoryContextSwitchTo(fcinfo->flinfo->fn_mcxt);
array = DatumGetArrayTypeP(datum);
TransformGUCArray(array, &fcache->configNames,
&fcache->configValues);
+
+ /* transform names to config handles to avoid lookup cost */
+ fcache->configHandles = NIL;
+ foreach(lc, fcache->configNames)
+ {
+ char *name = (char *) lfirst(lc);
+
+ fcache->configHandles = lappend(fcache->configHandles,
+ get_config_handle(name));
+ }
+
MemoryContextSwitchTo(oldcxt);
}
@@ -696,17 +710,26 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
SetUserIdAndSecContext(fcache->userid,
save_sec_context | SECURITY_LOCAL_USERID_CHANGE);
- forboth(lc1, fcache->configNames, lc2, fcache->configValues)
+ forthree(lc1, fcache->configNames,
+ lc2, fcache->configHandles,
+ lc3, fcache->configValues)
{
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);
+ config_handle *handle = lfirst(lc2);
+ char *value = lfirst(lc3);
+
+ /* handle not available if GUC is a placeholder */
+ if (handle)
+ (void) set_config_with_handle(handle, value,
+ context, source, GetUserId(),
+ action, true, 0, false);
+ 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 e76c083003..4ba6ea6c83 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3357,6 +3357,52 @@ set_config_option_ext(const char *name, const char *value,
bool is_reload)
{
struct config_generic *record;
+
+ if (elevel == 0)
+ {
+ if (source == PGC_S_DEFAULT || source == PGC_S_FILE)
+ {
+ /*
+ * To avoid cluttering the log, only the postmaster bleats loudly
+ * about problems with the config file.
+ */
+ elevel = IsUnderPostmaster ? DEBUG3 : LOG;
+ }
+ else if (source == PGC_S_GLOBAL ||
+ source == PGC_S_DATABASE ||
+ source == PGC_S_USER ||
+ source == PGC_S_DATABASE_USER)
+ elevel = WARNING;
+ else
+ elevel = ERROR;
+ }
+
+ record = find_option(name, true, false, elevel);
+ if (record == NULL)
+ return 0;
+
+ return set_config_with_handle(record, value,
+ context, source, srole,
+ action, changeVal, elevel,
+ is_reload);
+}
+
+
+/*
+ * set_config_with_handle: sets option with the given handle to the given
+ * value.
+ *
+ * This should be used by callers that repeatedly set the same config
+ * option(s), and want to avoid the overhead of a hash lookup each time.
+ */
+int
+set_config_with_handle(config_handle * handle, const char *value,
+ GucContext context, GucSource source, Oid srole,
+ GucAction action, bool changeVal, int elevel,
+ bool is_reload)
+{
+ struct config_generic *record = handle;
+ const char *name = record->name;
union config_var_val newval_union;
void *newextra = NULL;
bool prohibitValueChange = false;
@@ -3395,10 +3441,6 @@ set_config_option_ext(const char *name, const char *value,
(errcode(ERRCODE_INVALID_TRANSACTION_STATE),
errmsg("cannot set parameters during a parallel operation")));
- record = find_option(name, true, false, elevel);
- if (record == NULL)
- return 0;
-
/*
* Check if the option can be set at this time. See guc.h for the precise
* rules.
@@ -4166,6 +4208,22 @@ set_config_option_ext(const char *name, const char *value,
}
+/*
+ * Retrieve a config_handle for the given name, suitable for calling
+ * set_config_with_handle(). Only return handle to permanent GUC.
+ */
+config_handle *
+get_config_handle(const char *name)
+{
+ struct config_generic *gen = find_option(name, false, false, 0);
+
+ if (gen && ((gen->flags & GUC_CUSTOM_PLACEHOLDER) == 0))
+ return gen;
+
+ return NULL;
+}
+
+
/*
* 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 20fe13702b..a5a19ca6a1 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -144,6 +144,8 @@ typedef struct ConfigVariable
struct ConfigVariable *next;
} ConfigVariable;
+typedef struct config_generic config_handle;
+
extern bool ParseConfigFile(const char *config_file, bool strict,
const char *calling_file, int calling_lineno,
int depth, int elevel,
@@ -387,6 +389,12 @@ extern int set_config_option_ext(const char *name, const char *value,
Oid srole,
GucAction action, bool changeVal, int elevel,
bool is_reload);
+extern int set_config_with_handle(config_handle * handle, const char *value,
+ GucContext context, GucSource source,
+ Oid srole,
+ GucAction action, bool changeVal,
+ int elevel, bool is_reload);
+extern config_handle * get_config_handle(const char *name);
extern void AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt);
extern char *GetConfigOptionByName(const char *name, const char **varname,
bool missing_ok);
--
2.34.1
