Re: [Review] Re: 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.

Fabien COELHO <coelho(at)cri(dot)ensmp(dot)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(at)gmx(dot)net> writes:
> On lr, 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(at)cri(dot)ensmp(dot)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(at)gmail(dot)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 apply
>
> Perhaps 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

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.

On Sun, Jun 16, 2013 at 12:25 PM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)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.

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(at)2ndquadrant(dot)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

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.

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.

> I flag it 'return with feedback', please update the patch so it builds.
> Everything else is ok.

Here it is.

--
Fabien.

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.

On 2013-06-22 15:10:07 -0400, Robert Haas wrote:
> On Sat, Jun 22, 2013 at 9:16 AM, Cédric Villemain
> <cedric(at)2ndquadrant(dot)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

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(at)2ndquadrant(dot)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

On 2013-06-24 09:55:22 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)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

On 06/24/2013 06:55 AM, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)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

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.

> >> 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

Josh Berkus <josh(at)agliodbs(dot)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

On 07/16/2013 03:12 PM, Tom Lane wrote:
> Josh Berkus <josh(at)agliodbs(dot)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