At Thu, 15 Mar 2018 15:09:54 +0900, Michael Paquier <mich...@paquier.xyz> wrote in <20180315060954.ge...@paquier.xyz> > On Thu, Mar 15, 2018 at 02:03:15PM +0900, Kyotaro HORIGUCHI wrote: > > At Thu, 15 Mar 2018 00:33:25 -0400, Tom Lane <t...@sss.pgh.pa.us> wrote in > > <22193.1521088...@sss.pgh.pa.us> > >> Kyotaro HORIGUCHI <horiguchi.kyot...@lab.ntt.co.jp> writes: > >>> Doesn't it make sense if we provide a buildtime-script that > >>> collects the function names and builds a .h file containing a > >>> function using the list? > >> > >> Surely this is a fundamentally misguided approach. How could it > >> handle extension GUCs? > > > > I understand it is "out of scope" of this thread (for now). The > > starting issue here is "the static list of list-guc's are stale" > > so what a static list cannot cope with is still cannot be coped > > with by this. > > I like the mention of the idea, now it is true that that it would be a > half-baked work without parameters from plpgsql, and a way to correctly > track parameters marking a custom GUC as GUC_INPUT_LIST in all C files. > > > Or, we could cope with this issue if the list-ness of used GUCs > > is stored permanently in somewhere. Maybe pg_proc.proconfigislist > > or something... > > That does not help for PL's GUCs as well. Those may not be loaded when > pg_get_functiondef gets called.
So, we should reject to define function in the case. We don't
accept the GUC element if it is just a placeholder.
The attached is a rush work of my idea. Diff for pg_proc.h is too
large so it is separated and gziped.
It adds a column named "proconfigislist" of array(bool) in
pg_proc. When defined function has set clauses, it generates a
proconfig-is-list-or-not array and set. It ends with error if
required module is not loaded yet. Perhaps
GetConfigOptionFlags(,false) shouldn't return 0 if no option
element is found but I don't amend it for now.
Finally, it works as the follows. I think this leands to a kind
of desired behavior.
======
postgres=# CREATE FUNCTION func_with_set_params() RETURNS integer
AS 'select 1;'
LANGUAGE SQL
set plpgsql.extra_errors to 'shadowed_variables'
set work_mem to '48MB'
set plpgsql.extra_warnings to 'shadowed_variables';
ERROR: the module for variable "plpgsql.extra_errors" is not loaded yet
DETAIL: The module must be loaded before referring this variable.
postgres=# load 'plpgsql';
postgres=# CREATE FUNCTION func_with_set_params() RETURNS integer
AS 'select 1;'
LANGUAGE SQL
set plpgsql.extra_errors to 'shadowed_variables'
set work_mem to '48MB'
set plpgsql.extra_warnings to 'shadowed_variables';
postgres=# select proname, proconfig, proconfigislist from pg_proc where
proconfig is not null;
-[ RECORD 1
]---+--------------------------------------------------------------------------------------------------
proname | func_with_set_params
proconfig |
{plpgsql.extra_errors=shadowed_variables,work_mem=48MB,plpgsql.extra_warnings=shadowed_variables}
proconfigislist | {t,f,t}
=======
pg_get_functiondef() can work correctly with this even if
required modules are not loaded.
But, I suppose it is a bit too big.
> At the end, we are spending a lot of resources on that. So in order to
> close this thread, I would suggest to just complete the list of
> hardcoded parameters on backend and frontend, and add as well a comment
> including "GUC_INPUT_LIST" so as it can be found by grepping the code.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From d48def7a489046e44755bef250f6b35d78339803 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyot...@lab.ntt.co.jp>
Date: Thu, 15 Mar 2018 18:21:50 +0900
Subject: [PATCH 1/2] Store whether elements of proconfig are list or not in
pg_proc.
---
src/backend/catalog/pg_aggregate.c | 1 +
src/backend/catalog/pg_proc.c | 5 +++
src/backend/commands/functioncmds.c | 77 ++++++++++++++++++++++++++++++++++++-
src/backend/commands/proclang.c | 3 ++
src/backend/commands/typecmds.c | 1 +
src/backend/utils/misc/guc.c | 24 ++++++++++++
src/include/catalog/pg_class.h | 2 +-
src/include/catalog/pg_proc_fn.h | 1 +
src/include/utils/guc.h | 1 +
9 files changed, 113 insertions(+), 2 deletions(-)
diff --git a/src/backend/catalog/pg_aggregate.c b/src/backend/catalog/pg_aggregate.c
index 50d8d81f2c..3aaf0eac15 100644
--- a/src/backend/catalog/pg_aggregate.c
+++ b/src/backend/catalog/pg_aggregate.c
@@ -631,6 +631,7 @@ AggregateCreate(const char *aggName,
parameterDefaults, /* parameterDefaults */
PointerGetDatum(NULL), /* trftypes */
PointerGetDatum(NULL), /* proconfig */
+ PointerGetDatum(NULL), /* proconfigislist */
1, /* procost */
0); /* prorows */
procOid = myself.objectId;
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index 40e579f95d..cab0a225bd 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -87,6 +87,7 @@ ProcedureCreate(const char *procedureName,
List *parameterDefaults,
Datum trftypes,
Datum proconfig,
+ Datum proconfigislist,
float4 procost,
float4 prorows)
{
@@ -374,6 +375,10 @@ ProcedureCreate(const char *procedureName,
values[Anum_pg_proc_proconfig - 1] = proconfig;
else
nulls[Anum_pg_proc_proconfig - 1] = true;
+ if (proconfigislist != PointerGetDatum(NULL))
+ values[Anum_pg_proc_proconfigislist - 1] = proconfigislist;
+ else
+ nulls[Anum_pg_proc_proconfigislist - 1] = true;
/* proacl will be determined later */
rel = heap_open(ProcedureRelationId, RowExclusiveLock);
diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index b1f87d056e..d6da508eb3 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -631,6 +631,73 @@ update_proconfig_value(ArrayType *a, List *set_items)
return a;
}
+static ArrayType *
+make_proconfig_islist(ArrayType *proconfig)
+{
+ ArrayType *retarray = NULL;
+ int index;
+ int i;
+ bool isnull;
+ int16 bool_typlen;
+ bool bool_typbyval;
+ bool bool_typalign;
+
+ get_typlenbyvalalign(BOOLOID,
+ &bool_typlen, &bool_typbyval, &bool_typalign);
+
+ index = ARR_DIMS(proconfig)[0];
+
+ for (i = 1 ; i <= index ; i++)
+ {
+ Datum d;
+ char *name;
+ bool is_list = false;
+
+ d = array_ref(proconfig, 1, &i,
+ -1 /* varlenarray */ ,
+ -1 /* TEXT's typlen */ ,
+ false /* TEXT's typbyval */ ,
+ 'i' /* TEXT's typalign */ ,
+ &isnull);
+ if (!isnull)
+ {
+ char *p;
+ int flags;
+
+ name = TextDatumGetCString(d);
+ p = strchr(name, '=');
+ if (p == NULL)
+ elog(ERROR, "something's wrong?");
+ *p = 0;
+
+ flags = GetConfigOptionFlags(name, false);
+ if (flags & GUC_CUSTOM_PLACEHOLDER)
+ ereport(ERROR,
+ (errmsg ("the module for variable \"%s\" is not loaded yet",
+ name),
+ errdetail ("The module must be loaded before referring this variable.")));
+ if (flags & GUC_LIST_INPUT)
+ is_list = true;
+ }
+
+ if (retarray)
+ retarray = array_set(retarray, 1, &i,
+ BoolGetDatum(is_list), false,
+ -1,
+ bool_typlen, bool_typbyval, bool_typalign);
+ else
+ {
+ Datum booldatum = BoolGetDatum(is_list);
+ retarray =
+ construct_array(&booldatum, 1,
+ BOOLOID,
+ bool_typlen, bool_typbyval, bool_typalign);
+ }
+ }
+
+ return retarray;
+}
+
/*
* Dissect the list of options assembled in gram.y into function
@@ -649,6 +716,7 @@ compute_function_attributes(ParseState *pstate,
bool *security_definer,
bool *leakproof_p,
ArrayType **proconfig,
+ ArrayType **proconfigislist,
float4 *procost,
float4 *prorows,
char *parallel_p)
@@ -767,7 +835,10 @@ compute_function_attributes(ParseState *pstate,
if (leakproof_item)
*leakproof_p = intVal(leakproof_item->arg);
if (set_items)
+ {
*proconfig = update_proconfig_value(NULL, set_items);
+ *proconfigislist = make_proconfig_islist(*proconfig);
+ }
if (cost_item)
{
*procost = defGetNumeric(cost_item);
@@ -887,6 +958,7 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
isLeakProof;
char volatility;
ArrayType *proconfig;
+ ArrayType *proconfigislist;
float4 procost;
float4 prorows;
HeapTuple languageTuple;
@@ -911,6 +983,7 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
isLeakProof = false;
volatility = PROVOLATILE_VOLATILE;
proconfig = NULL;
+ proconfigislist = NULL;
procost = -1; /* indicates not set */
prorows = -1; /* indicates not set */
parallel = PROPARALLEL_UNSAFE;
@@ -922,7 +995,8 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
&as_clause, &language, &transformDefElem,
&isWindowFunc, &volatility,
&isStrict, &security, &isLeakProof,
- &proconfig, &procost, &prorows, ¶llel);
+ &proconfig, &proconfigislist,
+ &procost, &prorows, ¶llel);
/* Look up the language and validate permissions */
languageTuple = SearchSysCache1(LANGNAME, PointerGetDatum(language));
@@ -1113,6 +1187,7 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
parameterDefaults,
PointerGetDatum(trftypes),
PointerGetDatum(proconfig),
+ PointerGetDatum(proconfigislist),
procost,
prorows);
}
diff --git a/src/backend/commands/proclang.c b/src/backend/commands/proclang.c
index 447bd49f89..2fcba890fe 100644
--- a/src/backend/commands/proclang.c
+++ b/src/backend/commands/proclang.c
@@ -142,6 +142,7 @@ CreateProceduralLanguage(CreatePLangStmt *stmt)
NIL,
PointerGetDatum(NULL),
PointerGetDatum(NULL),
+ PointerGetDatum(NULL),
1,
0);
handlerOid = tmpAddr.objectId;
@@ -181,6 +182,7 @@ CreateProceduralLanguage(CreatePLangStmt *stmt)
NIL,
PointerGetDatum(NULL),
PointerGetDatum(NULL),
+ PointerGetDatum(NULL),
1,
0);
inlineOid = tmpAddr.objectId;
@@ -223,6 +225,7 @@ CreateProceduralLanguage(CreatePLangStmt *stmt)
NIL,
PointerGetDatum(NULL),
PointerGetDatum(NULL),
+ PointerGetDatum(NULL),
1,
0);
valOid = tmpAddr.objectId;
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index bf3cd3a454..9cbd98fbca 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1685,6 +1685,7 @@ makeRangeConstructors(const char *name, Oid namespace,
NIL, /* parameterDefaults */
PointerGetDatum(NULL), /* trftypes */
PointerGetDatum(NULL), /* proconfig */
+ PointerGetDatum(NULL), /* proconfigislist */
1.0, /* procost */
0.0); /* prorows */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a4f9b3668e..c67f68dc1b 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -8112,6 +8112,30 @@ GetConfigOptionByName(const char *name, const char **varname, bool missing_ok)
return _ShowOption(record, true);
}
+/*
+ * Return "flags" for a given GUC variable. If missing_ok is true, ignore
+ * any errors and return 0 to the caller instead.
+ */
+int
+GetConfigOptionFlags(const char *name, bool missing_ok)
+{
+ struct config_generic *record;
+
+ record = find_option(name, false, ERROR);
+
+ if (record == NULL)
+ {
+ if (missing_ok)
+ return 0;
+
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("unrecognized configuration parameter \"%s\"", name)));
+ }
+
+ return record->flags;
+}
+
/*
* Return GUC variable value by variable number; optionally return canonical
* form of name. Return value is palloc'd.
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index 01cf59e7a3..26b1866c69 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -151,7 +151,7 @@ DATA(insert OID = 1247 ( pg_type PGNSP 71 0 PGUID 0 0 0 0 0 0 0 f f p r 30 0 t
DESCR("");
DATA(insert OID = 1249 ( pg_attribute PGNSP 75 0 PGUID 0 0 0 0 0 0 0 f f p r 22 0 f f f f f f f t n f 3 1 _null_ _null_ _null_));
DESCR("");
-DATA(insert OID = 1255 ( pg_proc PGNSP 81 0 PGUID 0 0 0 0 0 0 0 f f p r 28 0 t f f f f f f t n f 3 1 _null_ _null_ _null_));
+DATA(insert OID = 1255 ( pg_proc PGNSP 81 0 PGUID 0 0 0 0 0 0 0 f f p r 29 0 t f f f f f f t n f 3 1 _null_ _null_ _null_));
DESCR("");
DATA(insert OID = 1259 ( pg_class PGNSP 83 0 PGUID 0 0 0 0 0 0 0 f f p r 33 0 t f f f f f f t n f 3 1 _null_ _null_ _null_));
DESCR("");
diff --git a/src/include/catalog/pg_proc_fn.h b/src/include/catalog/pg_proc_fn.h
index b66871bc46..94dadc5c7b 100644
--- a/src/include/catalog/pg_proc_fn.h
+++ b/src/include/catalog/pg_proc_fn.h
@@ -40,6 +40,7 @@ extern ObjectAddress ProcedureCreate(const char *procedureName,
List *parameterDefaults,
Datum trftypes,
Datum proconfig,
+ Datum proconfigislist,
float4 procost,
float4 prorows);
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 2e03640c0b..7951257c50 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -368,6 +368,7 @@ extern int set_config_option(const char *name, const char *value,
extern void AlterSystemSetConfigFile(AlterSystemStmt *setstmt);
extern char *GetConfigOptionByName(const char *name, const char **varname,
bool missing_ok);
+extern int GetConfigOptionFlags(const char *name, bool missing_ok);
extern void GetConfigOptionByNum(int varnum, const char **values, bool *noshow);
extern int GetNumConfigOptions(void);
--
2.16.2
0002-pg_proc.h-part.patch.gz
Description: Binary data
