[PATCH] two-arg current_setting() with fallback

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jul 3, 2015 at 2:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

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