From: | Rodolfo Campero <rodolfo(dot)campero(at)anachronics(dot)com> |
---|---|
To: | Peter Eisentraut <peter_e(at)gmx(dot)net> |
Cc: | Amit Khandekar <amit(dot)khandekar(at)enterprisedb(dot)com>, Ali Dar <ali(dot)munir(dot)dar(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: information schema parameter_default implementation |
Date: | 2013-11-22 12:37:25 |
Message-ID: | CAHNrXgF8K1ti8-OJ9u+Th2zzumSi=3p4Qy8JMywvP6bDbf+nsQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Review: information schema parameter_default implementation (v2)
This is a review of the patch submitted in
http://archives.postgresql.org/message-id/1384483678.5008.1.camel@vanquo.pezone.net
(information schema parameter_default implementation).
Previous review from Amit Khandekar covers technical aspects:
http://www.postgresql.org/message-id/CACoZds0eB3-yZ+pVLp9Sf5Xs_9xsDZ=JdP1D83RA-hjvJJofiA@mail.gmail.com
Submission review
=================
* Is the patch in a patch format which has context? (eg: context diff
format)
Yes
* Does it apply cleanly to the current git master?
I had to apply "fromdos" to remove trailing whitespace.
After that, the patch applies cleanly to HEAD.
Make builds without warnings, except for:
In file included from gram.y:13675:0:
scan.c: In function ‘yy_try_NUL_trans’:
scan.c:10185:23: warning: unused variable ‘yyg’ [-Wunused-variable]
but from previous messages in this mailing list I think that's unrelated to
this patch and normal.
The regression tests all pass successfully against the new patch.
* Does it include reasonable tests, necessary doc patches, etc?
Yes
Usability review
================
* Does the patch actually implement that?
The patch implements the column "parameter_default" of information schema
view "parameters", defined in the SQL:2011 standard.
I could not get a hand to the spec, but I found a document where it is
mentioned: http://msdn.microsoft.com/en-us/library/jj191733(v=sql.105).aspx
* Do we want that?
I think we do, as it is defined in the spec.
* Do we already have it?
No
* Does it follow SQL spec, or the community-agreed behavior?
SQL:2011.
* Does it include pg_dump support (if applicable)?
N/A
* Are there dangers?
None AFAICS.
* Have all the bases been covered?
Yes.
Feature test
============
* Does the feature work as advertised?
Yes
* Are there corner cases the author has failed to consider?
None that I can see.
* Are there any assertion failures or crashes?
No
Performance review
==================
N/A
Coding review
=============
I'm not skilled enough to do a code review; see previous review from Amit:
http://www.postgresql.org/message-id/CACoZds0eB3-yZ+pVLp9Sf5Xs_9xsDZ=JdP1D83RA-hjvJJofiA@mail.gmail.com
2013/11/21 Peter Eisentraut <peter_e(at)gmx(dot)net>
> On 11/20/13, 8:39 PM, Rodolfo Campero wrote:
> > 2013/11/20 Peter Eisentraut <peter_e(at)gmx(dot)net <mailto:peter_e(at)gmx(dot)net>>
> >
> > Updated patch
> >
> >
> > I can't apply the patch; maybe I'm doing something wrong?
>
> It looks like you are not in the right directory.
>
>
--
Rodolfo Campero
Anachronics S.R.L.
Tel: (54 11) 4899 2088
rodolfo(dot)campero(at)anachronics(dot)com
http://www.anachronics.com
From | Date | Subject | |
---|---|---|---|
Next Message | Andrew Gierth | 2013-11-22 12:53:10 | Re: UNNEST with multiple args, and TABLE with multiple funcs |
Previous Message | Fabrízio de Royes Mello | 2013-11-22 12:25:49 | Re: [PATCH] Store Extension Options |