pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

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

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

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

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
/messages/by-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

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
/messages/by-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

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
/messages/by-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

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

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

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

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

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

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

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

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

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

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

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

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

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:
/messages/by-id/152049236165.23137.5241258464332317087@wrigleys.postgresql.org
--
Michael