Re: [PATCH] two-arg current_setting() with fallback

From: Jeevan Chalke <jeevan(dot)chalke(at)gmail(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] two-arg current_setting() with fallback
Date: 2015-06-04 08:17:54
Message-ID: 20150604081754.9937.57332.pgcf@coridan.postgresql.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Jeevan Chalke 2015-06-04 08:25:52 Re: [PATCH] two-arg current_setting() with fallback
Previous Message Nils Goroll 2015-06-04 07:33:46 Re: xid wrap / optimize frozen tables?