Thread: [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.



--
David Christensen
End Point Corporation
david@endpoint.com
785-727-1171

David Christensen <david@endpoint.com> 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

> On Mar 19, 2015, at 6:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Christensen <david@endpoint.com> 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-argform 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
thesignature 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
sureif having the same pg_proc OID with different values or an alternate pg_proc OID would cause issues down the line;
anyoneknow if this is a dead-end? 

Regards,

David
--
David Christensen
End Point Corporation
david@endpoint.com
785-727-1171

On Fri, Mar 20, 2015 at 10:54 AM, David Christensen <david@endpoint.com> wrote:
> In that case, the other thought I had here is that we change the function signature of current_setting() to be a
two-argform 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
thesignature 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
notsure 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

> On Mar 20, 2015, at 11:10 AM, David G. Johnston <david.g.johnston@gmail.com> wrote:
>
> On Fri, Mar 20, 2015 at 9:04 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Mar 20, 2015 at 10:54 AM, David Christensen <david@endpoint.com> wrote:
> > In that case, the other thought I had here is that we change the function signature of current_setting() to be a
two-argform 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
thesignature 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
notsure 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
forthe 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.
(Icouldn'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
david@endpoint.com
785-727-1171

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.

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

> Date: Thu, 4 Jun 2015 13:55:52 +0530
> From: Jeevan Chalke <jeevan.chalke@enterprisedb.com>
> To: david.g.johnston@gmail.com
> Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
> Subject: Re: [PATCH] two-arg current_setting() with fallback
> Message-ID: <CAM2+6=VsTg=pAEdq6ioKpoLx7yapE9cs3UvFbNV310Ur7UpqhQ@mail.gmail.com>

> 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
david@endpoint.com
785-727-1171

Jeevan Chalke <jeevan.chalke@enterprisedb.com> 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