Re: [Review] Re: [HACKERS] minor patch submission: CREATE CAST ... AS EXPLICIT
On 07/09/2013 01:10 AM, Fabien COELHO wrote: Where are we with this patch? Fabien, are you going to submit an updated version which addresses the objections, or should I mark it Returned With Feedback? There is no need for an updated patch. I addressed the objections with words, not code:-) So, Tom, Robert, Cedric: can we have a verdict? Commit or no? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [Review] Re: [HACKERS] minor patch submission: CREATE CAST ... AS EXPLICIT
Josh Berkus j...@agliodbs.com writes: On 07/09/2013 01:10 AM, Fabien COELHO wrote: Where are we with this patch? Fabien, are you going to submit an updated version which addresses the objections, or should I mark it Returned With Feedback? There is no need for an updated patch. I addressed the objections with words, not code:-) So, Tom, Robert, Cedric: can we have a verdict? Commit or no? My vote is still no, because of (1) the keyword-creep issue, and (2) the fact that this is proposing to invent non-standard syntax for functionality that's in the SQL standard. 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
Re: [Review] Re: [HACKERS] minor patch submission: CREATE CAST ... AS EXPLICIT
On 07/16/2013 03:12 PM, Tom Lane wrote: Josh Berkus j...@agliodbs.com writes: On 07/09/2013 01:10 AM, Fabien COELHO wrote: Where are we with this patch? Fabien, are you going to submit an updated version which addresses the objections, or should I mark it Returned With Feedback? There is no need for an updated patch. I addressed the objections with words, not code:-) So, Tom, Robert, Cedric: can we have a verdict? Commit or no? My vote is still no, because of (1) the keyword-creep issue, and (2) the fact that this is proposing to invent non-standard syntax for functionality that's in the SQL standard. Ok, marking Returned with Feedback. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [Review] Re: [HACKERS] minor patch submission: CREATE CAST ... AS EXPLICIT
Generally speaking, I agree with Robert's objection. The patch in itself adds only one unnecessary keyword, which probably wouldn't be noticeable, but the argument for it implies that we should be willing to add a lot more equally-unnecessary keywords, which I'm not. gram.o is already about 10% of the entire postgres executable, which probably goes far towards explaining why its inner loop always shows up high in profiling: cache misses are routine. And the size of those tables is at least linear in the number of keywords --- perhaps worse than linear, I'm not sure. Adding a bunch of keywords *will* cost us in performance. I'm not willing to pay that cost for something that adds neither features nor spec compliance. (1) Here are objective measures of the postgres stripped binary size compiled from scratch on my laptop this morning: amd64, gcc version 4.7.3 (Debian 4.7.3-4) then gcc version 4.8.1 (Debian 4.8.1-6) with no option to configure, I got: - without AS EXPLICIT: 5286408 bytes gram.o: 904936 bytes keywords.o: 20392 bytes keywords.o : 20376 bytes gram.o: 909240 bytes keywords.o : 20376 bytes gram.o: 900504 bytes - with AS EXPLICIT : 5282312 bytes gram.o: 901632 bytes keywords.o: 20440 bytes keywords.o : 20424 bytes gram.o: 905904 bytes keywords.o : 20424 bytes gram.o: 897168 bytes The executable binary is reduced by 4 KB with AS EXPLICIT, which seems to come from some ld flucke really, because the only difference I've found are the 8 bytes added to keywords.o. This must be very specific to the version and options used with gcc ld on my laptop. same here, amd64. gcc to more impact, I didn't tryed with clang. As for the general issue with the parser size: I work with languages and compilers as a researcher. We had issues at one point with a scanner because of too many keywords, and we solved it by removing keywords from the scanner and checking them semantically in the parser with a hash table, basically as suggested by Andres. The SQL syntax is pretty redundant so that there are little choices at each point. Some tools can generate perfect hash functions that can help (e.g. gperf). However I'm not sure of the actual impact in size and time by following this path, I'm just sure that there would be less keywords. IMHO, this issue is orthogonal independent from this patch. Finally, to answer your question directly, I'm really a nobody here, so you can do whatever pleases you with the patch. I have no strong objection to the patch. It is only decoration and should not hurt. -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation signature.asc Description: This is a digitally signed message part.
Re: [Review] Re: [HACKERS] minor patch submission: CREATE CAST ... AS EXPLICIT
Hello Josh, Generally speaking, I agree with Robert's objection. The patch in itself adds only one unnecessary keyword, which probably wouldn't be noticeable, but the argument for it implies that we should be willing to add a lot more equally-unnecessary keywords, which I'm not. gram.o is already about 10% of the entire postgres executable, which probably goes far towards explaining why its inner loop always shows up high in profiling: cache misses are routine. And the size of those tables is at least linear in the number of keywords --- perhaps worse than linear, I'm not sure. Adding a bunch of keywords *will* cost us in performance. I'm not willing to pay that cost for something that adds neither features nor spec compliance. (1) Here are objective measures of the postgres stripped binary size compiled from scratch on my laptop this morning: - without AS EXPLICIT: 5286408 bytes gram.o: 904936 bytes keywords.o: 20392 bytes - with AS EXPLICIT : 5282312 bytes gram.o: 901632 bytes keywords.o: 20440 bytes The executable binary is reduced by 4 KB with AS EXPLICIT, which seems to come from some ld flucke really, because the only difference I've found are the 8 bytes added to keywords.o. This must be very specific to the version and options used with gcc ld on my laptop. (2) user annoyance vs cycles I strongly disagree that user annoyance is of little value. This is one of the reason why I cannot stand MySQL (oops, EXCEPT is not implemented, casts are not for all types, the default engine is not ACID, the standard information_schema implementation does *NOT* implement the standard...). I've made an extensive argument earlier in the thread. Where are we with this patch? Fabien, are you going to submit an updated version which addresses the objections, or should I mark it Returned With Feedback? There is no need for an updated patch. I addressed the objections with words, not code:-) I've stronly argued against the premises of Robert Tom objections. Moreover, it happens that the patch does reduce, because of some flucke, the executable size, which is one of the motivation for the objections. I have no doubt that the potential cache misses induced by the 8 bytes extension of the keyword table are of less value that user time and annoyance. As for the general issue with the parser size: I work with languages and compilers as a researcher. We had issues at one point with a scanner because of too many keywords, and we solved it by removing keywords from the scanner and checking them semantically in the parser with a hash table, basically as suggested by Andres. The SQL syntax is pretty redundant so that there are little choices at each point. Some tools can generate perfect hash functions that can help (e.g. gperf). However I'm not sure of the actual impact in size and time by following this path, I'm just sure that there would be less keywords. IMHO, this issue is orthogonal independent from this patch. Finally, to answer your question directly, I'm really a nobody here, so you can do whatever pleases you with the patch. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [Review] Re: [HACKERS] minor patch submission: CREATE CAST ... AS EXPLICIT
On 06/24/2013 06:55 AM, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: What about simply not using a keyword at that location at all? Something like the attached hack? Generally speaking, I agree with Robert's objection. The patch in itself adds only one unnecessary keyword, which probably wouldn't be noticeable, but the argument for it implies that we should be willing to add a lot more equally-unnecessary keywords, which I'm not. gram.o is already about 10% of the entire postgres executable, which probably goes far towards explaining why its inner loop always shows up high in profiling: cache misses are routine. And the size of those tables is at least linear in the number of keywords --- perhaps worse than linear, I'm not sure. Adding a bunch of keywords *will* cost us in performance. I'm not willing to pay that cost for something that adds neither features nor spec compliance. Where are we with this patch? Fabien, are you going to submit an updated version which addresses the objections, or should I mark it Returned With Feedback? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [Review] Re: [HACKERS] minor patch submission: CREATE CAST ... AS EXPLICIT
Hello Fabien, I flag it 'return with feedback', please update the patch so it builds. Everything else is ok. Here it is. The patch does not apply and git also whines about trailing space. It needs a v3... Please note that a community-agreed behavior on this patch is not yet acquired, you should consider that too. -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation signature.asc Description: This is a digitally signed message part.
Re: [Review] Re: [HACKERS] minor patch submission: CREATE CAST ... AS EXPLICIT
Here it is.
The patch does not apply and git also whines about trailing space.
It needs a v3...
The attachement here works for me.
Could you be more precise about the issue?
postgresql git branch test master
postgresql git checkout test
Switched to branch 'test'
postgresql patch -p1 ../as_explicit-v2.patch
patching file doc/src/sgml/ref/create_cast.sgml
patching file src/backend/parser/gram.y
patching file src/include/parser/kwlist.h
patching file src/test/regress/expected/create_cast.out
patching file src/test/regress/sql/create_cast.sql
Please note that a community-agreed behavior on this patch is not yet
acquired, you should consider that too.
Sure. I've sent my argumentation against Robert objections.
--
Fabien.diff --git a/doc/src/sgml/ref/create_cast.sgml b/doc/src/sgml/ref/create_cast.sgml
index 29ea298..0ace996 100644
--- a/doc/src/sgml/ref/create_cast.sgml
+++ b/doc/src/sgml/ref/create_cast.sgml
@@ -20,15 +20,15 @@
synopsis
CREATE CAST (replaceablesource_type/replaceable AS replaceabletarget_type/replaceable)
WITH FUNCTION replaceablefunction_name/replaceable (replaceableargument_type/replaceable [, ...])
-[ AS ASSIGNMENT | AS IMPLICIT ]
+[ AS ASSIGNMENT | AS EXPLICIT | AS IMPLICIT ]
CREATE CAST (replaceablesource_type/replaceable AS replaceabletarget_type/replaceable)
WITHOUT FUNCTION
-[ AS ASSIGNMENT | AS IMPLICIT ]
+[ AS ASSIGNMENT | AS EXPLICIT | AS IMPLICIT ]
CREATE CAST (replaceablesource_type/replaceable AS replaceabletarget_type/replaceable)
WITH INOUT
-[ AS ASSIGNMENT | AS IMPLICIT ]
+[ AS ASSIGNMENT | AS EXPLICIT | AS IMPLICIT ]
/synopsis
/refsynopsisdiv
@@ -74,7 +74,8 @@ SELECT CAST(42 AS float8);
/para
para
- By default, a cast can be invoked only by an explicit cast request,
+ By default, or if the cast is declared literalAS EXPLICIT/,
+ a cast can be invoked only by an explicit cast request,
that is an explicit literalCAST(replaceablex/ AS
replaceabletypename/)/literal or
replaceablex/literal::/replaceabletypename/
@@ -239,6 +240,21 @@ SELECT CAST ( 2 AS numeric ) + 4.0;
/varlistentry
varlistentry
+ termliteralAS EXPLICIT/literal/term
+
+ listitem
+ para
+ Indicates that the cast can be invoked only with an explicit
+ cast request, that is an explicit literalCAST(replaceablex/ AS
+ replaceabletypename/)/literal or
+ replaceablex/literal::/replaceabletypename/
+ construct.
+ This is the default.
+ /para
+ /listitem
+/varlistentry
+
+varlistentry
termliteralAS IMPLICIT/literal/term
listitem
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 5094226..2c0694f 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -533,7 +533,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
DICTIONARY DISABLE_P DISCARD DISTINCT DO DOCUMENT_P DOMAIN_P DOUBLE_P DROP
EACH ELSE ENABLE_P ENCODING ENCRYPTED END_P ENUM_P ESCAPE EVENT EXCEPT
- EXCLUDE EXCLUDING EXCLUSIVE EXECUTE EXISTS EXPLAIN
+ EXCLUDE EXCLUDING EXCLUSIVE EXECUTE EXISTS EXPLAIN EXPLICIT
EXTENSION EXTERNAL EXTRACT
FALSE_P FAMILY FETCH FIRST_P FLOAT_P FOLLOWING FOR FORCE FOREIGN FORWARD
@@ -6720,6 +6720,7 @@ CreateCastStmt: CREATE CAST '(' Typename AS Typename ')'
cast_context: AS IMPLICIT_P { $$ = COERCION_IMPLICIT; }
| AS ASSIGNMENT { $$ = COERCION_ASSIGNMENT; }
+ | AS EXPLICIT { $$ = COERCION_EXPLICIT; }
| /*EMPTY*/{ $$ = COERCION_EXPLICIT; }
;
@@ -12723,6 +12724,7 @@ unreserved_keyword:
| EXCLUSIVE
| EXECUTE
| EXPLAIN
+ | EXPLICIT
| EXTENSION
| EXTERNAL
| FAMILY
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index 68a13b7..f97389b 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -149,6 +149,7 @@ PG_KEYWORD(exclusive, EXCLUSIVE, UNRESERVED_KEYWORD)
PG_KEYWORD(execute, EXECUTE, UNRESERVED_KEYWORD)
PG_KEYWORD(exists, EXISTS, COL_NAME_KEYWORD)
PG_KEYWORD(explain, EXPLAIN, UNRESERVED_KEYWORD)
+PG_KEYWORD(explicit, EXPLICIT, UNRESERVED_KEYWORD)
PG_KEYWORD(extension, EXTENSION, UNRESERVED_KEYWORD)
PG_KEYWORD(external, EXTERNAL, UNRESERVED_KEYWORD)
PG_KEYWORD(extract, EXTRACT, COL_NAME_KEYWORD)
diff --git a/src/test/regress/expected/create_cast.out b/src/test/regress/expected/create_cast.out
index 56cd86e..a8858fa 100644
--- a/src/test/regress/expected/create_cast.out
+++ b/src/test/regress/expected/create_cast.out
@@ -27,8 +27,8 @@ ERROR: function casttestfunc(text) does not exist
LINE 1: SELECT casttestfunc('foo'::text);
^
HINT: No function matches the given name and argument types. You might need to add explicit type casts.
--- Try binary coercion cast
-CREATE CAST (text AS casttesttype) WITHOUT FUNCTION;
+-- Try binary coercion cast and verbose AS EXPLICIT
+CREATE CAST (text AS casttesttype) WITHOUT FUNCTION AS EXPLICIT;
Re: [Review] Re: [HACKERS] minor patch submission: CREATE CAST ... AS EXPLICIT
On 2013-06-22 15:10:07 -0400, Robert Haas wrote:
On Sat, Jun 22, 2013 at 9:16 AM, Cédric Villemain
ced...@2ndquadrant.com wrote:
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).
I object to this patch. This patch a new keyword, EXPLICIT, for
reasons that are strictly cosmetic. Everything that you can do with
this patch can also be done without this patch. It is not a good idea
to slow down parsing of every SQL statement we have just so that
someone can write CREATE CAST .. AS EXPLICIT. Granted, the parsing
slowdown for just one keyword is probably not noticeable, but it's
cumulative with every new keyword we add. Adding them to support new
features is one thing, but adding them to support purely optional
syntax is, I think, going too far.
What about simply not using a keyword at that location at all? Something
like the attached hack?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 5094226..8021f96 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -6718,8 +6718,15 @@ CreateCastStmt: CREATE CAST '(' Typename AS Typename ')'
}
;
-cast_context: AS IMPLICIT_P { $$ = COERCION_IMPLICIT; }
- | AS ASSIGNMENT { $$ = COERCION_ASSIGNMENT; }
+cast_context: AS name
+{
+ if (pg_strcasecmp($2, EXPLICIT) == 0)
+ $$ = COERCION_EXPLICIT;
+ else if (pg_strcasecmp($2, IMPLICIT) == 0)
+ $$ = COERCION_IMPLICIT;
+ else
+ elog(ERROR, frak!);
+}
| /*EMPTY*/{ $$ = COERCION_EXPLICIT; }
;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [Review] Re: [HACKERS] minor patch submission: CREATE CAST ... AS EXPLICIT
Le lundi 24 juin 2013 11:44:21, Fabien COELHO a écrit : Here it is. The patch does not apply and git also whines about trailing space. It needs a v3... The attachement here works for me. Could you be more precise about the issue? postgresql git branch test master postgresql git checkout test Switched to branch 'test' postgresql patch -p1 ../as_explicit-v2.patch patching file doc/src/sgml/ref/create_cast.sgml patching file src/backend/parser/gram.y patching file src/include/parser/kwlist.h patching file src/test/regress/expected/create_cast.out patching file src/test/regress/sql/create_cast.sql Ah, got it. 'git apply' is more strict. Patch apply with patch -p1 ( I though I tryed, but it seems not) == patch -p1 as_explicit-v2.patch (Stripping trailing CRs from patch.) patching file doc/src/sgml/ref/create_cast.sgml (Stripping trailing CRs from patch.) patching file src/backend/parser/gram.y (Stripping trailing CRs from patch.) patching file src/include/parser/kwlist.h (Stripping trailing CRs from patch.) patching file src/test/regress/expected/create_cast.out (Stripping trailing CRs from patch.) patching file src/test/regress/sql/create_cast.sql == -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation signature.asc Description: This is a digitally signed message part.
Re: [Review] Re: [HACKERS] minor patch submission: CREATE CAST ... AS EXPLICIT
Andres Freund and...@2ndquadrant.com writes: What about simply not using a keyword at that location at all? Something like the attached hack? Hack is much too polite a word for that. This will for example fail to respect the difference between quoted and unquoted words. If the argument for this patch is to make the syntax more regular and less surprising, I hardly think that we should add surprise of a different sort. Generally speaking, I agree with Robert's objection. The patch in itself adds only one unnecessary keyword, which probably wouldn't be noticeable, but the argument for it implies that we should be willing to add a lot more equally-unnecessary keywords, which I'm not. gram.o is already about 10% of the entire postgres executable, which probably goes far towards explaining why its inner loop always shows up high in profiling: cache misses are routine. And the size of those tables is at least linear in the number of keywords --- perhaps worse than linear, I'm not sure. Adding a bunch of keywords *will* cost us in performance. I'm not willing to pay that cost for something that adds neither features nor spec compliance. 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
Re: [Review] Re: [HACKERS] minor patch submission: CREATE CAST ... AS EXPLICIT
On 2013-06-24 09:55:22 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: What about simply not using a keyword at that location at all? Something like the attached hack? Hack is much too polite a word for that. This will for example fail to respect the difference between quoted and unquoted words. Well. If you you have a better name for some quick proof of concept patch... Anyway: The point of the patch is not to suggest the use 'name' for that - although we already do that in some places - but to prove that we can get away with sort of undeclared keywords in a bunch of places. I think doing so can reduce the number of keywords in a bunch of places. E.g. EXPLICIT wouldn't need to be one if we invented infrastructure for it. The scanner obviously cannot discern those from real keywords and literals, but we can easily do a recheck in code in the specific bison rules as long as we are sure the syntax is unambigous. Which it is a in a good part of the DDL support which in turn is a good sized part of the overall grammar. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[Review] Re: [HACKERS] 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; acronymSQL/acronym standard, except that SQL does not make provisions for binary-coercible types or extra arguments to implementation functions. - literalAS IMPLICIT/ is a productnamePostgreSQL/productname - extension, too. + literalAS IMPLICIT/ and literalAS EXPLICIT/ are + a productnamePostgreSQL/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 signature.asc Description: This is a digitally signed message part.
Re: [Review] Re: [HACKERS] minor patch submission: CREATE CAST ... AS EXPLICIT
On Sat, Jun 22, 2013 at 9:16 AM, Cédric Villemain ced...@2ndquadrant.com wrote: 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). I object to this patch. This patch a new keyword, EXPLICIT, for reasons that are strictly cosmetic. Everything that you can do with this patch can also be done without this patch. It is not a good idea to slow down parsing of every SQL statement we have just so that someone can write CREATE CAST .. AS EXPLICIT. Granted, the parsing slowdown for just one keyword is probably not noticeable, but it's cumulative with every new keyword we add. Adding them to support new features is one thing, but adding them to support purely optional syntax is, I think, going too far. -- 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
Re: [Review] Re: [HACKERS] minor patch submission: CREATE CAST ... AS EXPLICIT
Hello Robert, I object to this patch. This patch a new keyword, EXPLICIT, for reasons that are strictly cosmetic. Everything that you can do with this patch can also be done without this patch. It is not a good idea to slow down parsing of every SQL statement we have just so that someone can write CREATE CAST .. AS EXPLICIT. Granted, the parsing slowdown for just one keyword is probably not noticeable, but it's cumulative with every new keyword we add. Adding them to support new features is one thing, but adding them to support purely optional syntax is, I think, going too far. I partly object to the objection:-) I agree that it may induce a very small delay to the parser, however I *do* think that cosmetic things are important. In order to ease understanding, learning and memorizing a language, concepts must have names, syntaxes, and be orthogonal and symmetric where applicable. In this example, there are 3 kinds of casts, all 3 have a conceptual name (explicit, implicit, assignment) but only two have a syntax, and the other one is the absence of syntax. So you have to memorize this stupid information (which one of the three does not have a syntax) or read the documentation every time to remember that explicit is the one without a syntax. Note also that you must state implicit explicitely, but explicit is told implicitely, which does not really help. The impact is also on the documentation which is not symmetric because it is based on the syntax which is not, so it is simply a little harder to understand. Every year I do my class about PL/pgSQL and extensions to Pg, and every year some students will try as explicit because it is logical to do so. I think that she is right and that it should work, instead of having to explain that explicit is implicit when dealing with Pg casts. Although it is my job, I would prefer to spend time explaining more interesting things. From the software engineering point of view, having a syntax for all case means that the developer must think about which kind of cast she really wants, instead of doing the default thing just because it is the default. So in my mind the tradeoff is between people time annoyance and a few machine cycles, and I have no hesitation to choose the later. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [Review] Re: [HACKERS] minor patch submission: CREATE CAST ... AS EXPLICIT
Hello Cédric, So maybe it is possible to rephrase this piece: - literalAS IMPLICIT/ is a productnamePostgreSQL/productname - extension, too. + literalAS IMPLICIT/ and literalAS EXPLICIT/ are + a productnamePostgreSQL/productname extension, too. Ok. 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. Elegant is important:-) See my answer to Robert's objection. * Coding review Patch does not pass test: ./check_keywords.pl gram.y ../../../src/include/parser/kwlist.h Oops! That is not elegant! I had to update the unreserved keyword list in order to be able to build postgresql. ** Does it produce compiler warnings? don't build as is. Need patch update Indeed. I flag it 'return with feedback', please update the patch so it builds. Everything else is ok. Yep. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [Review] Re: [HACKERS] minor patch submission: CREATE CAST ... AS EXPLICIT
I flag it 'return with feedback', please update the patch so it builds.
Everything else is ok.
Here it is.
--
Fabien.diff --git a/doc/src/sgml/ref/create_cast.sgml b/doc/src/sgml/ref/create_cast.sgml
index 29ea298..0ace996 100644
--- a/doc/src/sgml/ref/create_cast.sgml
+++ b/doc/src/sgml/ref/create_cast.sgml
@@ -20,15 +20,15 @@
synopsis
CREATE CAST (replaceablesource_type/replaceable AS replaceabletarget_type/replaceable)
WITH FUNCTION replaceablefunction_name/replaceable (replaceableargument_type/replaceable [, ...])
-[ AS ASSIGNMENT | AS IMPLICIT ]
+[ AS ASSIGNMENT | AS EXPLICIT | AS IMPLICIT ]
CREATE CAST (replaceablesource_type/replaceable AS replaceabletarget_type/replaceable)
WITHOUT FUNCTION
-[ AS ASSIGNMENT | AS IMPLICIT ]
+[ AS ASSIGNMENT | AS EXPLICIT | AS IMPLICIT ]
CREATE CAST (replaceablesource_type/replaceable AS replaceabletarget_type/replaceable)
WITH INOUT
-[ AS ASSIGNMENT | AS IMPLICIT ]
+[ AS ASSIGNMENT | AS EXPLICIT | AS IMPLICIT ]
/synopsis
/refsynopsisdiv
@@ -74,7 +74,8 @@ SELECT CAST(42 AS float8);
/para
para
- By default, a cast can be invoked only by an explicit cast request,
+ By default, or if the cast is declared literalAS EXPLICIT/,
+ a cast can be invoked only by an explicit cast request,
that is an explicit literalCAST(replaceablex/ AS
replaceabletypename/)/literal or
replaceablex/literal::/replaceabletypename/
@@ -239,6 +240,21 @@ SELECT CAST ( 2 AS numeric ) + 4.0;
/varlistentry
varlistentry
+ termliteralAS EXPLICIT/literal/term
+
+ listitem
+ para
+ Indicates that the cast can be invoked only with an explicit
+ cast request, that is an explicit literalCAST(replaceablex/ AS
+ replaceabletypename/)/literal or
+ replaceablex/literal::/replaceabletypename/
+ construct.
+ This is the default.
+ /para
+ /listitem
+/varlistentry
+
+varlistentry
termliteralAS IMPLICIT/literal/term
listitem
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 5094226..2c0694f 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -533,7 +533,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
DICTIONARY DISABLE_P DISCARD DISTINCT DO DOCUMENT_P DOMAIN_P DOUBLE_P DROP
EACH ELSE ENABLE_P ENCODING ENCRYPTED END_P ENUM_P ESCAPE EVENT EXCEPT
- EXCLUDE EXCLUDING EXCLUSIVE EXECUTE EXISTS EXPLAIN
+ EXCLUDE EXCLUDING EXCLUSIVE EXECUTE EXISTS EXPLAIN EXPLICIT
EXTENSION EXTERNAL EXTRACT
FALSE_P FAMILY FETCH FIRST_P FLOAT_P FOLLOWING FOR FORCE FOREIGN FORWARD
@@ -6720,6 +6720,7 @@ CreateCastStmt: CREATE CAST '(' Typename AS Typename ')'
cast_context: AS IMPLICIT_P { $$ = COERCION_IMPLICIT; }
| AS ASSIGNMENT { $$ = COERCION_ASSIGNMENT; }
+ | AS EXPLICIT { $$ = COERCION_EXPLICIT; }
| /*EMPTY*/{ $$ = COERCION_EXPLICIT; }
;
@@ -12723,6 +12724,7 @@ unreserved_keyword:
| EXCLUSIVE
| EXECUTE
| EXPLAIN
+ | EXPLICIT
| EXTENSION
| EXTERNAL
| FAMILY
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index 68a13b7..f97389b 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -149,6 +149,7 @@ PG_KEYWORD(exclusive, EXCLUSIVE, UNRESERVED_KEYWORD)
PG_KEYWORD(execute, EXECUTE, UNRESERVED_KEYWORD)
PG_KEYWORD(exists, EXISTS, COL_NAME_KEYWORD)
PG_KEYWORD(explain, EXPLAIN, UNRESERVED_KEYWORD)
+PG_KEYWORD(explicit, EXPLICIT, UNRESERVED_KEYWORD)
PG_KEYWORD(extension, EXTENSION, UNRESERVED_KEYWORD)
PG_KEYWORD(external, EXTERNAL, UNRESERVED_KEYWORD)
PG_KEYWORD(extract, EXTRACT, COL_NAME_KEYWORD)
diff --git a/src/test/regress/expected/create_cast.out b/src/test/regress/expected/create_cast.out
index 56cd86e..a8858fa 100644
--- a/src/test/regress/expected/create_cast.out
+++ b/src/test/regress/expected/create_cast.out
@@ -27,8 +27,8 @@ ERROR: function casttestfunc(text) does not exist
LINE 1: SELECT casttestfunc('foo'::text);
^
HINT: No function matches the given name and argument types. You might need to add explicit type casts.
--- Try binary coercion cast
-CREATE CAST (text AS casttesttype) WITHOUT FUNCTION;
+-- Try binary coercion cast and verbose AS EXPLICIT
+CREATE CAST (text AS casttesttype) WITHOUT FUNCTION AS EXPLICIT;
SELECT casttestfunc('foo'::text); -- doesn't work, as the cast is explicit
ERROR: function casttestfunc(text) does not exist
LINE 1: SELECT casttestfunc('foo'::text);
@@ -54,7 +54,7 @@ SELECT 1234::int4::casttesttype; -- No cast yet, should fail
ERROR: cannot cast type integer to casttesttype
LINE 1: SELECT 1234::int4::casttesttype;
^
-CREATE CAST (int4 AS casttesttype) WITH INOUT;
+CREATE CAST (int4 AS casttesttype) WITH INOUT; -- default AS EXPLICIT
SELECT 1234::int4::casttesttype; -- Should work now
casttesttype
--
diff --git a/src/test/regress/sql/create_cast.sql
