Code review comment for lp:~laurynas-biveinis/percona-server/tokudb-multiple-clust-keys

> The best way to handle this has been implemented in MariaDB via
> SE-specific options in CREATE/ALTER. I doubt we should port those
> extensions as a part of this work, just pointing out that the way we are
> implementing it will inevitably have unwanted side effects.

Fully agreed.

> Wrt optimizer changes: there’s code in the optimizer that takes flags
> from key definition into account (rather than just flags returned by the
> storage engine). For example, some code checks key definition flags and
> assumes it to be a regular key if none if the previous checks for
> specific flags return true. If you allow CLUSTERING keys for storage
> engine that don’t support them, you still get the HA_CLUSTERING key set
> in key definition flags.

OK, I see.

> > Added a handlerton flag HTON_SUPPORTS_CLUSTERED_KEYS. This is
> > different from e.g. HA_CAN_RTREE etc, which are handler flags, but the
> > property of supporting clustered keys seems to me to be global and not
> > of table level for a SE.
> >
> > For error message I reused ER_ILLEGAL_HA_CREATE_OPTION ("Table storage
> > engine 'InnoDB' does not support the create option 'CLUSTERING'")
> >
>
> That looks good in general. Please also add code that enforces this
> restriction to open_binary_frm(). I.e. it should fail with an error if a
> non-InnoDB table happens to contain both HA_SPATIAL and HA_FULLTEXT
> flags for an index.

Done. Should this happen, it should return ER_NOT_FORM_FILE.

> >> - I see there’s no support for UNIQUE CLUSTERING KEYs. Let’s fix it if
> >> it’s an oversight, or document if it’s a known limitation?
> >
> > An oversight on my part, not sure about Tokutek. Syntax extended as
>
> Are you going to report it to Tokutek?

Yes.

> > follows. Where previously "UNIQUE" was accepted as a valid modifier,
> > "CLUSTERING" is accepted too, as well as "UNIQUE CLUSTERING" and
> > "CLUSTERING UNIQUE":
> > - CREATE UNIQUE INDEX;
> > - as a part of key definition:
> > - CREATE TABLE (..., [CONSTRAINT ... ] UNIQUE KEY ...,)
> > - ALTER TABLE ADD [CONSTRAINT ...] UNIQUE KEY
> >
> > The way the grammar was changed, it accepts "UNIQUE UNIQUE" and
> > "CLUSTERING CLUSTERING" too. IMHO benign.
> >
>
> Hm, doesn’t look that benign to me. UNIQUE UNIQUE is invalid SQL which a
> user may want to be aware of. It would be aware before the change. It
> will not be aware after the change.

OK, fixed, added tests too.

> > Also now it is possible to create such "constraints" as CONSTRAINT c1
> > CLUSTERING KEY, which is not really a constraint at all, but again
> > IMHO benign and I don't see an easy way to forbid it without blowing
> > up shift/reduce conflict count in the parser.
> >
>
> I don’t see a point in supporting CONSTRAINT c1 CLUSTERING KEY.

I don't see a point neither, but "constraint" is hard to distinguish
from "key" in the parser grammar, so it was a side effect of adding
clustering support for the keys. I have tried two fixes. One was
based on syntax changes only, but I couldn't avoid bumping
shift/reduce conflict count. Thus I settled for a semantic action
hack. There are some differences in error reporting between the two
("-" is the syntactical fix, "+" the pushed semantical one):

 CREATE TABLE t1 (a INT PRIMARY KEY, b INT, CONSTRAINT c1 CLUSTERING KEY b (b)) ENGINE=InnoDB;
-ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'KEY b (b)) ENGINE=InnoDB' at line 1
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') ENGINE=InnoDB' at line 1

 ALTER TABLE t1 ADD CONSTRAINT c1 CLUSTERING INDEX b (b);
-ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'INDEX b (b)' at line 1
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1

Added tests too.

> > One place where the syntax was not extended was field attribute
> > syntax:
> > - CREATE TABLE (..., b INT UNIQUE CLUSTERING KEY, ...).
> > It appeared to require more changes to the parser, and can be done in
> > a follow-up if really needed.
> >
>
> What changes to the parser do you have in mind?

I had something complicated in mind, but now I have found an easier
way. The field attribute support is now implemented. But there are
some concerns:
- it caused extra two shift/reduce conflicts in the parser;
- CLUSTERING is now an allowed property of fields in addition to
  indexes, which required further work and at least in one place I
  don't see how to support a CLUSTERING | UNIQUE field properly:
  INFORMATION_SCHEMA.COLUMNS.COLUMN_KEY which is not well-suited for
  OR'ed flags.

> >> - any specific reasons to add another reserved keyword to the
> >> parser?
> >
> > I have reviewed the existing keywords and couldn't choose any IMHO
> > suitable replacement candidate. Also, I'm not sure Tokutek would
> > appreciate a forced mass search-replace in their testsuite that also
> > makes 5.5 to 5.6 upmerges less automatic.
>
> No-no, the question was about reasons for making CLUSTERING a reserved
> word. Each newly introduced reserved word means someone somewhere may
> need to rename schema and/or rewrite applications.
>
> This is also the first Percona-specific reserved word in the grammar. So
> there must be some good reasons for it?

I might be misunderstanding what a "reserved word" is (please
correct), but I'll assume this is about "a reserved keyword" as
opposed to " a non-reserved keyword".

Adding CLUSTERING_SYM to keyword_sp or keyword rule adds causes +4
shift/reduce and +2 reduce/reduce conflicts. I can attempt to analyze
the reduce/reduce conflicts at least, but after a first attempt I'm
not too certain in success. FWIW, UNIQUE_SYM, which comes in the
grammar in almost the same places CLUSTERING_SYM does, is a reserved
keyword too. And Oracle did not shy away from adding reserved keywords
in 5.5/5.6/5.7 neither.

Assuming I'm talking about the right thing, I see that we have at
least one extra reserved keyword against Oracle MySQL already:
"NOLOCK".