Re: [HACKERS] [PATCH] two-arg current_setting() with fallback
On Fri, Jul 3, 2015 at 2:12 AM, Tom Lane wrote: > Jeevan Chalke writes: > > Attached patch which fixes my review comments. > > Applied with minor adjustments (mostly cosmetic, but did neither of you > notice the compiler warning?) > Oops. Sorry for that. Added -Wall -Werror in my configuration. Thanks > regards, tom lane > -- Jeevan B Chalke Principal Software Engineer, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company
Re: [HACKERS] [PATCH] two-arg current_setting() with fallback
Jeevan Chalke writes: > Attached patch which fixes my review comments. Applied with minor adjustments (mostly cosmetic, but did neither of you notice the compiler warning?) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] two-arg current_setting() with fallback
> Date: Thu, 4 Jun 2015 13:55:52 +0530 > From: Jeevan Chalke > To: david.g.johns...@gmail.com > Cc: PostgreSQL Hackers > Subject: Re: [PATCH] two-arg current_setting() with fallback > Message-ID: > > Hi, > > Attached patch which fixes my review comments. > > Since code changes were good, just fixed reported cosmetic changes. > > David, can you please cross check? Hi Jeevan, I’m just on the digest list, and it looks like I didn’t get copied on your response, so missed it until now. Thanks for the review; I reviewed the changes you made and it looks good. Regards, David -- David Christensen PostgreSQL Team Manager End Point Corporation da...@endpoint.com 785-727-1171 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] two-arg current_setting() with fallback
Hi,
Attached patch which fixes my review comments.
Since code changes were good, just fixed reported cosmetic changes.
David, can you please cross check?
Thanks
--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
diff --git a/contrib/tsearch2/tsearch2.c b/contrib/tsearch2/tsearch2.c
index 143dabb..4354c5b 100644
--- a/contrib/tsearch2/tsearch2.c
+++ b/contrib/tsearch2/tsearch2.c
@@ -363,7 +363,7 @@ tsa_tsearch2(PG_FUNCTION_ARGS)
tgargs[i + 1] = trigger->tgargs[i];
tgargs[1] = pstrdup(GetConfigOptionByName("default_text_search_config",
- NULL));
+ NULL, false));
tgargs_old = trigger->tgargs;
trigger->tgargs = tgargs;
trigger->tgnargs++;
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index c6e3540..7f6c4ad 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -16435,7 +16435,7 @@ SELECT collation for ('foo' COLLATE "de_DE");
current_setting
-current_setting(setting_name)
+current_setting(setting_name [, missing_ok ])
text
get current value of setting
@@ -16474,7 +16474,9 @@ SELECT collation for ('foo' COLLATE "de_DE");
The function current_setting yields the
current value of the setting setting_name.
It corresponds to the SQL command
-SHOW. An example:
+SHOW. If setting does not exist,
+throws an error unless missing_ok is
+true. An example:
SELECT current_setting('datestyle');
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index b3c9f14..a43925d 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -7137,7 +7137,7 @@ ExtractSetVariableArgs(VariableSetStmt *stmt)
case VAR_SET_VALUE:
return flatten_set_variable_args(stmt->name, stmt->args);
case VAR_SET_CURRENT:
- return GetConfigOptionByName(stmt->name, NULL);
+ return GetConfigOptionByName(stmt->name, NULL, false);
default:
return NULL;
}
@@ -7206,7 +7206,7 @@ set_config_by_name(PG_FUNCTION_ARGS)
true, 0, false);
/* get the new current value */
- new_value = GetConfigOptionByName(name, NULL);
+ new_value = GetConfigOptionByName(name, NULL, false);
/* Convert return string to text */
PG_RETURN_TEXT_P(cstring_to_text(new_value));
@@ -7633,7 +7633,7 @@ GetPGVariableResultDesc(const char *name)
const char *varname;
/* Get the canonical spelling of name */
- (void) GetConfigOptionByName(name, &varname);
+ (void) GetConfigOptionByName(name, &varname, false);
/* need a tuple descriptor representing a single TEXT column */
tupdesc = CreateTemplateTupleDesc(1, false);
@@ -7656,7 +7656,7 @@ ShowGUCConfigOption(const char *name, DestReceiver *dest)
char *value;
/* Get the value and canonical spelling of name */
- value = GetConfigOptionByName(name, &varname);
+ value = GetConfigOptionByName(name, &varname, false);
/* need a tuple descriptor representing a single TEXT column */
tupdesc = CreateTemplateTupleDesc(1, false);
@@ -7740,19 +7740,26 @@ ShowAllGUCConfig(DestReceiver *dest)
}
/*
- * Return GUC variable value by name; optionally return canonical
- * form of name. Return value is palloc'd.
+ * Return GUC variable value by name; optionally return canonical form of
+ * name. If the GUC is unset, then throw an error unless missing_ok is true,
+ * in which case return NULL. Return value is palloc'd.
*/
char *
-GetConfigOptionByName(const char *name, const char **varname)
+GetConfigOptionByName(const char *name, const char **varname, bool missing_ok)
{
struct config_generic *record;
record = find_option(name, false, ERROR);
if (record == NULL)
+ {
+ if (missing_ok)
+ return NULL;
+
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("unrecognized configuration parameter \"%s\"", name)));
+ }
+
if ((record->flags & GUC_SUPERUSER_ONLY) && !superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -8046,12 +8053,42 @@ show_config_by_name(PG_FUNCTION_ARGS)
varname = TextDatumGetCString(PG_GETARG_DATUM(0));
/* Get the value */
- varval = GetConfigOptionByName(varname, NULL);
+ varval = GetConfigOptionByName(varname, NULL, false);
+
+ /* Convert to text */
+ PG_RETURN_TEXT_P(cstring_to_text(varval));
+}
+
+/*
+ * show_config_by_name_missing_ok - equiv to SHOW X command but implemented as
+ * a function. If X does not exist, suppress the error and just return NULL
+ * if missing_ok is TRUE.
+ */
+Datum
+show_config_by_name_missing_ok(PG_FUNCTION_ARGS)
+{
+ char *varname;
+ bool missing_ok;
+ char *varval;
+
+ /* Get the GUC variable name */
+ varname = TextDatumGetCString(PG_GETARG_DATUM(0));
+
+ /* Get the missing_ok value */
+ missing_ok = PG_GETARG_BOOL(1);
+
+ /* Get the value */
+ varval = GetConfigOptionByName(varname, NULL, missing_ok);
+
+ /* return NULL if this was NULL */
+ if (!varval)
+ PG_RETURN_NULL(
Re: [HACKERS] [PATCH] two-arg current_setting() with fallback
The following review has been posted through the commitfest application: make installcheck-world: tested, failed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed I have reviewed the patch. Here are my review comments: 1. Patch does not apply due to some recent changes in pg_proc.h 2. -GetConfigOptionByName(const char *name, const char **varname) +GetConfigOptionByNameMissingOK(const char *name, const char **varname, bool missing_ok) Will it be better if we keep the name as is and change the callers to pass false for missing_ok parameter? It looks weired to have an extra #define just to avoid that. I see countable callers and thus see NO issues changing those. 3. Oid used for new function is already used. Check unused_oids.sh. 4. Changes in builtins.h are accidental. Need to remove that. However, code changes looks good and implements the desired feature. The new status of this patch is: Waiting on Author -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] two-arg current_setting() with fallback
Hello, >Well, speaking of the two-arg form vs alternate name, here's a version of the patch which includes the >new behavior Thought i will attempt a review. The patch applies cleanly to latest HEAD. patch -p1 < /home/Sameer/Downloads/0001-Add-two-arg-form-of-current_setting-to-optionally-su.patch patching file doc/src/sgml/func.sgml Hunk #1 succeeded at 16216 (offset 1 line). Hunk #2 succeeded at 16255 (offset 1 line). patching file src/backend/utils/misc/guc.c Hunk #1 succeeded at 7693 (offset -3 lines). Hunk #2 succeeded at 8012 (offset -3 lines). patching file src/include/catalog/pg_proc.h Hunk #1 succeeded at 3044 (offset 4 lines). patching file src/include/utils/builtins.h patching file src/include/utils/guc.h patching file src/test/regress/expected/guc.out patching file src/test/regress/sql/guc.sql But i do get error at make make -C catalog schemapg.h make[3]: Entering directory `/home/Sameer/git/latest_postgres/postgres/src/backend/catalog' cd ../../../src/include/catalog && '/usr/bin/perl' ./duplicate_oids 3280 make[3]: *** [postgres.bki] Error 1 make[3]: Leaving directory `/home/Sameer/git/latest_postgres/postgres/src/backend/catalog' make[2]: *** [submake-schemapg] Error 2 make[2]: Leaving directory `/home/Sameer/git/latest_postgres/postgres/src/backend' make[1]: *** [all-backend-recurse] Error 2 make[1]: Leaving directory `/home/Sameer/git/latest_postgres/postgres/src' make: *** [all-src-recurse] Error 2 regards Sameer -- View this message in context: http://postgresql.nabble.com/PATCH-two-arg-current-setting-with-fallback-tp5842654p5847904.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] two-arg current_setting() with fallback
> On Mar 20, 2015, at 11:10 AM, David G. Johnston > wrote: > > On Fri, Mar 20, 2015 at 9:04 AM, Robert Haas wrote: > On Fri, Mar 20, 2015 at 10:54 AM, David Christensen > wrote: > > In that case, the other thought I had here is that we change the function > > signature of current_setting() to be a two-arg form where the second > > argument is a boolean "throw_error", with a default argument of true to > > preserve existing semantics, and returning NULL if that argument is false. > > However, I'm not sure if there are some issues with changing the signature > > of an existing function (e.g., with pg_upgrade, etc.). > > > > My *impression* is that since pg_upgrade rebuilds the system tables for a > > new install it shouldn't be an issue, but not sure if having the same > > pg_proc OID with different values or an alternate pg_proc OID would cause > > issues down the line; anyone know if this is a dead-end? > > I think if the second argument is defaulted it would be OK. However > it might make sense to instead add a new two-argument function and > leave the existing one-argument function alone, because setting > default arguments for functions defined in pg_proc.h is kind of a > chore. > > Isn't there some other update along this whole error-vs-null choice going > around where a separate name was chosen for the new null-returning function > instead of adding a boolean switch argument? Well, speaking of the two-arg form vs alternate name, here's a version of the patch which includes the new behavior. (I couldn't think of a good name to expose for an alternate function, but I'm open to suggestions.) Regards, David -- David Christensen End Point Corporation da...@endpoint.com 785-727-1171 0001-Add-two-arg-form-of-current_setting-to-optionally-su.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] two-arg current_setting() with fallback
On Fri, Mar 20, 2015 at 9:04 AM, Robert Haas wrote: > On Fri, Mar 20, 2015 at 10:54 AM, David Christensen > wrote: > > In that case, the other thought I had here is that we change the > function signature of current_setting() to be a two-arg form where the > second argument is a boolean "throw_error", with a default argument of true > to preserve existing semantics, and returning NULL if that argument is > false. However, I'm not sure if there are some issues with changing the > signature of an existing function (e.g., with pg_upgrade, etc.). > > > > My *impression* is that since pg_upgrade rebuilds the system tables for > a new install it shouldn't be an issue, but not sure if having the same > pg_proc OID with different values or an alternate pg_proc OID would cause > issues down the line; anyone know if this is a dead-end? > > I think if the second argument is defaulted it would be OK. However > it might make sense to instead add a new two-argument function and > leave the existing one-argument function alone, because setting > default arguments for functions defined in pg_proc.h is kind of a > chore. > Isn't there some other update along this whole error-vs-null choice going around where a separate name was chosen for the new null-returning function instead of adding a boolean switch argument? David J.
Re: [HACKERS] [PATCH] two-arg current_setting() with fallback
On Fri, Mar 20, 2015 at 10:54 AM, David Christensen wrote: > In that case, the other thought I had here is that we change the function > signature of current_setting() to be a two-arg form where the second argument > is a boolean "throw_error", with a default argument of true to preserve > existing semantics, and returning NULL if that argument is false. However, > I'm not sure if there are some issues with changing the signature of an > existing function (e.g., with pg_upgrade, etc.). > > My *impression* is that since pg_upgrade rebuilds the system tables for a new > install it shouldn't be an issue, but not sure if having the same pg_proc OID > with different values or an alternate pg_proc OID would cause issues down the > line; anyone know if this is a dead-end? I think if the second argument is defaulted it would be OK. However it might make sense to instead add a new two-argument function and leave the existing one-argument function alone, because setting default arguments for functions defined in pg_proc.h is kind of a chore. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] two-arg current_setting() with fallback
> On Mar 19, 2015, at 6:27 PM, Tom Lane wrote:
>
> David Christensen writes:
>> The two-arg form of the current_setting() function will allow a
>> fallback value to be returned instead of throwing an error when an
>> unknown GUC is provided. This would come in most useful when using
>> custom GUCs; e.g.:
>
>> -- errors out if the 'foo.bar' setting is unset
>> SELECT current_setting('foo.bar');
>
>> -- returns current setting of foo.bar, or 'default' if not set
>> SELECT current_setting('foo.bar', 'default')
>
>> This would save you having to wrap the use of the function in an
>> exception block just to catch and utilize a default setting value
>> within a function.
>
> That seems kind of ugly, not least because it assumes that you know
> a value that couldn't be mistaken for a valid value of the GUC.
> (I realize that you are thinking of cases where you want to pretend
> that the GUC has some valid value, but that's not the only use case.)
>
> ISTM, since we don't allow GUCs to have null values, it'd be better to
> define the variant function as returning NULL for no-such-GUC. Then the
> behavior you want could be achieved by wrapping that in a COALESCE, but
> the behavior of probing whether the GUC is set at all would be achieved
> with an IS NULL test.
>
> regards, tom lane
In that case, the other thought I had here is that we change the function
signature of current_setting() to be a two-arg form where the second argument
is a boolean "throw_error", with a default argument of true to preserve
existing semantics, and returning NULL if that argument is false. However, I'm
not sure if there are some issues with changing the signature of an existing
function (e.g., with pg_upgrade, etc.).
My *impression* is that since pg_upgrade rebuilds the system tables for a new
install it shouldn't be an issue, but not sure if having the same pg_proc OID
with different values or an alternate pg_proc OID would cause issues down the
line; anyone know if this is a dead-end?
Regards,
David
--
David Christensen
End Point Corporation
da...@endpoint.com
785-727-1171
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] two-arg current_setting() with fallback
David Christensen writes:
> The two-arg form of the current_setting() function will allow a
> fallback value to be returned instead of throwing an error when an
> unknown GUC is provided. This would come in most useful when using
> custom GUCs; e.g.:
> -- errors out if the 'foo.bar' setting is unset
> SELECT current_setting('foo.bar');
> -- returns current setting of foo.bar, or 'default' if not set
> SELECT current_setting('foo.bar', 'default')
> This would save you having to wrap the use of the function in an
> exception block just to catch and utilize a default setting value
> within a function.
That seems kind of ugly, not least because it assumes that you know
a value that couldn't be mistaken for a valid value of the GUC.
(I realize that you are thinking of cases where you want to pretend
that the GUC has some valid value, but that's not the only use case.)
ISTM, since we don't allow GUCs to have null values, it'd be better to
define the variant function as returning NULL for no-such-GUC. Then the
behavior you want could be achieved by wrapping that in a COALESCE, but
the behavior of probing whether the GUC is set at all would be achieved
with an IS NULL test.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] two-arg current_setting() with fallback
Apologies if this is a double-post.
Enclosed is a patch that creates a two-arg current_setting() function. From
the commit message:
The two-arg form of the current_setting() function will allow a
fallback value to be returned instead of throwing an error when an
unknown GUC is provided. This would come in most useful when using
custom GUCs; e.g.:
-- errors out if the 'foo.bar' setting is unset
SELECT current_setting('foo.bar');
-- returns current setting of foo.bar, or 'default' if not set
SELECT current_setting('foo.bar', 'default')
This would save you having to wrap the use of the function in an
exception block just to catch and utilize a default setting value
within a function.
0001-Add-two-arg-for-of-current_setting-NAME-FALLBACK.patch
Description: Binary data
--
David Christensen
End Point Corporation
da...@endpoint.com
785-727-1171
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
