[Review] Re: minor patch submission: CREATE CAST ... AS EXPLICIT

Le lundi 17 juin 2013 00:02:21, Fabien COELHO a écrit :
> >> What activity would you expect?
> >
> > A patch which applies cleanly to git HEAD. This one doesn't for me,
> > although I'm not really sure why, I don't see any obvious conflicts.
>
> Please find attached a freshly generated patch against current master.

* Submission review:
patch is in unified format and apply on HEAD.
patch contains documentation, however I believe 'AS IMPLICIT' is a PostgreSQL
extension with special behavior and 'AS EXPLICIT' respect the standard except
that PostgreSQL adds only the expression 'AS EXPLICIT' (it is also the default
in the standard). So maybe it is possible to rephrase this piece:

@@ -411,8 +427,8 @@ CREATE CAST (bigint AS int4) WITH FUNCTION int4(bigint) AS
ASSIGNMENT;
<acronym>SQL</acronym> standard,
except that SQL does not make provisions for binary-coercible
types or extra arguments to implementation functions.
- <literal>AS IMPLICIT</> is a <productname>PostgreSQL</productname>
- extension, too.
+ <literal>AS IMPLICIT</> and <literal>AS EXPLICIT</> are
+ a <productname>PostgreSQL</productname> extension, too.
</para>
</refsect1>

After digging in the archive and the CF: original request is at
https://commitfest.postgresql.org/action/patch_view?id=563

* Usability review
** Does the patch actually implement that? yes
** Do we want that?
Back in 2012 Tom exposed arguments against it, or at least not a clear +1.
The patch add nothing but more explicit creation statement, it has gone
untouched for 2 years without interest so it is surely not something really
important for PostgreSQL users. However we already have non-standard words for
CREATE CAST, this new one is not very intrusive .

** Does it follow SQL spec, or the community-agreed behavior?
It does not follow SQL spec.

** Does it include pg_dump support (if applicable)?
Not but it is probably not interesting to add that to the pg_dump output: it
increases incompatibility with SQL spec for no gain. The result is that the
patch only allows to CREATE CAST..AS EXPLICIT without error. Then pg_dump
won't know if the CAST has been created with the default or an 'explicit
default'...

** Are there dangers?
It seems no.

* Feature test
** Does the feature work as advertised? Yes
** Are there corner cases the author has failed to consider?
I think no, but my skills with the parser are limited (gram.y, ...)
** Are there any assertion failures or crashes?
no

* Performance review: not relevant.

* Coding review
Patch does not pass test:
./check_keywords.pl gram.y ../../../src/include/parser/kwlist.h

I had to update the unreserved keyword list in order to be able to build
postgresql.

** Does it follow the project coding guidelines? yes
** Are there portability issues? no (only for SQL)
** Will it work on Windows/BSD etc? yes
** Are the comments sufficient and accurate? Yes
** Does it do what it says, correctly? Yes
** Does it produce compiler warnings? don't build as is. Need patch update
** Can you make it crash? no

* Architecture review
** Is everything done in a way that fits together coherently with other
features/modules? Yes
** Are there interdependencies that can cause problems? No.

I flag it 'return with feedback', please update the patch so it builds.
Everything else is ok.
--
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation