Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Tom Lane]

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

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Michael Paquier]

On Wed, Mar 21, 2018 at 01:40:23AM -0400, Tom Lane wrote:
> Michael Paquier  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


signature.asc
Description: PGP signature

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Tom Lane]

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

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Tom Lane]

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

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 2cd54ec..b0559ca 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
***
*** 61,66 
--- 61,67 
  #include "utils/array.h"
  #include "utils/builtins.h"
  #include "utils/fmgroids.h"
+ #include "utils/guc.h"
  #include "utils/hsearch.h"
  #include "utils/lsyscache.h"
  #include "utils/rel.h"
*** pg_get_functiondef(PG_FUNCTION_ARGS)
*** 2597,2607 
   quote_identifier(configitem));
  
  /*
!  * 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(, pos);
  else
  	simple_quote_literal(, pos);
--- 2598,2612 
   quote_identifier(configitem));
  
  /*
!  * Variables that are marked GUC_LIST_QUOTE were already fully
!  * quoted by flatten_set_variable_args() before they were put
!  * into the proconfig array; we mustn't re-quote them or we'll
!  * make a mess.  Variables that are not so marked should just
!  * be emitted as simple string literals.  If the variable is
!  * not known to guc.c, we'll do the latter; this makes it
!  * unsafe to use GUC_LIST_QUOTE for extension variables.
   */
! if (GetConfigOptionFlags(configitem, true) & GUC_LIST_QUOTE)
  	appendStringInfoString(, pos);
  else
  	simple_quote_literal(, pos);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 7a7ac47..398680a 100644
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*** static const unit_conversion time_unit_c
*** 796,803 
   *
   * 6. Don't forget to document the option (at least in config.sgml).
   *
!  * 7. If it's a new GUC_LIST option you must edit pg_dumpall.c to ensure
!  *	  it is not single quoted at dump time.
   */
  
  
--- 796,803 
   *
   * 6. Don't forget to document the option (at least in config.sgml).
   *
!  * 7. If it's a new GUC_LIST_QUOTE option, you must add it to
!  *	  variable_is_guc_list_quote() in src/bin/pg_dump/dumputils.c.
   */
  
  
*** GetConfigOptionResetString(const char *n
*** 6859,6864 
--- 6859,6888 
  	return NULL;
  }
  
+ /*
+  * Get the GUC flags associated with the given option.
+  *
+  * If the option doesn't exist, return 0 if missing_ok is true,
+  * otherwise throw an ereport and don't return.
+  */
+ int
+ GetConfigOptionFlags(const char *name, bool missing_ok)
+ {
+ 	struct config_generic *record;
+ 
+ 	record = find_option(name, false, WARNING);
+ 	if (record == NULL)
+ 	{
+ 		if (missing_ok)
+ 			return 0;
+ 		ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+  errmsg("unrecognized configuration parameter \"%s\"",
+ 		name)));
+ 	}
+ 	return record->flags;
+ }
+ 
  
  /*
   * flatten_set_variable_args
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 7f5bb13..875545f 100644
*** a/src/bin/pg_dump/dumputils.c
--- b/src/bin/pg_dump/dumputils.c
*** buildACLQueries(PQExpBuffer acl_subquery
*** 851,856 
--- 851,879 
  }
  
  /*
+  * Detect whether the given GUC variable is of GUC_LIST_QUOTE type.
+  *
+  * It'd be better if we could inquire this directly from the backend; but even
+  * if there were a function for that, it could only tell us about variables
+  * currently known to guc.c, so that it'd be unsafe for extensions to declare
+  * GUC_LIST_QUOTE variables anyway.  Lacking a solution for that, it doesn't
+  * seem worth the work to do more than have this list, 

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Kyotaro HORIGUCHI]

At Mon, 19 Mar 2018 23:07:13 -0400, Tom Lane  wrote in 
<10037.1521515...@sss.pgh.pa.us>
> Michael Paquier  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
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***
*** 6859,6864  GetConfigOptionResetString(const char *name)
--- 6859,6891 
  	return NULL;
  }
  
+ /*
+  * quote_list_identifiers
+  *		quote each element in the given list string
+  */
+ static const char *
+ quote_list_identifiers(char *str)
+ {
+ 	StringInfoData buf;
+ 	List		   *namelist;
+ 	ListCell	   *lc;
+ 
+ 	/* Just quote the whole if this is not a list */
+ 	if (!SplitIdentifierString(str, ',', ))
+ 		return quote_identifier(str);
+ 
+ 	initStringInfo();
+ 	foreach (lc, namelist)
+ 	{
+ 		char *elmstr = (char *) lfirst(lc);
+ 
+ 		if (lc != list_head(namelist))
+ 			appendStringInfoString(, ", ");
+ 		appendStringInfoString(, quote_identifier(elmstr));
+ 	}
+ 
+ 	return buf.data;
+ }
  
  /*
   * flatten_set_variable_args
***
*** 6973,6979  flatten_set_variable_args(const char *name, List *args)
  	 * quote it if it's not a vanilla identifier.
  	 */
  	if (flags & GUC_LIST_QUOTE)
! 		appendStringInfoString(, quote_identifier(val));
  	else
  		appendStringInfoString(, val);
  }
--- 7000,7018 
  	 * quote it if it's not a vanilla identifier.
  	 */
  	if (flags & GUC_LIST_QUOTE)
! 	{
! 		if (list_length(args) > 1)
! 			appendStringInfoString(, quote_identifier(val));
! 		else
! 		{
! 			/*
! 			 * If we have only one elment, it may be a list
! 			 * that is quoted as a whole.
! 			 */
! 			appendStringInfoString(,
!    quote_list_identifiers(val));
! 		}
! 	}
  	else
  		appendStringInfoString(, val);
  }

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Tom Lane]

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

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Tom Lane]

Kyotaro HORIGUCHI  writes:
> At Fri, 16 Mar 2018 21:15:54 +0900, Michael Paquier  
> wrote in <20180316121554.ga2...@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

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Arthur Zakirov]

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

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Pavel Stehule]

2018-03-16 9:34 GMT+01:00 Kyotaro HORIGUCHI :

> At Fri, 16 Mar 2018 09:16:54 +0100, Pavel Stehule 
> wrote in  zyujawdnu...@mail.gmail.com>
> > 2018-03-16 5:46 GMT+01:00 Michael Paquier :
> >
> > > 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.
> > > 5241258464332317...@wrigleys.postgresql.org
> > > --
> > > Michael
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
>

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Kyotaro HORIGUCHI]

At Fri, 16 Mar 2018 09:16:54 +0100, Pavel Stehule  
wrote in 
> 2018-03-16 5:46 GMT+01:00 Michael Paquier :
> 
> > 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.
> > 5241258464332317...@wrigleys.postgresql.org
> > --
> > Michael

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Pavel Stehule]

2018-03-16 5:46 GMT+01:00 Michael Paquier :

> 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.
> 5241258464332317...@wrigleys.postgresql.org
> --
> Michael
>

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Michael Paquier]

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.5241258464332317...@wrigleys.postgresql.org
--
Michael
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index b58ee3c387..ccd7863c6c 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -2596,11 +2596,23 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
  quote_identifier(configitem));
 
 /*
- * Some GUC variable names are 'LIST' type and hence must not
- * be quoted.
+ * Some GUC variable names are of type GUC_LIST_INPUT and
+ * hence must not be quoted.
+ * XXX: Keep this list in sync with what is defined in guc.c and
+ * any modules using list parameters!
  */
-if (pg_strcasecmp(configitem, "DateStyle") == 0
-	|| pg_strcasecmp(configitem, "search_path") == 0)
+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)
 	appendStringInfoString(, pos);
 else
 	simple_quote_literal(, pos);
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 7f5bb1343e..c39096ebff 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -887,11 +887,23 @@ makeAlterConfigCommand(PGconn *conn, const char *configitem,
 	appendPQExpBuffer(buf, "SET %s TO ", fmtId(mine));
 
 	/*
-	 * Some GUC variable names are 'LIST' type and hence must not be quoted.
-	 * XXX this list is incomplete ...
+	 * Some GUC variable names are of type GUC_LIST_INPUT and hence
+	 * must not be quoted.
+	 * XXX: Keep this list in sync with what is defined in guc.c and
+	 * any modules using list parameters!
 	 */
-	if (pg_strcasecmp(mine, "DateStyle") == 0
-		|| pg_strcasecmp(mine, "search_path") == 0)
+	if (pg_strcasecmp(mine, "DateStyle") == 0 ||
+		pg_strcasecmp(mine, "listen_addresses") == 0 ||
+		pg_strcasecmp(mine, "local_preload_libraries") == 0 ||
+		pg_strcasecmp(mine, "log_destination") == 0 ||
+		pg_strcasecmp(mine, "plpgsql.extra_errors") == 0 ||
+		pg_strcasecmp(mine, "plpgsql.extra_warnings") == 0 ||
+		pg_strcasecmp(mine, "search_path") == 0 ||
+		pg_strcasecmp(mine, "session_preload_libraries") == 0 ||
+		pg_strcasecmp(mine, "shared_preload_libraries") == 0 ||
+		pg_strcasecmp(mine, "synchronous_standby_names") == 0 ||
+		pg_strcasecmp(mine, "temp_tablespaces") == 0 ||
+		pg_strcasecmp(mine, "wal_consistency_checking") == 0)
 		appendPQExpBufferStr(buf, pos);
 	else
 		appendStringLiteralConn(buf, pos, conn);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 566cbf2cda..1ec9d4b0fd 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -11877,11 +11877,23 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
 		appendPQExpBuffer(q, "\nSET %s TO ", fmtId(configitem));
 
 		/*
-		 * Some GUC variable names are 'LIST' type and hence must not be
-		 * quoted.
+		 * Some GUC variable names are of type GUC_LIST_INPUT and hence
+		 * must not be quoted.
+		 * XXX: Keep this list in sync with what is defined in guc.c and
+		 * any modules using list parameters!
 		 */
-		if (pg_strcasecmp(configitem, "DateStyle") == 0
-			|| pg_strcasecmp(configitem, "search_path") == 0)
+		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 ||

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Arthur Zakirov]

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

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Kyotaro HORIGUCHI]

At Thu, 15 Mar 2018 15:09:54 +0900, Michael Paquier  wrote 
in <20180315060954.ge...@paquier.xyz>
> On Thu, Mar 15, 2018 at 02:03:15PM +0900, Kyotaro HORIGUCHI wrote:
> > At Thu, 15 Mar 2018 00:33:25 -0400, Tom Lane  wrote in 
> > <22193.1521088...@sss.pgh.pa.us>
> >> Kyotaro HORIGUCHI  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 
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
+++ 

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Michael Paquier]

On Thu, Mar 15, 2018 at 02:03:15PM +0900, Kyotaro HORIGUCHI wrote:
> At Thu, 15 Mar 2018 00:33:25 -0400, Tom Lane  wrote in 
> <22193.1521088...@sss.pgh.pa.us>
>> Kyotaro HORIGUCHI  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


signature.asc
Description: PGP signature

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Kyotaro HORIGUCHI]

At Thu, 15 Mar 2018 00:33:25 -0400, Tom Lane  wrote in 
<22193.1521088...@sss.pgh.pa.us>
> Kyotaro HORIGUCHI  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

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Tom Lane]

Kyotaro HORIGUCHI  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

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Kyotaro HORIGUCHI]

Hello,

At Wed, 14 Mar 2018 17:46:21 +0900, Michael Paquier  wrote 
in <20180314084621.ga...@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
>From 17cadb17279c802c79e4afa0d7fcd7f63ed38d03 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 15 Mar 2018 13:12:17 +0900
Subject: [PATCH] Autogenerating function to detect list-formatted GUC
 variables

Some features require to know whether a GUC variables is list or not
but the set of such functions can be modified through development. We
tried to check it on-the-fly but found that that makes things more
complex. Instead, this generates the list of such variables by
scanning the whole source tree, instead of manually maintaining it.
---
 src/backend/Makefile|   4 +-
 src/backend/utils/adt/ruleutils.c   |   4 +-
 src/bin/pg_dump/Makefile|   4 +-
 src/bin/pg_dump/dumputils.c |   5 +-
 src/bin/pg_dump/pg_dump.c   |   4 +-
 src/include/Makefile|   7 +-
 src/include/gen_listvars.pl | 145 
 src/test/regress/expected/rules.out |  24 ++
 src/test/regress/sql/rules.sql  |  12 +++
 9 files changed, 197 insertions(+), 12 deletions(-)
 create mode 100755 src/include/gen_listvars.pl

diff --git a/src/backend/Makefile b/src/backend/Makefile
index 4a28267339..10656c53e6 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -169,7 +169,7 @@ submake-schemapg:
 
 .PHONY: generated-headers
 
-generated-headers: $(top_builddir)/src/include/parser/gram.h $(top_builddir)/src/include/catalog/schemapg.h $(top_builddir)/src/include/storage/lwlocknames.h $(top_builddir)/src/include/utils/errcodes.h $(top_builddir)/src/include/utils/fmgroids.h $(top_builddir)/src/include/utils/fmgrprotos.h $(top_builddir)/src/include/utils/probes.h
+generated-headers: $(top_builddir)/src/include/parser/gram.h $(top_builddir)/src/include/catalog/schemapg.h $(top_builddir)/src/include/storage/lwlocknames.h $(top_builddir)/src/include/utils/errcodes.h $(top_builddir)/src/include/utils/fmgroids.h $(top_builddir)/src/include/utils/fmgrprotos.h $(top_builddir)/src/include/utils/probes.h $(top_builddir)/src/include/common/check_listvars.h
 
 $(top_builddir)/src/include/parser/gram.h: parser/gram.h
 	prereqdir=`cd '$(dir $<)' >/dev/null && pwd` && \
@@ -205,6 +205,8 @@ $(top_builddir)/src/include/utils/probes.h: utils/probes.h
 	cd '$(dir $@)' && rm -f $(notdir $@) && \
 	$(LN_S) "../../../$(subdir)/utils/probes.h" .
 
+$(top_builddir)/src/include/common/check_listvars.h:
+	$(MAKE) -C $(dir $@).. common/check_listvars.h
 
 utils/probes.o: utils/probes.d $(SUBDIROBJS)
 	$(DTRACE) $(DTRACEFLAGS) -C -G -s $(call expand_subsys,$^) -o $@
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index b58ee3c387..d2ad034e06 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -41,6 +41,7 @@
 #include "catalog/pg_type.h"
 #include "commands/defrem.h"
 #include "commands/tablespace.h"
+#include "common/check_listvars.h"
 #include "common/keywords.h"
 #include "executor/spi.h"
 #include "funcapi.h"
@@ -2599,8 +2600,7 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
  * 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)
+if (IsConfigOptionAList(configitem))
 	appendStringInfoString(, pos);
 else
 	simple_quote_literal(, pos);
diff --git a/src/bin/pg_dump/Makefile b/src/bin/pg_dump/Makefile

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Kyotaro HORIGUCHI]

Hello,

At Tue, 6 Mar 2018 07:14:00 +0100, Pavel Stehule  
wrote in 

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Pavel Stehule]

2018-03-06 2:32 GMT+01:00 Michael Paquier :

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

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Pavel Stehule]

2018-02-21 8:35 GMT+01:00 Michael Paquier :

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

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Arthur Zakirov]

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

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Arthur Zakirov]

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

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Michael Paquier]

On Thu, Jan 11, 2018 at 10:42:38AM -0500, Tom Lane wrote:
> Michael Paquier  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
diff --git a/src/backend/utils/adt/ruleutils.c 
b/src/backend/utils/adt/ruleutils.c
index 9cdbb06add..7914f4dd88 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -61,6 +61,7 @@
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
+#include "utils/guc.h"
 #include "utils/hsearch.h"
 #include "utils/lsyscache.h"
 #include "utils/rel.h"
@@ -2561,6 +2562,7 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
{
char   *configitem = TextDatumGetCString(d);
char   *pos;
+   int flags;
 
pos = strchr(configitem, '=');
if (pos == NULL)
@@ -2571,11 +2573,11 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
 
quote_identifier(configitem));
 
/*
-* Some GUC variable names are 'LIST' type and 
hence must not
-* be quoted.
+* Some GUC variable names are of type 
GUC_LIST_INPUT
+* and hence must not be quoted.
 */
-   if (pg_strcasecmp(configitem, "DateStyle") == 0
-   || pg_strcasecmp(configitem, 
"search_path") == 0)
+   flags = GetConfigOptionFlags(configitem, false);
+   if ((flags & GUC_LIST_INPUT) != 0)
appendStringInfoString(, pos);
else
simple_quote_literal(, pos);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 72f6be329e..5a39c96242 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -8099,6 +8099,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/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 27628a397c..f9b9de8894 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -11777,11 

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Tom Lane]

Michael Paquier  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(, pos);
> else
> simple_quote_literal(, 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

pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Michael Paquier]

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(, pos);
else
simple_quote_literal(, 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


signature.asc
Description: PGP signature