Horton

Thread

Commits

  1. Prevent extensions from creating custom GUCs that are GUC_LIST_QUOTE.

  2. Fix mishandling of quoted-list GUC values in pg_dump and ruleutils.c.

  1. pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Michael Paquier <michael.paquier@gmail.com> — 2018-01-11T06:49:00Z

    Hi all,

While reviewing another patch related to the use of pg_strcasecmp in the
backend, I have noticed this bit in ruleutils.c:

    /*
     * Some GUC variable names are 'LIST' type and hence must not
     * be quoted.
     */
    if (pg_strcasecmp(configitem, "DateStyle") == 0
        || pg_strcasecmp(configitem, "search_path") == 0)
        appendStringInfoString(&buf, pos);
    else
        simple_quote_literal(&buf, pos);

However this fails to consider all GUCs marked as GUC_LIST_INPUT, like
the recent wal_consistency_checking. guc.c already holds a find_option()
which can be used to retrieve the flags of a parameter. What about using
that and filtering by GUC_LIST_INPUT? This requires exposing the
function, and I am not sure what people think about that.

Thoughts?
--
Michael

  2. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-11T15:42:38Z

    Michael Paquier <michael.paquier@gmail.com> writes:
> While reviewing another patch related to the use of pg_strcasecmp in the
> backend, I have noticed this bit in ruleutils.c:

>     /*
>      * Some GUC variable names are 'LIST' type and hence must not
>      * be quoted.
>      */
>     if (pg_strcasecmp(configitem, "DateStyle") == 0
>         || pg_strcasecmp(configitem, "search_path") == 0)
>         appendStringInfoString(&buf, pos);
>     else
>         simple_quote_literal(&buf, pos);

> However this fails to consider all GUCs marked as GUC_LIST_INPUT, like
> the recent wal_consistency_checking.

Mmm, yeah, probably time to find a more maintainable solution to that.

> guc.c already holds a find_option()
> which can be used to retrieve the flags of a parameter. What about using
> that and filtering by GUC_LIST_INPUT? This requires exposing the
> function, and I am not sure what people think about that.

Don't like exposing find_option directly, but I think it would make
sense to provide an API along the lines of
int GetConfigOptionFlags(const char *name, bool missing_ok).

			regards, tom lane

  3. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Michael Paquier <michael.paquier@gmail.com> — 2018-01-12T01:24:40Z

    On Thu, Jan 11, 2018 at 10:42:38AM -0500, Tom Lane wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> guc.c already holds a find_option()
>> which can be used to retrieve the flags of a parameter. What about using
>> that and filtering by GUC_LIST_INPUT? This requires exposing the
>> function, and I am not sure what people think about that.
> 
> Don't like exposing find_option directly, but I think it would make
> sense to provide an API along the lines of
> int GetConfigOptionFlags(const char *name, bool missing_ok).

OK, I can live with that. What do you think about the attached? I'll be
happy to produce patches for back-branches as necessary. When an option
is not found, I have made the function return 0 as value for the flags,
which is consistent with flatten_set_variable_args(). To make things
behave more consistently with GUC_LIST_QUOTE GUCs, it seems to me that
those should not be quoted as well (ALTER SYSTEM shares the same
compatibility). And attached is a patch.

While reviewing more the code, I have noticed the same code pattern in
pg_dump.c and pg_dumpall.c. In the patch attached, I have corrected
things so as all parameters marked as GUC_LIST_QUOTE are correctly not
quoted because we have no generic solution to know from frontends what's
a GUC type (it would make sense to me to expose a text[] with this
information in pg_settings). However, let's be honest, it does not make
sense to update all those parameters because who is really going to use
them for functions! Two things that make sense to me are just
wal_consistency_checking and synchronous_standby_names for developers
which could use it to tune WAL generated within a set of SQL or plpgsql
functions. As it is easier to delete than add code here, I have added
all of them to ease the production of a committable version. For
correctness, still having them may make sense.
--
Michael

  4. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Artur Zakirov <a.zakirov@postgrespro.ru> — 2018-02-20T15:46:57Z

    Hello,

On Fri, Jan 12, 2018 at 10:24:40AM +0900, Michael Paquier wrote:
> OK, I can live with that. What do you think about the attached? I'll be
> happy to produce patches for back-branches as necessary. When an option
> is not found, I have made the function return 0 as value for the flags,
> which is consistent with flatten_set_variable_args(). To make things
> behave more consistently with GUC_LIST_QUOTE GUCs, it seems to me that
> those should not be quoted as well (ALTER SYSTEM shares the same
> compatibility). And attached is a patch.

Just 2 cents from me. It seems that there is a problem with extensions
GUCs. For example:

=# CREATE FUNCTION func_with_set_params() RETURNS integer
    AS 'select 1;'
    LANGUAGE SQL
    set plpgsql.extra_errors to 'shadowed_variables';
=# SELECT pg_get_functiondef('func_with_set_params'::regproc);
                    pg_get_functiondef                    
----------------------------------------------------------
 CREATE OR REPLACE FUNCTION public.func_with_set_params()+
  RETURNS integer                                        +
  LANGUAGE sql                                           +
  SET "plpgsql.extra_errors" TO 'shadowed_variables'     +
 AS $function$select 1;$function$                        +

It is good while plpgsql is loaded. But when we exit the session and try
it again in another:

=# SELECT pg_get_functiondef('func_with_set_params'::regproc);
ERROR:  unrecognized configuration parameter "plpgsql.extra_errors"

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

  5. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Michael Paquier <michael@paquier.xyz> — 2018-02-21T07:35:50Z

    On Tue, Feb 20, 2018 at 06:46:57PM +0300, Arthur Zakirov wrote:
> Just 2 cents from me. It seems that there is a problem with extensions
> GUCs.
>
> [...]
>
> =# SELECT pg_get_functiondef('func_with_set_params'::regproc);
> ERROR:  unrecognized configuration parameter "plpgsql.extra_errors"

You have an excellent point here, and I did not consider it.
For pg_dump and pg_dumpall, something like what I described in
https://www.postgresql.org/message-id/20180112012440.GA2222@paquier.xyz
to extend pg_settings so as GUC types are visible via SQL could make
sense, now it is really a lot for just being able to quote parameters
correctly.  On top of that, what I suggested previously would not work
reliably except if we have pg_get_functiondef load the library
associated to a given parameter.  Even in this case there could
perfectly be a custom parameter from another plugin and not a parameter
associated to the function language itself.

It seems to me that this brings us back to a best-effort for the backend
and the frontend by maintaining a list of hardcoded parameter names,
which could be refined a maximum by considering lists for in-core
extensions and perhaps the most famous extensions around :(

Thoughts?
--
Michael

  6. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Artur Zakirov <a.zakirov@postgrespro.ru> — 2018-03-03T18:13:46Z

    On Wed, Feb 21, 2018 at 04:35:50PM +0900, Michael Paquier wrote:
> You have an excellent point here, and I did not consider it.
> For pg_dump and pg_dumpall, something like what I described in
> https://www.postgresql.org/message-id/20180112012440.GA2222@paquier.xyz
> to extend pg_settings so as GUC types are visible via SQL could make
> sense, now it is really a lot for just being able to quote parameters
> correctly.  On top of that, what I suggested previously would not work
> reliably except if we have pg_get_functiondef load the library
> associated to a given parameter.  Even in this case there could
> perfectly be a custom parameter from another plugin and not a parameter
> associated to the function language itself.

In my opinion GetConfigOptionFlags() can be used for core and plpgsql GUCs.
A variable should not be quoted if it has GUC_LIST_INPUT flag or it is
plpgsql.extra_warnings or plpgsql.extra_errors.

I'm not sure that it is good to raise an error when the variable isn't
found, because without the patch the error isn't raised. But maybe better
to raise it to notify a user that there is a wrong variable. In this case we
may not raise the error if variable name contains
GUC_QUALIFIER_SEPARATOR.

> It seems to me that this brings us back to a best-effort for the backend
> and the frontend by maintaining a list of hardcoded parameter names,
> which could be refined a maximum by considering lists for in-core
> extensions and perhaps the most famous extensions around :(

It seems for frontend maintaining a hardcoded list is the only way.
Agree that extending pg_settings for that could be too much.


I've searched extensions in GitHub with GUC_LIST_INPUT variables. There
are couple of them:
- https://github.com/ohmu/pgmemcache
- https://github.com/marconeperes/pgBRTypes
And some not very fresh:
- https://github.com/witblitz/pldotnet
- https://github.com/ohmu/pgloggingfilter
- https://github.com/wangyuehong/pggearman
- https://github.com/siavashg/pgredis

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

  7. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Pavel Stehule <pavel.stehule@gmail.com> — 2018-03-05T20:25:09Z

    2018-02-21 8:35 GMT+01:00 Michael Paquier <michael@paquier.xyz>:

> On Tue, Feb 20, 2018 at 06:46:57PM +0300, Arthur Zakirov wrote:
> > Just 2 cents from me. It seems that there is a problem with extensions
> > GUCs.
> >
> > [...]
> >
> > =# SELECT pg_get_functiondef('func_with_set_params'::regproc);
> > ERROR:  unrecognized configuration parameter "plpgsql.extra_errors"
>
> You have an excellent point here, and I did not consider it.
> For pg_dump and pg_dumpall, something like what I described in
> https://www.postgresql.org/message-id/20180112012440.GA2222@paquier.xyz
> to extend pg_settings so as GUC types are visible via SQL could make
> sense, now it is really a lot for just being able to quote parameters
> correctly.  On top of that, what I suggested previously would not work
> reliably except if we have pg_get_functiondef load the library
> associated to a given parameter.  Even in this case there could
> perfectly be a custom parameter from another plugin and not a parameter
> associated to the function language itself.
>
> It seems to me that this brings us back to a best-effort for the backend
> and the frontend by maintaining a list of hardcoded parameter names,
> which could be refined a maximum by considering lists for in-core
> extensions and perhaps the most famous extensions around :(
>

I afraid so there is not good solution. Is possible to store options in
original form? When the function will be displayed, then the original value
will be displayed. The current patch (with known extensions) can be used as
bug fix and back patched. In new version we store original value with
quotes.

Regards

Pavel



>
>
> Thoughts?
> --
> Michael
>

  8. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Michael Paquier <michael@paquier.xyz> — 2018-03-06T01:32:00Z

    On Mon, Mar 05, 2018 at 09:25:09PM +0100, Pavel Stehule wrote:
> I afraid so there is not good solution. Is possible to store options in
> original form? When the function will be displayed, then the original value
> will be displayed. The current patch (with known extensions) can be used as
> bug fix and back patched. In new version we store original value with
> quotes.

You mean storing the values in pg_proc.proconfig at the creation time of
the function?  That would basically work, except for the case of a
function using a parameter which is not from the same PL.  The function
creation would fail because it cannot find the parameter it is looking
for as we need to look at loaded parameters to know it uses a list or
not :(
--
Michael

  9. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Pavel Stehule <pavel.stehule@gmail.com> — 2018-03-06T06:14:00Z

    2018-03-06 2:32 GMT+01:00 Michael Paquier <michael@paquier.xyz>:

> On Mon, Mar 05, 2018 at 09:25:09PM +0100, Pavel Stehule wrote:
> > I afraid so there is not good solution. Is possible to store options in
> > original form? When the function will be displayed, then the original
> value
> > will be displayed. The current patch (with known extensions) can be used
> as
> > bug fix and back patched. In new version we store original value with
> > quotes.
>
> You mean storing the values in pg_proc.proconfig at the creation time of
> the function?  That would basically work, except for the case of a
> function using a parameter which is not from the same PL.  The function
> creation would fail because it cannot find the parameter it is looking
> for as we need to look at loaded parameters to know it uses a list or
> not :(
>

yes. It can fails on execution time, but it is something like runtime error.

just dump should to produce same form like was input. So if on input we got
quotes, then we should to use quotes on output. Without querying somewhere.

The possible quotes can be removed in function compile time.

Pavel

> --
> Michael
>

  10. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-03-14T08:30:59Z

    Hello,

At Tue, 6 Mar 2018 07:14:00 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in <CAFj8pRB7u-D1NA8a22dytwicKv4RWrYqKCF=yiL5kKMKbssPSw@mail.gmail.com>
> 2018-03-06 2:32 GMT+01:00 Michael Paquier <michael@paquier.xyz>:
> 
> > On Mon, Mar 05, 2018 at 09:25:09PM +0100, Pavel Stehule wrote:
> > > I afraid so there is not good solution. Is possible to store options in
> > > original form? When the function will be displayed, then the original
> > value
> > > will be displayed. The current patch (with known extensions) can be used
> > as
> > > bug fix and back patched. In new version we store original value with
> > > quotes.
> >
> > You mean storing the values in pg_proc.proconfig at the creation time of
> > the function?  That would basically work, except for the case of a
> > function using a parameter which is not from the same PL.  The function
> > creation would fail because it cannot find the parameter it is looking
> > for as we need to look at loaded parameters to know it uses a list or
> > not :(
> >
> 
> yes. It can fails on execution time, but it is something like runtime error.
> 
> just dump should to produce same form like was input. So if on input we got
> quotes, then we should to use quotes on output. Without querying somewhere.
> 
> The possible quotes can be removed in function compile time.

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?

The attached perl script is a rush work of such script, which
works at the top of the source tree. It just prints the function
definition, does not generate a .h file.

I haven't confirmed anything about it but I had the following
output from the current master.

> inline bool
> IsConfigOptionIsAList(const char *name)
> 
> {
> 	if (pg_strcasecmp(name, "DateStyle") == 0
> 		|| pg_strcasecmp(name, "temp_tablespaces") == 0
> 		|| pg_strcasecmp(name, "session_preload_libraries") == 0
> 		|| pg_strcasecmp(name, "shared_preload_libraries") == 0
> 		|| pg_strcasecmp(name, "local_preload_libraries") == 0
> 		|| pg_strcasecmp(name, "search_path") == 0
> 		|| pg_strcasecmp(name, "log_destination") == 0
> 		|| pg_strcasecmp(name, "listen_addresses") == 0
> 		|| pg_strcasecmp(name, "synchronous_standby_names") == 0
> 		|| pg_strcasecmp(name, "wal_consistency_checking") == 0)
> 		return true;
> 	return false;
> }


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  11. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Michael Paquier <michael@paquier.xyz> — 2018-03-14T08:46:21Z

    On Wed, Mar 14, 2018 at 05:30:59PM +0900, Kyotaro HORIGUCHI wrote:
> 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?
> 
> The attached perl script is a rush work of such script, which
> works at the top of the source tree. It just prints the function
> definition, does not generate a .h file.
> 
> I haven't confirmed anything about it but I had the following
> output from the current master.

I quite like that idea actually.  Please note that pl_handler.c declares
two more of them, so we may want a smarter parsing which takes into
account declarations of DefineCustom*Variable.
--
Michael

  12. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-03-15T04:27:40Z

    Hello,

At Wed, 14 Mar 2018 17:46:21 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180314084621.GA617@paquier.xyz>
> On Wed, Mar 14, 2018 at 05:30:59PM +0900, Kyotaro HORIGUCHI wrote:
> > 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?
> > 
> > The attached perl script is a rush work of such script, which
> > works at the top of the source tree. It just prints the function
> > definition, does not generate a .h file.
> > 
> > I haven't confirmed anything about it but I had the following
> > output from the current master.
> 
> I quite like that idea actually.  Please note that pl_handler.c declares
> two more of them, so we may want a smarter parsing which takes into
> account declarations of DefineCustom*Variable.

Only DefineCustomStringVariable can accept a list argument even
if flagged GUC_LIST_INPUT. (I'm not sure this should be
explicitly rejected.) I'm not sure this is smart enough but it
works. It would fail if description for variables contain the
same character sequence to the lexical structure around but it is
rare to happen.

The attached patch is the result. It adds a script to generate
include/common/check_listvars.h. It won't be updated even if
related files are modifed (since we cannot know it before
scanning the whole source.) but "make clean" removes it. Regtests
is a copy of Michael's v1 patch.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  13. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-15T04:33:25Z

    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?

			regards, tom lane

  14. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-03-15T05:03:15Z

    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.

As the discussion upthread, with the dynamic (or on the fly)
approach, pg_dump fails when required extension is not
loaded. Especially plpgsql variables are the candidate stopper of
ordinary pg_dump operation. We might have to implicitly load the
module by any means to make it work. If we treat extensions
properly, we must find the extension that have defined a GUC that
is about to be exported, then load it.

I don't think the automatic stuff is essential but the
check_listvars.h is still effective to reduce the effort needed
to maintain the multiple lists that should have the same set of
names of the list-gucs.

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...

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  15. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Michael Paquier <michael@paquier.xyz> — 2018-03-15T06:09:54Z

    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.

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.
--
Michael

  16. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-03-15T09:48:36Z

    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

  17. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Artur Zakirov <a.zakirov@postgrespro.ru> — 2018-03-15T10:33:51Z

    On Thu, Mar 15, 2018 at 06:48:36PM +0900, Kyotaro HORIGUCHI wrote:
> 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.

I think your approach has a vulnerability too. I believe that a
non GUC_LIST_INPUT extension GUC which was used to create a function may
become GUC_LIST_INPUT variable. If I'm not mistaken nothing stops from
that. In this case values in proconfigislist won't be valide anymore.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

  18. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Michael Paquier <michael@paquier.xyz> — 2018-03-16T01:21:39Z

    On Thu, Mar 15, 2018 at 01:33:51PM +0300, Arthur Zakirov wrote:
> I think your approach has a vulnerability too. I believe that a
> non GUC_LIST_INPUT extension GUC which was used to create a function may
> become GUC_LIST_INPUT variable. If I'm not mistaken nothing stops from
> that. In this case values in proconfigislist won't be valide anymore.

I don't understand what you mean here.  Are you referring to a custom
GUC which was initially declared as not being a list, but became a list
after a plugin upgrade with the same name?  Isn't the author to blame in
this case?
--
Michael

  19. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Michael Paquier <michael@paquier.xyz> — 2018-03-16T01:25:35Z

    On Thu, Mar 15, 2018 at 06:48:36PM +0900, Kyotaro HORIGUCHI wrote:
> 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.

How can you be sure that a parameter actually exists?  A function
definition could as well use a parameter which does not exist, but you
would get this error as well, no?  I find that error and handling a bit
confusing.

> postgres=# load 'plpgsql';
> [...]
> pg_get_functiondef() can work correctly with this even if
> required modules are not loaded.

Yeah, but the neck to any approaches here is that many applications may
rely on the existing behavior, and would miss the fact that they need to
load a module manually.

> But, I suppose it is a bit too big.

That's of course not backpatchable.
--
Michael

  20. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Michael Paquier <michael@paquier.xyz> — 2018-03-16T04:46:15Z

    On Fri, Mar 16, 2018 at 10:25:35AM +0900, Michael Paquier wrote:
>> But, I suppose it is a bit too big.
> 
> That's of course not backpatchable.

So in this jungle attached is my counter-proposal.  As the same code
pattern is repeated in three places, we could as well refactor the whole
into a common routine, say in src/common/guc_options.c or similar.
Perhaps just on HEAD and not back-branches as this is always annoying
for packagers on Windows using custom scripts.  Per the lack of
complains only doing something on HEAD, with only a subset of the
parameters which make sense for CREATE FUNCTION is what makes the most
sense in my opinion.

As mentioned in this bug, the handling of empty values gets kind of
tricky as in this case proconfig stores a set of quotes and not an
actual value:
https://www.postgresql.org/message-id/152049236165.23137.5241258464332317087@wrigleys.postgresql.org
--
Michael

  21. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Pavel Stehule <pavel.stehule@gmail.com> — 2018-03-16T08:16:54Z

    2018-03-16 5:46 GMT+01:00 Michael Paquier <michael@paquier.xyz>:

> On Fri, Mar 16, 2018 at 10:25:35AM +0900, Michael Paquier wrote:
> >> But, I suppose it is a bit too big.
> >
> > That's of course not backpatchable.
>
> So in this jungle attached is my counter-proposal.  As the same code
> pattern is repeated in three places, we could as well refactor the whole
> into a common routine, say in src/common/guc_options.c or similar.
> Perhaps just on HEAD and not back-branches as this is always annoying
> for packagers on Windows using custom scripts.  Per the lack of
> complains only doing something on HEAD, with only a subset of the
> parameters which make sense for CREATE FUNCTION is what makes the most
> sense in my opinion.
>

Last patch is good enough solution, I tested it and it is working

Little bit strange is support of GUC, that cannot be atteched to any fx.
Probably only PGC_USERSET, maybe PGC_SUSET has sense there

+                if (pg_strcasecmp(configitem, "DateStyle") == 0 ||
+                    pg_strcasecmp(configitem, "listen_addresses") == 0 ||
+                    pg_strcasecmp(configitem, "local_preload_libraries")
== 0 ||
+                    pg_strcasecmp(configitem, "log_destination") == 0 ||
+                    pg_strcasecmp(configitem, "plpgsql.extra_errors") == 0
||
+                    pg_strcasecmp(configitem, "plpgsql.extra_warnings") ==
0 ||
+                    pg_strcasecmp(configitem, "search_path") == 0 ||
+                    pg_strcasecmp(configitem, "session_preload_libraries")
== 0 ||
+                    pg_strcasecmp(configitem, "shared_preload_libraries")
== 0 ||
+                    pg_strcasecmp(configitem, "synchronous_standby_names")
== 0 ||
+                    pg_strcasecmp(configitem, "temp_tablespaces") == 0 ||
+                    pg_strcasecmp(configitem, "wal_consistency_checking")
== 0)


another idea, how to solve this issue for extensions. The information about
format of extensions GUC can be stored in new column pg_extension - maybe
just a array of list of LIST GUC.

CREATE EXTENSION plpgsql ...
   GUC_PREFIX 'plpgsql'
   LIST_GUC ('extra_errors','extra_warnings')
   ...

Regards

Pavel



>
> As mentioned in this bug, the handling of empty values gets kind of
> tricky as in this case proconfig stores a set of quotes and not an
> actual value:
> https://www.postgresql.org/message-id/152049236165.23137.
> 5241258464332317087@wrigleys.postgresql.org
> --
> Michael
>

  22. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-03-16T08:34:33Z

    At Fri, 16 Mar 2018 09:16:54 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in <CAFj8pRC6+_8c6PLHbDzCSKEDpkEhcVqyFmJn9_zYUjAwdnUboQ@mail.gmail.com>
> 2018-03-16 5:46 GMT+01:00 Michael Paquier <michael@paquier.xyz>:
> 
> > On Fri, Mar 16, 2018 at 10:25:35AM +0900, Michael Paquier wrote:
> > >> But, I suppose it is a bit too big.
> > >
> > > That's of course not backpatchable.

I meant that it is just too big for the objective regardless of
back-patching:p

So, automatic generation is not acceptable, dynamic checking
(including that at the function creation) would be
unacceptable. So I agree that what we can do now is just maintian
the list of pg_strcasecmp.

> > So in this jungle attached is my counter-proposal.  As the same code
> > pattern is repeated in three places, we could as well refactor the whole
> > into a common routine, say in src/common/guc_options.c or similar.
> > Perhaps just on HEAD and not back-branches as this is always annoying
> > for packagers on Windows using custom scripts.  Per the lack of
> > complains only doing something on HEAD, with only a subset of the
> > parameters which make sense for CREATE FUNCTION is what makes the most
> > sense in my opinion.
> >
> 
> Last patch is good enough solution, I tested it and it is working
> 
> Little bit strange is support of GUC, that cannot be atteched to any fx.
> Probably only PGC_USERSET, maybe PGC_SUSET has sense there

That's true, but doesn't the additional rule make it more
bothersome to maintain the list?

> +                if (pg_strcasecmp(configitem, "DateStyle") == 0 ||
> +                    pg_strcasecmp(configitem, "listen_addresses") == 0 ||
> +                    pg_strcasecmp(configitem, "local_preload_libraries")
> == 0 ||
> +                    pg_strcasecmp(configitem, "log_destination") == 0 ||
> +                    pg_strcasecmp(configitem, "plpgsql.extra_errors") == 0
> ||
> +                    pg_strcasecmp(configitem, "plpgsql.extra_warnings") ==
> 0 ||
> +                    pg_strcasecmp(configitem, "search_path") == 0 ||
> +                    pg_strcasecmp(configitem, "session_preload_libraries")
> == 0 ||
> +                    pg_strcasecmp(configitem, "shared_preload_libraries")
> == 0 ||
> +                    pg_strcasecmp(configitem, "synchronous_standby_names")
> == 0 ||
> +                    pg_strcasecmp(configitem, "temp_tablespaces") == 0 ||
> +                    pg_strcasecmp(configitem, "wal_consistency_checking")
> == 0)
> 
> 
> another idea, how to solve this issue for extensions. The information about
> format of extensions GUC can be stored in new column pg_extension - maybe
> just a array of list of LIST GUC.
> 
> CREATE EXTENSION plpgsql ...
>    GUC_PREFIX 'plpgsql'
>    LIST_GUC ('extra_errors','extra_warnings')
>    ...
> 
> Regards
> 
> Pavel
> 
> 
> 
> >
> > As mentioned in this bug, the handling of empty values gets kind of
> > tricky as in this case proconfig stores a set of quotes and not an
> > actual value:
> > https://www.postgresql.org/message-id/152049236165.23137.
> > 5241258464332317087@wrigleys.postgresql.org
> > --
> > Michael

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  23. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Pavel Stehule <pavel.stehule@gmail.com> — 2018-03-16T08:40:18Z

    2018-03-16 9:34 GMT+01:00 Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp
>:

> At Fri, 16 Mar 2018 09:16:54 +0100, Pavel Stehule <pavel.stehule@gmail.com>
> wrote in <CAFj8pRC6+_8c6PLHbDzCSKEDpkEhcVqyFmJn9_
> zYUjAwdnUboQ@mail.gmail.com>
> > 2018-03-16 5:46 GMT+01:00 Michael Paquier <michael@paquier.xyz>:
> >
> > > On Fri, Mar 16, 2018 at 10:25:35AM +0900, Michael Paquier wrote:
> > > >> But, I suppose it is a bit too big.
> > > >
> > > > That's of course not backpatchable.
>
> I meant that it is just too big for the objective regardless of
> back-patching:p
>
> So, automatic generation is not acceptable, dynamic checking
> (including that at the function creation) would be
> unacceptable. So I agree that what we can do now is just maintian
> the list of pg_strcasecmp.
>
> > > So in this jungle attached is my counter-proposal.  As the same code
> > > pattern is repeated in three places, we could as well refactor the
> whole
> > > into a common routine, say in src/common/guc_options.c or similar.
> > > Perhaps just on HEAD and not back-branches as this is always annoying
> > > for packagers on Windows using custom scripts.  Per the lack of
> > > complains only doing something on HEAD, with only a subset of the
> > > parameters which make sense for CREATE FUNCTION is what makes the most
> > > sense in my opinion.
> > >
> >
> > Last patch is good enough solution, I tested it and it is working
> >
> > Little bit strange is support of GUC, that cannot be atteched to any fx.
> > Probably only PGC_USERSET, maybe PGC_SUSET has sense there
>
> That's true, but doesn't the additional rule make it more
> bothersome to maintain the list?
>

Hard to say. I have not opinion. But just when I see in this context
variables like listen_address, shared_preload_librarries, then it looks
messy.




>
> > +                if (pg_strcasecmp(configitem, "DateStyle") == 0 ||
> > +                    pg_strcasecmp(configitem, "listen_addresses") == 0
> ||
> > +                    pg_strcasecmp(configitem, "local_preload_libraries")
> > == 0 ||
> > +                    pg_strcasecmp(configitem, "log_destination") == 0 ||
> > +                    pg_strcasecmp(configitem, "plpgsql.extra_errors")
> == 0
> > ||
> > +                    pg_strcasecmp(configitem, "plpgsql.extra_warnings")
> ==
> > 0 ||
> > +                    pg_strcasecmp(configitem, "search_path") == 0 ||
> > +                    pg_strcasecmp(configitem,
> "session_preload_libraries")
> > == 0 ||
> > +                    pg_strcasecmp(configitem,
> "shared_preload_libraries")
> > == 0 ||
> > +                    pg_strcasecmp(configitem,
> "synchronous_standby_names")
> > == 0 ||
> > +                    pg_strcasecmp(configitem, "temp_tablespaces") == 0
> ||
> > +                    pg_strcasecmp(configitem,
> "wal_consistency_checking")
> > == 0)
> >
> >
> > another idea, how to solve this issue for extensions. The information
> about
> > format of extensions GUC can be stored in new column pg_extension - maybe
> > just a array of list of LIST GUC.
> >
> > CREATE EXTENSION plpgsql ...
> >    GUC_PREFIX 'plpgsql'
> >    LIST_GUC ('extra_errors','extra_warnings')
> >    ...
> >
> > Regards
> >
> > Pavel
> >
> >
> >
> > >
> > > As mentioned in this bug, the handling of empty values gets kind of
> > > tricky as in this case proconfig stores a set of quotes and not an
> > > actual value:
> > > https://www.postgresql.org/message-id/152049236165.23137.
> > > 5241258464332317087@wrigleys.postgresql.org
> > > --
> > > Michael
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
>

  24. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Artur Zakirov <a.zakirov@postgrespro.ru> — 2018-03-16T10:38:31Z

    On Fri, Mar 16, 2018 at 10:21:39AM +0900, Michael Paquier wrote:
> On Thu, Mar 15, 2018 at 01:33:51PM +0300, Arthur Zakirov wrote:
> > I think your approach has a vulnerability too. I believe that a
> > non GUC_LIST_INPUT extension GUC which was used to create a function may
> > become GUC_LIST_INPUT variable. If I'm not mistaken nothing stops from
> > that. In this case values in proconfigislist won't be valide anymore.
> 
> I don't understand what you mean here.  Are you referring to a custom
> GUC which was initially declared as not being a list, but became a list
> after a plugin upgrade with the same name?

Yes exactly. Sorry for the unclear message.

> Isn't the author to blame in this case?

Maybe he is. It may be better to rename a variable if it became a list.
I haven't strong opinion here though. I wanted to point the case where
proconfigislist column won't work.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

  25. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Michael Paquier <michael@paquier.xyz> — 2018-03-16T12:15:54Z

    On Fri, Mar 16, 2018 at 09:40:18AM +0100, Pavel Stehule wrote:
> 2018-03-16 9:34 GMT+01:00 Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp
>> That's true, but doesn't the additional rule make it more
>> bothersome to maintain the list?
> 
> Hard to say. I have not opinion. But just when I see in this context
> variables like listen_address, shared_preload_libraries, then it looks
> messy.

Let's be clear.  I have listed all the variables in the patch to gather
more easily opinions, and because it is easier to review the whole stack
this way. I personally think that the only variables where the patch
makes sense are:
- DateStyle
- search_path
- plpgsql.extra_errors
- plpgsql.extra_warnings
- wal_consistency_checking
So I would be incline to drop the rest from the patch.  If there are
authors of popular extensions willing to get this support, let's update
the list once they argue about it and only if it makes sense.  However,
as far as I can see, there are no real candidates.  So let's keep the
list simple.
--
Michael

  26. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-03-19T05:49:09Z

    At Fri, 16 Mar 2018 21:15:54 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180316121554.GA2552@paquier.xyz>
> On Fri, Mar 16, 2018 at 09:40:18AM +0100, Pavel Stehule wrote:
> > 2018-03-16 9:34 GMT+01:00 Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp
> >> That's true, but doesn't the additional rule make it more
> >> bothersome to maintain the list?
> > 
> > Hard to say. I have not opinion. But just when I see in this context
> > variables like listen_address, shared_preload_libraries, then it looks
> > messy.
> 
> Let's be clear.  I have listed all the variables in the patch to gather
> more easily opinions, and because it is easier to review the whole stack
> this way. I personally think that the only variables where the patch
> makes sense are:
> - DateStyle
> - search_path
> - plpgsql.extra_errors
> - plpgsql.extra_warnings
> - wal_consistency_checking
> So I would be incline to drop the rest from the patch.  If there are
> authors of popular extensions willing to get this support, let's update
> the list once they argue about it and only if it makes sense.  However,
> as far as I can see, there are no real candidates.  So let's keep the
> list simple.

FWIW +1 from me. It seems reasonable as the amendment to the
current status.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  27. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-19T23:15:36Z

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> At Fri, 16 Mar 2018 21:15:54 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180316121554.GA2552@paquier.xyz>
>> Let's be clear.  I have listed all the variables in the patch to gather
>> more easily opinions, and because it is easier to review the whole stack
>> this way. I personally think that the only variables where the patch
>> makes sense are:
>> - DateStyle
>> - search_path
>> - plpgsql.extra_errors
>> - plpgsql.extra_warnings
>> - wal_consistency_checking
>> So I would be incline to drop the rest from the patch.  If there are
>> authors of popular extensions willing to get this support, let's update
>> the list once they argue about it and only if it makes sense.  However,
>> as far as I can see, there are no real candidates.  So let's keep the
>> list simple.

> FWIW +1 from me. It seems reasonable as the amendment to the
> current status.

It suddenly struck me that the scope of the patch is wider than it needs
to be.  We don't need special behavior for all GUC_LIST variables, only
for GUC_LIST_QUOTE variables.  (For example, SET datestyle = 'iso, mdy'
works just as well as SET datestyle = iso, mdy.)

This is a good thing not least because all the GUC_LIST_QUOTE variables
are in core.  I've been trying to think of a way that we could have
correct behavior for variables that the core backend doesn't know about
and whose extension shlibs aren't currently loaded, and I can't come up
with one.  So I think that in practice, we have to document that
GUC_LIST_QUOTE is unsupported for extension variables (and, perhaps,
enforce this in DefineCustomStringVariable? or is that cure worse than
the disease?)

			regards, tom lane

  28. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Michael Paquier <michael@paquier.xyz> — 2018-03-20T00:39:51Z

    On Mon, Mar 19, 2018 at 07:15:36PM -0400, Tom Lane wrote:
> This is a good thing not least because all the GUC_LIST_QUOTE variables
> are in core.  I've been trying to think of a way that we could have
> correct behavior for variables that the core backend doesn't know about
> and whose extension shlibs aren't currently loaded, and I can't come up
> with one.  So I think that in practice, we have to document that
> GUC_LIST_QUOTE is unsupported for extension variables

I would propose to add a sentence on the matter at the bottom of the
CREATE FUNCTION page.  Even with that, the handling of items in the list
is incorrect with any patches on this thread: the double quotes should
be applied to each element of the list, not the whole list.  For example
with HEAD and this function:
=# CREATE or replace FUNCTION func_with_set_params() RETURNS integer
AS 'select 1;'
LANGUAGE SQL
SET search_path TO 'pg_catalog, public'
SET wal_consistency_checking TO heap, heap2
SET session_preload_libraries TO 'foo, bar'
IMMUTABLE STRICT;

Then retrieving the function definition results in that:
=# SELECT pg_get_functiondef('func_with_set_params'::regproc);
CREATE OR REPLACE FUNCTION public.func_with_set_params()
RETURNS integer
 LANGUAGE sql
 IMMUTABLE STRICT
 SET search_path TO "pg_catalog, public"
 SET wal_consistency_checking TO 'heap, heap2'
 SET session_preload_libraries TO '"foo, bar"'
AS $function$select 1;$function$

And that's wrong as "pg_catalog, public" is only a one-element list, no?

> (and, perhaps,
> enforce this in DefineCustomStringVariable? or is that cure worse than
> the disease?)

Extension authors can just mark the variables they define with
GUC_LIST_QUOTE, so enforcing it would be a step backwards in my
opinion.
--
Michael

  29. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-20T03:07:13Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Mar 19, 2018 at 07:15:36PM -0400, Tom Lane wrote:
>> This is a good thing not least because all the GUC_LIST_QUOTE variables
>> are in core.  I've been trying to think of a way that we could have
>> correct behavior for variables that the core backend doesn't know about
>> and whose extension shlibs aren't currently loaded, and I can't come up
>> with one.  So I think that in practice, we have to document that
>> GUC_LIST_QUOTE is unsupported for extension variables

> I would propose to add a sentence on the matter at the bottom of the
> CREATE FUNCTION page.  Even with that, the handling of items in the list
> is incorrect with any patches on this thread: the double quotes should
> be applied to each element of the list, not the whole list.

No, because all the places we are worried about are interpreting the
contents of proconfig or setconfig columns, and we know that that info
has been through flatten_set_variable_args().  If that function thought
that GUC_LIST_QUOTE was applicable, it already did the appropriate
quoting, and we can't quote over that without making a mess.  But if it
did not think that GUC_LIST_QUOTE was applicable, then its output can
just be treated as a single string, and that will work fine.

Our problem basically is that if we don't know the variable that was
being processed, we can't be sure whether GUC_LIST_QUOTE was used.
I don't currently see a solution to that other than insisting that
GUC_LIST_QUOTE only be used for known core GUCs.

			regards, tom lane

  30. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-03-20T03:37:47Z

    At Mon, 19 Mar 2018 23:07:13 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <10037.1521515233@sss.pgh.pa.us>
> Michael Paquier <michael@paquier.xyz> writes:
> > On Mon, Mar 19, 2018 at 07:15:36PM -0400, Tom Lane wrote:
> >> This is a good thing not least because all the GUC_LIST_QUOTE variables
> >> are in core.  I've been trying to think of a way that we could have
> >> correct behavior for variables that the core backend doesn't know about
> >> and whose extension shlibs aren't currently loaded, and I can't come up
> >> with one.  So I think that in practice, we have to document that
> >> GUC_LIST_QUOTE is unsupported for extension variables
> 
> > I would propose to add a sentence on the matter at the bottom of the
> > CREATE FUNCTION page.  Even with that, the handling of items in the list
> > is incorrect with any patches on this thread: the double quotes should
> > be applied to each element of the list, not the whole list.
> 
> No, because all the places we are worried about are interpreting the
> contents of proconfig or setconfig columns, and we know that that info
> has been through flatten_set_variable_args().  If that function thought
> that GUC_LIST_QUOTE was applicable, it already did the appropriate
> quoting, and we can't quote over that without making a mess.  But if it
> did not think that GUC_LIST_QUOTE was applicable, then its output can
> just be treated as a single string, and that will work fine.
> 
> Our problem basically is that if we don't know the variable that was
> being processed, we can't be sure whether GUC_LIST_QUOTE was used.
> I don't currently see a solution to that other than insisting that
> GUC_LIST_QUOTE only be used for known core GUCs.

The documentation of SET command says as the follows. (CREATE
FUNCTION says a bit different but seems working in the same way)

https://www.postgresql.org/docs/10/static/sql-set.html

> SET [ SESSION | LOCAL ] configuration_parameter { TO | = } { value | 'value' | DEFAULT }
and

> value
> 
>   New value of parameter. Values can be specified as string
>   constants, identifiers, numbers, or comma-separated lists of
>   these, as appropriate for the particular parameter. DEFAULT can
>   be written to specify resetting the parameter to its default
>   value (that is, whatever value it would have had if no SET had
>   been executed in the current session).

According to this description, a comma-separated list enclosed
with single quotes is a valid list value. (I remenber I was
trapped by the description.)

I should be stupid but couldn't we treat quoted comma-separated
list as a bare list if it is the only value in the argument? I
think no one sets such values containing commas as
temp_tablespaces, *_preload_libraries nor search_path.

But of course it may break something and may break some
extensions.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  31. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-20T17:27:35Z

    So here's what I think we should do about this:

1. Only GUC_LIST_QUOTE variables need to be special-cased.  It works
fine to treat plain LIST variables (e.g. datestyle) with the regular
code path.  This is good because there are so few of them.

2. We should make an effort to minimize the number of places that know
explicitly which variables are GUC_LIST_QUOTE.  In the backend, the
right thing to do is ask guc.c.  In pg_dump, there's no convenient
way to ask the backend (and certainly nothing that would work against
old servers), but we should at least make sure there's only one place
to fix not two.

3. We need to prevent extensions from trying to create GUC_LIST_QUOTE
variables, because those are not going to work correctly if they're
set before the extension is loaded, nor is there any hope of pg_dump
dumping them correctly.

The attached 0001 patch does the first two things, and could be
back-patched.  The 0002 patch, which I'd only propose for HEAD,
is one way we could address point 3.  A different way would be
to throw a WARNING rather than ERROR and then just mask off the
problematic bit.  Or we could decide that either of these cures
is worse than the disease, but I don't really agree with that.

Comments?

			regards, tom lane

  32. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Michael Paquier <michael@paquier.xyz> — 2018-03-21T05:01:03Z

    On Tue, Mar 20, 2018 at 01:27:35PM -0400, Tom Lane wrote:
> 1. Only GUC_LIST_QUOTE variables need to be special-cased.  It works
> fine to treat plain LIST variables (e.g. datestyle) with the regular
> code path.  This is good because there are so few of them.

Check.

> 2. We should make an effort to minimize the number of places that know
> explicitly which variables are GUC_LIST_QUOTE.  In the backend, the
> right thing to do is ask guc.c.  In pg_dump, there's no convenient
> way to ask the backend (and certainly nothing that would work against
> old servers), but we should at least make sure there's only one place
> to fix not two.

The only way I can think about for that would be to provide this
information in pg_settings using a text[] array or such which lists all
the GUC flags of a parameter.  That's a hell lot of infrastructure
though which can be easily replaced with the maintenance of a small
hardcoded parameter list.

> 3. We need to prevent extensions from trying to create GUC_LIST_QUOTE
> variables, because those are not going to work correctly if they're
> set before the extension is loaded, nor is there any hope of pg_dump
> dumping them correctly.

This really depends on the elements of the list involved here, so
pg_dump may be able to dump them correctly, though not reliably as that
would be fully application-dependent.  At the end I think that I am on
board with what you are proposing here.

> The attached 0001 patch does the first two things, and could be
> back-patched.  The 0002 patch, which I'd only propose for HEAD,
> is one way we could address point 3.  A different way would be
> to throw a WARNING rather than ERROR and then just mask off the
> problematic bit.  Or we could decide that either of these cures
> is worse than the disease, but I don't really agree with that.

Looks roughly sane to me.

+       if (flags & GUC_LIST_QUOTE)
+               elog(FATAL, "extensions cannot define GUC_LIST_QUOTE
variables");
This would be better as an ereport with ERRCODE_FEATURE_NOT_SUPPORTED I
think.  An ERROR is better in my opinion.

-    if (pg_strcasecmp(configitem, "DateStyle") == 0
-        || pg_strcasecmp(configitem, "search_path") == 0)
+    if (GetConfigOptionFlags(configitem, true) & GUC_LIST_QUOTE)
For boolean-based comparisons, I would recommend using a comparison with
zero.

+   record = find_option(name, false, WARNING);
+   if (record == NULL)
LOG instead of WARNING?
--
Michael

  33. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-21T05:40:23Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Mar 20, 2018 at 01:27:35PM -0400, Tom Lane wrote:
>> +       if (flags & GUC_LIST_QUOTE)
>> +               elog(FATAL, "extensions cannot define GUC_LIST_QUOTE variables");

> This would be better as an ereport with ERRCODE_FEATURE_NOT_SUPPORTED I
> think.  An ERROR is better in my opinion.

I don't mind making it an ereport, but I think it needs to be FATAL
for the reason stated in the comment.

>> +   record = find_option(name, false, WARNING);
>> +   if (record == NULL)

> LOG instead of WARNING?

I did it like that to match the similar code in flatten_set_variable_args.
In the end it doesn't matter, because find_option only uses its elevel
argument when create_placeholders is passed as true.

			regards, tom lane

  34. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Michael Paquier <michael@paquier.xyz> — 2018-03-22T01:20:39Z

    On Wed, Mar 21, 2018 at 01:40:23AM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> On Tue, Mar 20, 2018 at 01:27:35PM -0400, Tom Lane wrote:
>>> +       if (flags & GUC_LIST_QUOTE)
>>> +               elog(FATAL, "extensions cannot define GUC_LIST_QUOTE variables");
> 
>> This would be better as an ereport with ERRCODE_FEATURE_NOT_SUPPORTED I
>> think.  An ERROR is better in my opinion.
> 
> I don't mind making it an ereport, but I think it needs to be FATAL
> for the reason stated in the comment.

Okay for the FATAL.  I can see that at this time of the day your patch
0002 has already been pushed as 846b5a5 with an elog().  Still, it seems
to me that this is not an internal error but an error caused by an
external cause which can be user-visible, so I would push forward with
switching it to an ereport().
--
Michael

  35. Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-22T01:27:11Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Mar 21, 2018 at 01:40:23AM -0400, Tom Lane wrote:
>> I don't mind making it an ereport, but I think it needs to be FATAL
>> for the reason stated in the comment.

> Okay for the FATAL.  I can see that at this time of the day your patch
> 0002 has already been pushed as 846b5a5 with an elog().  Still, it seems
> to me that this is not an internal error but an error caused by an
> external cause which can be user-visible, so I would push forward with
> switching it to an ereport().

Well, after some consideration I pushed it like that because it's not
really a user-facing error but a developer-facing error.  Should we ask
translators to spend time on that message?  I doubt it.  Also, the
adjacent test to refuse PGC_POSTMASTER variables is just an elog;
that seems like pretty much the same sort of situation, and nobody's
complained about it.

If we do something here we should change both of those calls, but
I really doubt it's worth the effort.

			regards, tom lane