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
