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

>> - s/INFORMATION_SCHEMA.TABLES/SHOW CREATE TABLE/ in the revision
>> comment
>
> Fixed.
>
>> - I don’t think we should allow creation of non-TokuDB tables with
>> secondary clustering keys. Otherwise once the optimizer support is
>> reviewed and merged, we will get optimizer making incorrect
>> assumptions about secondary clustering keys for tables that don’t
>> actually implement it. On top of that, bad and hard-to-diagnose
>> things may happen on downgrade or migration to another server
>> flavor;
>
> I agree and this was how I implemented it the first time before
> Tokutek asked me to change to be treated as a hint. I didn't think of
> downgrade/migration issues then or I wouldn't have changed. (I don't
> see how the optimizer could make incorrect assumptions because no
> other storage engine would return HA_CLUSTERING in index_flags()).
>

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.

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.

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

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

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

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

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

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