minor patch submission: CREATE CAST ... AS EXPLICIT
Hello,
Please find attached a minor stylish patch. It compiles and the update
test cases work for me.
Description:
Add "AS EXPLICIT" to "CREATE CAST"
This gives a name to the default case of "CREATE CAST", which creates a
cast which must be explicitely invoked.
From a language definition perspective, it is helpful to have a name for
every case instead of an implicit fallback, without any word to describe
it. See for instance "CREATE USER CREATEDB/NOCREATEDB" or "CREATE RULE ...
DO ALSO/INSTEAD" for similar occurences of naming default cases.
--
Fabien.
Attachments:
as_explicit.patchtext/x-diff; name=as_explicit.patchDownload
diff --git a/doc/src/sgml/ref/create_cast.sgml b/doc/src/sgml/ref/create_cast.sgml
index c0039ed..35893b7 100644
--- a/doc/src/sgml/ref/create_cast.sgml
+++ b/doc/src/sgml/ref/create_cast.sgml
@@ -20,15 +20,15 @@
<synopsis>
CREATE CAST (<replaceable>source_type</replaceable> AS <replaceable>target_type</replaceable>)
WITH FUNCTION <replaceable>function_name</replaceable> (<replaceable>argument_type</replaceable> [, ...])
- [ AS ASSIGNMENT | AS IMPLICIT ]
+ [ AS ASSIGNMENT | AS EXPLICIT | AS IMPLICIT ]
CREATE CAST (<replaceable>source_type</replaceable> AS <replaceable>target_type</replaceable>)
WITHOUT FUNCTION
- [ AS ASSIGNMENT | AS IMPLICIT ]
+ [ AS ASSIGNMENT | AS EXPLICIT | AS IMPLICIT ]
CREATE CAST (<replaceable>source_type</replaceable> AS <replaceable>target_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 <literal>AS EXPLICIT</>,
+ a cast can be invoked only by an explicit cast request,
that is an explicit <literal>CAST(<replaceable>x</> AS
<replaceable>typename</>)</literal> or
<replaceable>x</><literal>::</><replaceable>typename</>
@@ -238,6 +239,21 @@ SELECT CAST ( 2 AS numeric ) + 4.0;
</varlistentry>
<varlistentry>
+ <term><literal>AS EXPLICIT</literal></term>
+
+ <listitem>
+ <para>
+ Indicates that the cast can be invoked only with an explicit
+ cast request, that is an explicit <literal>CAST(<replaceable>x</> AS
+ <replaceable>typename</>)</literal> or
+ <replaceable>x</><literal>::</><replaceable>typename</>
+ construct.
+ This is the default.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><literal>AS IMPLICIT</literal></term>
<listitem>
@@ -405,8 +421,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>
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 1d39674..de339db 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -499,7 +499,7 @@ static void SplitColQualList(List *qualList,
DICTIONARY DISABLE_P DISCARD DISTINCT DO DOCUMENT_P DOMAIN_P DOUBLE_P DROP
EACH ELSE ENABLE_P ENCODING ENCRYPTED END_P ENUM_P ESCAPE 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
@@ -6313,6 +6313,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; }
;
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index 12c2faf..f5b2f16 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -148,6 +148,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 b/src/test/regress/sql/create_cast.sql
index ad348da..91123ca 100644
--- a/src/test/regress/sql/create_cast.sql
+++ b/src/test/regress/sql/create_cast.sql
@@ -27,8 +27,8 @@ $$ SELECT 1; $$;
SELECT casttestfunc('foo'::text); -- fails, as there's no cast
--- 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
SELECT casttestfunc('foo'::text::casttesttype); -- should work
DROP CAST (text AS casttesttype); -- cleanup
@@ -40,7 +40,7 @@ SELECT casttestfunc('foo'::text); -- Should work now
-- Try I/O conversion cast.
SELECT 1234::int4::casttesttype; -- No cast yet, should fail
-CREATE CAST (int4 AS casttesttype) WITH INOUT;
+CREATE CAST (int4 AS casttesttype) WITH INOUT; -- default AS EXPLICIT
SELECT 1234::int4::casttesttype; -- Should work now
DROP CAST (int4 AS casttesttype);
Fabien COELHO <coelho@cri.ensmp.fr> writes:
Description:
Add "AS EXPLICIT" to "CREATE CAST"
This gives a name to the default case of "CREATE CAST", which creates a
cast which must be explicitely invoked.
I'm not sure this is a good idea. The CREATE CAST syntax is in the SQL
standard, and this isn't it. Now I realize that we've extended that
statement already to cover some functionality that's not in the
standard, but that doesn't mean we should create unnecessarily
nonstandard syntax for cases that are in the standard. If a commercial
vendor did that, wouldn't you castigate them for trying to create vendor
lock-in?
From a language definition perspective, it is helpful to have a name for
every case instead of an implicit fallback, without any word to describe
it. See for instance "CREATE USER CREATEDB/NOCREATEDB" or "CREATE RULE ...
DO ALSO/INSTEAD" for similar occurences of naming default cases.
If we were working in a green field, I couldn't fault this logic ... but
we are not.
regards, tom lane
Hello Tom,
Add "AS EXPLICIT" to "CREATE CAST" This gives a name to the default
case of "CREATE CAST", which creates a cast which must be explicitely
invoked.I'm not sure this is a good idea. The CREATE CAST syntax is in the SQL
standard, and this isn't it. Now I realize that we've extended that
statement already to cover some functionality that's not in the
standard, but that doesn't mean we should create unnecessarily
nonstandard syntax for cases that are in the standard.
The standard provides only one case, so "CAST" is good enough a name.
Once you start creating alternatives with distinct semantics, then it
helps to give the initial one a name as well to be able to discuss them
with something else that "the remaining case", or "when there is no
option", especially as there is something to discuss.
Note that the standard is still supported just the same, and the
documentation already underlines that "AS *" stuff is a pg extension,
nothing is really changed. Maybe the documentation could be clearer about
where the standard stops and where extensions start, even now without an
"AS EXPLICIT" clause.
If a commercial vendor did that, wouldn't you castigate them for trying
to create vendor lock-in?
I'm more concerned with explaining things to students, and its good to
have words and logic for that.
With respect to the standard, it seems good enough to me if (1) the
standard is well supported and (2) the documentation clearly says which
parts are extensions. If you really want to keep to the standard, then do
not offer any extension.
Moreover, this stuff is really minor compared to RULEs or many other
things specifics to pg, and the "lock-in" is light, you just have to
remove "AS EXPLICIT" to get away, no big deal.
Well, you decide anyway:-)
Have a nice day,
--
Fabien.
On lör, 2011-05-21 at 15:46 +0200, Fabien COELHO wrote:
Hello,
Please find attached a minor stylish patch. It compiles and the update
test cases work for me.Description:
Add "AS EXPLICIT" to "CREATE CAST"
This gives a name to the default case of "CREATE CAST", which creates a
cast which must be explicitely invoked.From a language definition perspective, it is helpful to have a name for
every case instead of an implicit fallback, without any word to describe
it. See for instance "CREATE USER CREATEDB/NOCREATEDB" or "CREATE RULE ...
DO ALSO/INSTEAD" for similar occurences of naming default cases.
Oddly enough, we did add the DO ALSO syntax much later, and no one
complained about that, as far as I recall.
Peter Eisentraut <peter_e@gmx.net> writes:
On lör, 2011-05-21 at 15:46 +0200, Fabien COELHO wrote:
From a language definition perspective, it is helpful to have a name for
every case instead of an implicit fallback, without any word to describe
it. See for instance "CREATE USER CREATEDB/NOCREATEDB" or "CREATE RULE ...
DO ALSO/INSTEAD" for similar occurences of naming default cases.
Oddly enough, we did add the DO ALSO syntax much later, and no one
complained about that, as far as I recall.
Sure, but CREATE RULE is entirely locally-grown syntax, so there is no
argument from standards compliance to consider there.
regards, tom lane
From a language definition perspective, it is helpful to have a name for
every case instead of an implicit fallback, without any word to describe
it. See for instance "CREATE USER CREATEDB/NOCREATEDB" or "CREATE RULE ...
DO ALSO/INSTEAD" for similar occurences of naming default cases.Oddly enough, we did add the DO ALSO syntax much later, and no one
complained about that, as far as I recall.
I complained:-) and I submitted the patch then, AFAICR.
--
Fabien.
On 22 May 2011 07:27, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
Hello Tom,
Add "AS EXPLICIT" to "CREATE CAST" This gives a name to the default case
of "CREATE CAST", which creates a cast which must be explicitely invoked.I'm not sure this is a good idea. The CREATE CAST syntax is in the SQL
standard, and this isn't it. Now I realize that we've extended that
statement already to cover some functionality that's not in the
standard, but that doesn't mean we should create unnecessarily
nonstandard syntax for cases that are in the standard.The standard provides only one case, so "CAST" is good enough a name.
Once you start creating alternatives with distinct semantics, then it helps
to give the initial one a name as well to be able to discuss them with
something else that "the remaining case", or "when there is no option",
especially as there is something to discuss.Note that the standard is still supported just the same, and the
documentation already underlines that "AS *" stuff is a pg extension,
nothing is really changed. Maybe the documentation could be clearer about
where the standard stops and where extensions start, even now without an "AS
EXPLICIT" clause.If a commercial vendor did that, wouldn't you castigate them for trying to
create vendor lock-in?I'm more concerned with explaining things to students, and its good to have
words and logic for that.With respect to the standard, it seems good enough to me if (1) the standard
is well supported and (2) the documentation clearly says which parts are
extensions. If you really want to keep to the standard, then do not offer
any extension.Moreover, this stuff is really minor compared to RULEs or many other things
specifics to pg, and the "lock-in" is light, you just have to remove "AS
EXPLICIT" to get away, no big deal.
Hi Fabien,
I'm taking a look at this patch for the commitfest. On first reading
of the patch, it looked pretty sensible to me, but I had some trouble
applying it to HEAD:
error: patch failed: doc/src/sgml/ref/create_cast.sgml:20
error: doc/src/sgml/ref/create_cast.sgml: patch does not apply
error: patch failed: src/backend/parser/gram.y:499
error: src/backend/parser/gram.y: patch does not apply
error: patch failed: src/include/parser/kwlist.h:148
error: src/include/parser/kwlist.h: patch does not apply
error: patch failed: src/test/regress/expected/create_cast.out:27
error: src/test/regress/expected/create_cast.out: patch does not apply
error: patch failed: src/test/regress/sql/create_cast.sql:27
error: src/test/regress/sql/create_cast.sql: patch does not apply
Perhaps the patch could use a refresh?
Also, for what it's worth, I buy into the argument for adding AS
EXPLICIT. This stuff is all an extension to the SQL standard already;
it might as well have a well-rounded syntax.
Cheers,
BJ
On 18 June 2011 09:49, Brendan Jurd <direvus@gmail.com> wrote:
Hi Fabien,
I'm taking a look at this patch for the commitfest. On first reading
of the patch, it looked pretty sensible to me, but I had some trouble
applying it to HEAD:error: patch failed: doc/src/sgml/ref/create_cast.sgml:20
error: doc/src/sgml/ref/create_cast.sgml: patch does not apply
error: patch failed: src/backend/parser/gram.y:499
error: src/backend/parser/gram.y: patch does not apply
error: patch failed: src/include/parser/kwlist.h:148
error: src/include/parser/kwlist.h: patch does not apply
error: patch failed: src/test/regress/expected/create_cast.out:27
error: src/test/regress/expected/create_cast.out: patch does not apply
error: patch failed: src/test/regress/sql/create_cast.sql:27
error: src/test/regress/sql/create_cast.sql: patch does not applyPerhaps the patch could use a refresh?
The author has yet to reply to the above -- we are still lacking a
patch version that applies cleanly to HEAD. I have marked this patch
'Waiting on Author'.
Cheers,
BJ
Hi,
I saw you added this 2-year old thread to the 2013-06 commitfest, but I
don't see any new activity. Huh?
On 28.05.2011 00:48, Fabien COELHO wrote:
From a language definition perspective, it is helpful to have a name for
every case instead of an implicit fallback, without any word to describe
it. See for instance "CREATE USER CREATEDB/NOCREATEDB" or "CREATE
RULE ...
DO ALSO/INSTEAD" for similar occurences of naming default cases.Oddly enough, we did add the DO ALSO syntax much later, and no one
complained about that, as far as I recall.I complained:-) and I submitted the patch then, AFAICR.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hello,
I saw you added this 2-year old thread to the 2013-06 commitfest, but I don't
see any new activity. Huh?
What activity would you expect? I sent the patch 2 years ago on the list,
and now that I figured out that there is a "submitted patch list" open for
consideration I added the corresponding link so that it may come out of
oblivion.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Jun 16, 2013 at 12:25 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
Hello,
I saw you added this 2-year old thread to the 2013-06 commitfest, but I
don't see any new activity. Huh?
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.
Cheers,
Jeff
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.
--
Fabien.
Attachments:
as_explicit_v2.patchtext/x-diff; name=as_explicit_v2.patchDownload
diff --git a/doc/src/sgml/ref/create_cast.sgml b/doc/src/sgml/ref/create_cast.sgml
index 29ea298..4430810 100644
--- a/doc/src/sgml/ref/create_cast.sgml
+++ b/doc/src/sgml/ref/create_cast.sgml
@@ -20,15 +20,15 @@
<synopsis>
CREATE CAST (<replaceable>source_type</replaceable> AS <replaceable>target_type</replaceable>)
WITH FUNCTION <replaceable>function_name</replaceable> (<replaceable>argument_type</replaceable> [, ...])
- [ AS ASSIGNMENT | AS IMPLICIT ]
+ [ AS ASSIGNMENT | AS EXPLICIT | AS IMPLICIT ]
CREATE CAST (<replaceable>source_type</replaceable> AS <replaceable>target_type</replaceable>)
WITHOUT FUNCTION
- [ AS ASSIGNMENT | AS IMPLICIT ]
+ [ AS ASSIGNMENT | AS EXPLICIT | AS IMPLICIT ]
CREATE CAST (<replaceable>source_type</replaceable> AS <replaceable>target_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 <literal>AS EXPLICIT</>,
+ a cast can be invoked only by an explicit cast request,
that is an explicit <literal>CAST(<replaceable>x</> AS
<replaceable>typename</>)</literal> or
<replaceable>x</><literal>::</><replaceable>typename</>
@@ -239,6 +240,21 @@ SELECT CAST ( 2 AS numeric ) + 4.0;
</varlistentry>
<varlistentry>
+ <term><literal>AS EXPLICIT</literal></term>
+
+ <listitem>
+ <para>
+ Indicates that the cast can be invoked only with an explicit
+ cast request, that is an explicit <literal>CAST(<replaceable>x</> AS
+ <replaceable>typename</>)</literal> or
+ <replaceable>x</><literal>::</><replaceable>typename</>
+ construct.
+ This is the default.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><literal>AS IMPLICIT</literal></term>
<listitem>
@@ -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>
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 5094226..96cfb40 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; }
;
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 b/src/test/regress/sql/create_cast.sql
index ad348da..91123ca 100644
--- a/src/test/regress/sql/create_cast.sql
+++ b/src/test/regress/sql/create_cast.sql
@@ -27,8 +27,8 @@ $$ SELECT 1; $$;
SELECT casttestfunc('foo'::text); -- fails, as there's no cast
--- 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
SELECT casttestfunc('foo'::text::casttesttype); -- should work
DROP CAST (text AS casttesttype); -- cleanup
@@ -40,7 +40,7 @@ SELECT casttestfunc('foo'::text); -- Should work now
-- Try I/O conversion cast.
SELECT 1234::int4::casttesttype; -- No cast yet, should fail
-CREATE CAST (int4 AS casttesttype) WITH INOUT;
+CREATE CAST (int4 AS casttesttype) WITH INOUT; -- default AS EXPLICIT
SELECT 1234::int4::casttesttype; -- Should work now
DROP CAST (int4 AS casttesttype);
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
On Sat, Jun 22, 2013 at 9:16 AM, Cédric Villemain
<cedric@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
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
Hello Cï¿œdric,
So maybe it is possible to rephrase this piece: - <literal>AS IMPLICIT</> is a <productname>PostgreSQL</productname> - extension, too. + <literal>AS IMPLICIT</> and <literal>AS EXPLICIT</> are + a <productname>PostgreSQL</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
I flag it 'return with feedback', please update the patch so it builds.
Everything else is ok.
Here it is.
--
Fabien.
Attachments:
as_explicit-v2.patchtext/x-diff; name=as_explicit-v2.patchDownload
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 (<replaceable>source_type</replaceable> AS <replaceable>target_type</replaceable>)
WITH FUNCTION <replaceable>function_name</replaceable> (<replaceable>argument_type</replaceable> [, ...])
- [ AS ASSIGNMENT | AS IMPLICIT ]
+ [ AS ASSIGNMENT | AS EXPLICIT | AS IMPLICIT ]
CREATE CAST (<replaceable>source_type</replaceable> AS <replaceable>target_type</replaceable>)
WITHOUT FUNCTION
- [ AS ASSIGNMENT | AS IMPLICIT ]
+ [ AS ASSIGNMENT | AS EXPLICIT | AS IMPLICIT ]
CREATE CAST (<replaceable>source_type</replaceable> AS <replaceable>target_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 <literal>AS EXPLICIT</>,
+ a cast can be invoked only by an explicit cast request,
that is an explicit <literal>CAST(<replaceable>x</> AS
<replaceable>typename</>)</literal> or
<replaceable>x</><literal>::</><replaceable>typename</>
@@ -239,6 +240,21 @@ SELECT CAST ( 2 AS numeric ) + 4.0;
</varlistentry>
<varlistentry>
+ <term><literal>AS EXPLICIT</literal></term>
+
+ <listitem>
+ <para>
+ Indicates that the cast can be invoked only with an explicit
+ cast request, that is an explicit <literal>CAST(<replaceable>x</> AS
+ <replaceable>typename</>)</literal> or
+ <replaceable>x</><literal>::</><replaceable>typename</>
+ construct.
+ This is the default.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><literal>AS 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 b/src/test/regress/sql/create_cast.sql
index ad348da..91123ca 100644
--- a/src/test/regress/sql/create_cast.sql
+++ b/src/test/regress/sql/create_cast.sql
@@ -27,8 +27,8 @@ $$ SELECT 1; $$;
SELECT casttestfunc('foo'::text); -- fails, as there's no cast
--- 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
SELECT casttestfunc('foo'::text::casttesttype); -- should work
DROP CAST (text AS casttesttype); -- cleanup
@@ -40,7 +40,7 @@ SELECT casttestfunc('foo'::text); -- Should work now
-- Try I/O conversion cast.
SELECT 1234::int4::casttesttype; -- No cast yet, should fail
-CREATE CAST (int4 AS casttesttype) WITH INOUT;
+CREATE CAST (int4 AS casttesttype) WITH INOUT; -- default AS EXPLICIT
SELECT 1234::int4::casttesttype; -- Should work now
DROP CAST (int4 AS casttesttype);
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
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.
Attachments:
as_explicit-v2.patchtext/x-diff; name=as_explicit-v2.patchDownload
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 (<replaceable>source_type</replaceable> AS <replaceable>target_type</replaceable>)
WITH FUNCTION <replaceable>function_name</replaceable> (<replaceable>argument_type</replaceable> [, ...])
- [ AS ASSIGNMENT | AS IMPLICIT ]
+ [ AS ASSIGNMENT | AS EXPLICIT | AS IMPLICIT ]
CREATE CAST (<replaceable>source_type</replaceable> AS <replaceable>target_type</replaceable>)
WITHOUT FUNCTION
- [ AS ASSIGNMENT | AS IMPLICIT ]
+ [ AS ASSIGNMENT | AS EXPLICIT | AS IMPLICIT ]
CREATE CAST (<replaceable>source_type</replaceable> AS <replaceable>target_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 <literal>AS EXPLICIT</>,
+ a cast can be invoked only by an explicit cast request,
that is an explicit <literal>CAST(<replaceable>x</> AS
<replaceable>typename</>)</literal> or
<replaceable>x</><literal>::</><replaceable>typename</>
@@ -239,6 +240,21 @@ SELECT CAST ( 2 AS numeric ) + 4.0;
</varlistentry>
<varlistentry>
+ <term><literal>AS EXPLICIT</literal></term>
+
+ <listitem>
+ <para>
+ Indicates that the cast can be invoked only with an explicit
+ cast request, that is an explicit <literal>CAST(<replaceable>x</> AS
+ <replaceable>typename</>)</literal> or
+ <replaceable>x</><literal>::</><replaceable>typename</>
+ construct.
+ This is the default.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><literal>AS 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 b/src/test/regress/sql/create_cast.sql
index ad348da..91123ca 100644
--- a/src/test/regress/sql/create_cast.sql
+++ b/src/test/regress/sql/create_cast.sql
@@ -27,8 +27,8 @@ $$ SELECT 1; $$;
SELECT casttestfunc('foo'::text); -- fails, as there's no cast
--- 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
SELECT casttestfunc('foo'::text::casttesttype); -- should work
DROP CAST (text AS casttesttype); -- cleanup
@@ -40,7 +40,7 @@ SELECT casttestfunc('foo'::text); -- Should work now
-- Try I/O conversion cast.
SELECT 1234::int4::casttesttype; -- No cast yet, should fail
-CREATE CAST (int4 AS casttesttype) WITH INOUT;
+CREATE CAST (int4 AS casttesttype) WITH INOUT; -- default AS EXPLICIT
SELECT 1234::int4::casttesttype; -- Should work now
DROP CAST (int4 AS casttesttype);
On 2013-06-22 15:10:07 -0400, Robert Haas wrote:
On Sat, Jun 22, 2013 at 9:16 AM, Cédric Villemain
<cedric@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
Attachments:
cast-explicit-hack.patchtext/x-patch; charset=us-asciiDownload
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; }
;
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
Andres Freund <andres@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
On 2013-06-24 09:55:22 -0400, Tom Lane wrote:
Andres Freund <andres@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
On 06/24/2013 06:55 AM, Tom Lane wrote:
Andres Freund <andres@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
Import Notes
Reply to msg id not found: WMfb72648cc3b61486d6bb95567d94b546419fa18c79b832742cbb5ff0a4b864ec2e24ab3846f8de7e3df3721b55a56126@asav-3.01.com
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
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
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
Import Notes
Reply to msg id not found: WM3d3c6130756f2b6ddf39a159347a9b2beb4a402b86ded5cf401b673c08ab0ffae6d5694d5ff6f5589285920760690855@asav-3.01.com
Josh Berkus <josh@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
On 07/16/2013 03:12 PM, Tom Lane wrote:
Josh Berkus <josh@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
Import Notes
Reply to msg id not found: WMb4aeda986ba0a53d12877bc6a86818954a989b68e2341a2e3671c8624e1c524b87819e38e3fc8d2903b6213a17270237@asav-1.01.com