Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs - Mailing list pgsql-hackers

At Thu, 15 Mar 2018 15:09:54 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180315060954.GE617@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 <tgl@sss.pgh.pa.us> wrote in <22193.1521088405@sss.pgh.pa.us>
> >> Kyotaro HORIGUCHI <horiguchi.kyotaro@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.kyotaro@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