Skip to content

PS-11264: Vector index support in Data Dictionary - #6000

Open
percona-mhansson wants to merge 4 commits into
percona:vector-mvpfrom
percona-mhansson:vector-mvp-dd
Open

PS-11264: Vector index support in Data Dictionary#6000
percona-mhansson wants to merge 4 commits into
percona:vector-mvpfrom
percona-mhansson:vector-mvp-dd

Conversation

@percona-mhansson

@percona-mhansson percona-mhansson commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Vector indexes have the type (algorithm) SE_SPECIFIC, and we add a column option in the data dictionary saying vector_index=1; which gets picked up by dedicated code in the data dictionary and the handler part of InnoDB.

In the SQL layer, the vector index is very much a thing; there is an HA_KEY_ALG_VECTOR, an HA_VECTOR and a KEYTYPE_VECTOR.

Extra SQL is added to display the type of a vector index as VECTOR rather than SE_SPECIFIC.

I you try to open a table containing a vector index in a trunk Percona server, you get a failed assertion in InnoDB when a client connects. However, if you drop the index, you can connect just fine.

@percona-mhansson
percona-mhansson changed the base branch from trunk to vector-mvp June 9, 2026 14:25
@percona-mhansson
percona-mhansson force-pushed the vector-mvp-dd branch 2 times, most recently from af4cd72 to fee791a Compare June 10, 2026 14:24
@percona-mhansson
percona-mhansson force-pushed the vector-mvp-dd branch 2 times, most recently from cd4a559 to 82b0023 Compare July 22, 2026 09:57
@satya-bodapati
satya-bodapati requested a review from Copilot July 22, 2026 10:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds end-to-end server and InnoDB/DD plumbing to recognize “vector indexes” as a first-class SQL concept while storing them as SE_SPECIFIC in the DD, plus adjusts SHOW/INFORMATION_SCHEMA output to display VECTOR.

Changes:

  • Introduces new SQL-layer key/index types for vector indexes (KEYTYPE_VECTOR, HA_VECTOR, HA_KEY_ALG_VECTOR) and a handler capability flag (HA_CAN_VECTOR).
  • Propagates vector-index identification into InnoDB dictionary/index structures and skips B-tree specific logic where vector indexes don’t have trees (similar to FTS handling).
  • Updates SHOW/IS output paths to surface vector index type as VECTOR instead of SE_SPECIFIC.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
storage/temptable/src/table.cc Treats vector index algorithm as unsupported in temptable index creation (abort path).
storage/temptable/src/handler.cc Ensures vector index algorithm reports no index flags in temptable handler.
storage/innobase/include/dict0mem.ic Initializes new dict_index_t vector marker during struct fill.
storage/innobase/include/dict0mem.h Adds is_vector_index and helper accessor; adjusts index-type bit width constant.
storage/innobase/include/dict0dict.ic Adds dict_index_is_vector() helper.
storage/innobase/handler/ha_innodb.cc Sets vector capability; propagates vector flag into InnoDB index objects; treats vector like FTS/spatial in several code paths.
storage/innobase/dict/dict0dict.cc Adds vector “internal index build” path and excludes vector indexes from some operations.
storage/innobase/dict/dict0dd.cc Excludes vector indexes from length/prefix logic and sets vector marker when building dict indexes from TABLE metadata.
storage/innobase/dict/dict0crea.cc Skips index-tree creation for vector indexes (like FTS).
storage/innobase/btr/btr0btr.cc Skips B-tree validation for vector indexes (like FTS/online DDL).
sql/sql_table.cc Adds KEYTYPE_VECTOR handling, vector index constraints, algorithm mapping, and related DDL validation.
sql/sql_show.cc Displays “VECTOR KEY” and reports INDEX_TYPE=VECTOR in SHOW INDEX output.
sql/key_spec.h Adds KEYTYPE_VECTOR.
sql/handler.h Adds HA_CAN_VECTOR handler capability flag.
sql/field.cc Allows VECTOR type to participate in key-part eligibility checks.
sql/dd/info_schema/show.cc Attempts to map SE_SPECIFIC to VECTOR for IS/SHOW KEYS query output.
sql/dd/impl/types/column_impl.cc Allows vector_index as a valid DD column option key.
sql/dd/dd_table.cc Persists vector_index column option and maps vector algorithm/type into DD fields.
sql/dd_table_share.cc Detects vector indexes when filling TABLE_SHARE key metadata from DD.
sql/create_field.cc Allows VECTOR type through key-length calculation paths (removes prior assert).
share/messages_to_clients.txt Adds client-visible error messages for vector-index constraints.
include/my_base.h Adds HA_KEY_ALG_VECTOR and HA_VECTOR key flag.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread storage/innobase/include/dict0mem.ic
Comment thread storage/innobase/include/dict0mem.h
Comment thread sql/sql_table.cc
Comment thread sql/sql_table.cc
Comment thread sql/sql_table.cc
Comment thread sql/sql_table.cc
Comment thread sql/dd/info_schema/show.cc Outdated
Comment on lines +883 to +893
Item *sub_part_item =
new (thd->mem_root) Item_field(pos, NullS, NullS, alias_sub_part.str);
if (sub_part_item == nullptr) return nullptr;

Item *one_item = new (thd->mem_root)
Item_string(STRING_WITH_LEN("1"), system_charset_info);
if (one_item == nullptr) return nullptr;

Item *is_single_sub_part =
new (thd->mem_root) Item_func_eq(pos, sub_part_item, one_item);
if (is_single_sub_part == nullptr) return nullptr;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@percona-mhansson did you check this comment, is this true?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, and this is the documented design: https://app.notion.com/p/percona/Vector-Search-MVP-Design-36c674d091f3803ea7c1c0c66526ac80?source=copy_link#37a674d091f380f6b4daeb8183fffe98

So let's change the design, then 🙂

We already have vector_index_type=hnsw in the options field. We can just check if the type is one of the known vector types. For the MVP there is only HNSW, so let's take a shortcut and just add the condition "is it equal to 'hnsw'", shall we?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, actually found a bug, but it requires RocksDB to trigger

mysql> CREATE TABLE t (a CHAR(2), KEY(a(1))) ENGINE=ROCKSDB;
Query OK, 0 rows affected (0.018 sec)

mysql> show keys from t\G
*************************** 1. row ***************************
        Table: t
   Non_unique: 1
     Key_name: a
 Seq_in_index: 1
  Column_name: a
    Collation: A
  Cardinality: NULL
     Sub_part: 1
       Packed: NULL
         Null: YES
   Index_type: VECTOR
      Comment: 
Index_comment: 
      Visible: YES
   Expression: NULL
1 row in set (0.010 sec)

Comment on lines 113 to 117
constexpr uint32_t DICT_MULTI_VALUE = 512;

/** number of bits used for SYS_INDEXES.TYPE */
constexpr uint32_t DICT_IT_BITS = 10;
constexpr uint32_t DICT_IT_BITS = 11;
/** @} */
Comment thread sql/handler.h
Comment thread sql/sql_table.cc

@satya-bodapati satya-bodapati left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[AI-review] Review of the DD/metadata layer. Context: this commit is the base of the ps-11300-hnsw-populate work (30 commits downstream), so several items below were found the hard way there. 7 inline suggestions attached; three items that can't be anchored to diff lines:

1. ha_innobase::index_flags() must return 0 for vector keys (function not touched by this PR). The stub index has no B-tree, but index_flags() still advertises HA_READ_*/HA_KEYREAD_ONLY, so find_shortest_key picks it for COUNT(*) / index-only scans — which return 0 rows. Fix verified downstream (mirror of the FULLTEXT early-out at the top of the function):

  /* The vector index is a stub at the B-tree level: it can never serve
  ordered reads, ranges or index-only scans. Without this,
  find_shortest_key picks it for COUNT(*) and returns no rows. */
  if (table_share->key_info[key].algorithm == HA_KEY_ALG_VECTOR) {
    return (0);
  }

Side effect: SHOW INDEXES shows Collation NULL for vector keys — semantically right.

2. vector_index=1 is set on every VECTOR column, indexed or not. fill_dd_columns_from_create_fields() stamps the option on any MYSQL_TYPE_VECTOR column, so a table with an unindexed vector column also gets Percona-specific SDI, and dd_is_vector_index() then classifies any single-column SE_SPECIFIC index on such a column as a vector index. Suggest making the marker an option on the dd::Index (where the WITH(...) params will live anyway), or setting the column option only when the column is under a KEYTYPE_VECTOR key.

3. Nits. (a) The SHOW KEYS rewrite (type='SE_SPECIFIC' AND Sub_part='1'VECTOR, string compare) is a heuristic — fine while nothing else emits SE_SPECIFIC, but deserves a comment saying so. (b) The handler.h ha_fast_update/upsert and field.cc include-comment hunks are unrelated reformatting — consider dropping to keep the diff reviewable.

Comment thread sql/sql_table.cc
Comment thread share/messages_to_clients.txt
Comment thread sql/sql_table.cc Outdated
Comment thread sql/sql_table.cc Outdated
} else if (key->type == KEYTYPE_VECTOR) {
// VECTOR indexes are only allowed on VECTOR columns.
if (sql_field->sql_type != MYSQL_TYPE_VECTOR) {
my_error(ER_UNKNOWN_ERROR, MYF(0));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[AI-review] Placeholder — today VECTOR KEY (int_col) fails with “ERROR HY000: Unknown error”:

Suggested change
my_error(ER_UNKNOWN_ERROR, MYF(0));
my_error(ER_VECTOR_INDEX_REQUIRES_VECTOR_COLUMN, MYF(0));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@percona-mhansson I still see my_error(ER_UNKNOWN_ERROR, MYF(0));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are you looking? This change isn't even part of this PR. Perhaps it was in the past?

Comment thread sql/sql_table.cc Outdated
Comment thread storage/innobase/include/dict0mem.h
Comment thread storage/innobase/include/dict0mem.h Outdated
@satya-bodapati

Copy link
Copy Markdown
Contributor

[AI-review] Suggested PR-description sections — @percona-mhansson feel free to paste/edit:

Summary of changes

Makes the vector index a first-class index kind in the SQL layer and a recognized stub in InnoDB — metadata only. No storage, population, or search; those come in follow-up PRs. Pattern throughout: treat a vector index like an FTS index (no B-tree, excluded from stats/validation/FK use).

High-level design

  • SQL layer: new HA_KEY_ALG_VECTOR, key flag HA_VECTOR (1<<30), KEYTYPE_VECTOR. Engines opt in via table flag HA_CAN_VECTOR (InnoDB only).
  • DD: no schema/SDI change. The index is persisted with existing enums (IA_SE_SPECIFIC + IT_MULTIPLE); a vector_index=1 column option is the marker. Identity is re-derived at open (dd_table_share.cc, dd_is_vector_index() in InnoDB).
  • InnoDB: dict_index_t::is_vector_index + dict_index_is_vector(); vector indexes get no index tree (dict_create_index_tree_in_mem), are skipped by btr validation, row-size checks, stats/cardinality (fixed 1.0), FK parent/supporting key lookup; index_read() refuses.
  • Downgrade: DD is readable by upstream, but upstream InnoDB asserts at open on an SE_SPECIFIC index — drop the vector index before downgrade.

DD changes

Item Change
SDI / DD schema none
dd::Index reuses algorithm=SE_SPECIFIC, type=MULTIPLE
dd::Column.options new whitelisted key vector_index (the marker)
Open path marker + shape (single visible element) => HA_VECTOR key

User interface changes

  • CREATE TABLE ... VECTOR KEY (col) accepted (InnoDB only).
  • SHOW CREATE TABLE prints VECTOR KEY; no prefix-length suffix.
  • SHOW INDEX / I_S.STATISTICS: Index_type = VECTOR.
  • New errors: ER_VECTOR_INDEX_NEEDS_PK, ER_ONLY_SINGLE_VECTOR_INDEX_ALLOWED.
  • Disallowed: >1 key part; >1 vector index per table; missing or non-BIGINT UNSIGNED single-column PK; engines without HA_CAN_VECTOR; vector index as FK parent/supporting key; explicit USING algorithm.

@percona-mhansson
percona-mhansson force-pushed the vector-mvp-dd branch 2 times, most recently from 017dc49 to 97fb978 Compare July 28, 2026 10:07
@satya-bodapati
satya-bodapati requested a review from dlenev August 10, 2026 14:08
Comment thread sql/dd/dd_table.cc
return dd::Index::IA_FULLTEXT;

case HA_KEY_ALG_VECTOR:
return dd::Index::IA_SE_SPECIFIC;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it simplify if we introduce dd::Index::IA_VECTOR ? To tell if an index is a vector or not, we are iterating all elements of the vector and returning true if at least one element is a vector type.

I see that we already have an in-memory flag HA_VECTOR. Maybe it simplifies if we introduce dd::Index::IT_VECTOR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would indeed be simpler. The problem is that these correspond to a table definition that we are not allowed to change. We are free to do what we want in-memory, but on-disk metadata cannot be altered.

https://app.notion.com/p/percona/Vector-Search-MVP-Design-36c674d091f3803ea7c1c0c66526ac80?source=copy_link#37a674d091f380298786eef49a0f76e5

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, MySQL lacks an extensible (DD) framework for adding new types of columns or indexes. Adding a new type in downstream means that we conflict with upstream uses the next bit/flag 😞

@satya-bodapati satya-bodapati Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One doubt, if user drops all vecotr indexes/columns etc, would this flag (dd::Index::IT_VECTOR) in DD still cause upstream mysqld to not use our datadir? :thinking:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not only that. @dlenev can probably fill in the gaps here, but from what I understand, we can't change the DD table definitions (in SQL).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, adding new enum elements in ENUM definitions in DD Indexes table will break compatibility with Upstream, which we should avoid. And yes, this won't be change which can be easily fixed by dropping all vector indexes in the system, as not the contents, but definition of DD table is changed in this case.

In theory we can introduce IA_VECTOR and IT_VECTOR on DD API/Index object level and map it to the same IA_SE_SPECIFIC and IT_MULTIPLE in DD tables underneath the API. But IMO this won't simplify things and can be more confusing actually (IMO it is nice when DD API concepts and contents of DD tables match!).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having said above I think the code can be improved by adding comment here explaining why we map VECTOR algorithm to IA_SE_SPECIFIC. Basically, explain that we don't want to break binary compatibility of DD tables definitions with Upstream by introducing new elements in index algorithm and types enum in DD indexes table.

Comment thread sql/sql_table.cc
Comment thread storage/innobase/dict/dict0dict.cc Outdated
/* FTS index consists of auxiliary tables, they shall be excluded from index
row size check */
if (new_index->type & DICT_FTS) {
if ((new_index->type & DICT_FTS) || dict_index_is_vector(new_index)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be simpler if we can rely on DICT_VECTOR as additional flag. This is in-memory only. ignore the SYS_INDEXES and the bit restrictions (they are from old 5.7 era)

@percona-mhansson percona-mhansson Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I shouldn't pay attention to everything Copilot says... 😄

#6000 (comment)

Done. I still need to add the bit, or an assertion fails.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah,in this case, it is pessimistic. 😄 It didn't fully check that those bits don't make it to disk.
Good to bump the upper limit for the number of bits

}

DEBUG_SYNC(thd, "innodb.after_init_check");
if (index->type & (DICT_FTS | DICT_SPATIAL)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking if you have verified all occurrences of DICT_FTS and added an exemption for vector index?

Maybe it's worth doing all InnoDB-related changes as a separate commit? And do we need InnoDB engine changes in this PR? 🤔

@percona-mhansson percona-mhansson Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need metadata support in order to implement the syntax, unless we want the syntax to be just untestable stubs. I thought most of the metadata would go into the DD, but I needed to pull some extra tricks to put it all together. I have also designed it so that part of the syntax checking is inside the innodb handler, so that part is needed, unless we change the design. (Fwiw, Upstream are thinking about this too. mysql/mysql-server#702. See Parsing and validation model)

That said, we can of course break the innodb changes out into a PR of its own - it can't be tested as it sits anyway.

Just tell me how you want to proceed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless AI can do it cleanly, I am OK with combined (innodb &non-innodb) patch is.

if ((key->flags & HA_FULLTEXT) || (key->flags & HA_SPATIAL)) {
if ((key->flags & HA_FULLTEXT) || (key->flags & HA_SPATIAL) ||
(key->flags & HA_VECTOR)) {
/* The record per key does not apply to FTS or Spatial indexes. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the other comment, do we need to scan the entire InnoDB code that uses key->flags handling for HA_FULLTEXT and add a similar HA_VECTOR check everywhere?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, most likely. And I'd like to add tests to cover all those places.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've started investigating now. Some are applicable, others are peculiarities of full-text indexes, like the conditions when you can alter them. But in general we can weed out a lot of bugs by examining those places

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the tricky part 😄 do we want to take a decision now on such usages? or wait until the problematic scenario to hit. How many places do we need to take action? 🤔 based on that we can think

@percona-mhansson

percona-mhansson commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Comment thread sql/sql_table.cc
Vector indexes have the type (algorithm) SE_SPECIFIC, and we add a column option
in the data dictionary saying `vector_index=1;` which gets picked up by
dedicated code in the data dictionary and the handler part of InnoDB.

In the SQL layer, the vector index is very much a thing; there is an
`HA_KEY_ALG_VECTOR`, an `HA_VECTOR` and a `KEYTYPE_VECTOR`.

Extra SQL is added to display the type of a vector index as VECTOR rather than
SE_SPECIFIC.

@dlenev dlenev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Martin!

Here is the first batch of my review comments for your patch.
More comments to follow.

Comment thread include/my_base.h
/** Key was automatically created to support Foreign Key constraint. */
#define HA_GENERATED_KEY (1 << 13)
/** Vector key (Percona). */
#define HA_VECTOR (1 << 30)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... Why this define has been placed at this position in the list?
Would not it be better/logical/simpler to do merges if it is put it after HA_MULTI_VALUED_KEY instead?
(HA_KEYFLAG_MASK is a macro so AFAIU HA_VECTOR will be resolved at the point where HA_KEYFLAG_MAS is used, and not defined!).
I would also added a comment explaining choice of constant (1 << 30 vs say 1 << 20 which is HA_MULTI_VALUED_KEY << 1).

Table Non_unique Key_name Seq_in_index Column_name Collation Cardinality Sub_part Packed Null Index_type Comment Index_comment Visible Expression
t1 0 PRIMARY 1 id A 0 NULL NULL BTREE YES NULL
t1 1 v1 1 v1 A 0 1 NULL VECTOR YES NULL
SELECT * FROM INFORMATION_SCHEMA.INNODB_INDEXES;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure the full output of this statement is stable enough and won't break if, for example, e.g. new table is added to data-dictionary or some new component installed by default?

I think it is better to exclude rows unrelated to other tables/indexes and possibly mask fragile things like index and table ids...

@@ -0,0 +1,8 @@
CREATE TABLE t1 (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I don't understand why this test is necessary...
It doesn't mention VECTOR or HNSW at all...
So perhaps it is better to exclude it?

# Start of Percona Server 8.4/9.7 error messages to be sent to client
#

ER_VECTOR_INDEX_NEEDS_PK

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I am not sure that it is worth to have ER_VECTOR_INDEX_NEEDS_PK and ER_ONLY_SINGLE_VECTOR_INDEX_ALLOWED... While I agree it is a good practice to
have specific and actionable error messages, these sound like messages for temporary
limitations which are likely to be relaxed soon.

The problem is that once we publish code with new error code/error message it is not easy to retract it,
even once they become unused.

So perhaps it is better to use ER_NOT_SUPPORTED_YET or ER_FEATURE_UNSUPPORTED
meanwhile?

@satya-bodapati @percona-mhansson what do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we can print custom string along with it, using ER_NOT_SUPPORTED_YET is ok.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we can.
Message for ER_NOT_SUPPORTED_YET looks like "This version of MySQL doesn't yet support '%s'", so we use string explaining what is not supported with it. And we can always construct custom error message for any error code (though this is not very user/search-friendly).

Table Non_unique Key_name Seq_in_index Column_name Collation Cardinality Sub_part Packed Null Index_type Comment Index_comment Visible Expression
t1 0 PRIMARY 1 id A 0 NULL NULL BTREE YES NULL
t1 1 v1 1 v1 A 0 1 NULL VECTOR YES NULL
SELECT * FROM INFORMATION_SCHEMA.INNODB_INDEXES;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also how about testing other INFORMATION_SCHEMA tables which contain information about indexes,
like STATISTICS ?

return false;
}

bool Select_lex_builder::add_from_item(PT_table_reference *tr) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change becomes unnecessary if we follow approach with modifying I_S.STATISTICS definition instead.

Comment thread sql/dd/dd_table.cc
col_options->set("is_array", true);
}

if (field.sql_type == MYSQL_TYPE_VECTOR) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I have said above I believe this vector_index marker belongs to Index::options() instead.

IMO even if you prefer to keep Column rows/objects participating in vector indexes marked as such for some reason, it is cleaner to do so only for columns which are part of index only, and not all vector columns.

Comment thread sql/dd/dd_table.cc
return dd::Index::IA_FULLTEXT;

case HA_KEY_ALG_VECTOR:
return dd::Index::IA_SE_SPECIFIC;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, adding new enum elements in ENUM definitions in DD Indexes table will break compatibility with Upstream, which we should avoid. And yes, this won't be change which can be easily fixed by dropping all vector indexes in the system, as not the contents, but definition of DD table is changed in this case.

In theory we can introduce IA_VECTOR and IT_VECTOR on DD API/Index object level and map it to the same IA_SE_SPECIFIC and IT_MULTIPLE in DD tables underneath the API. But IMO this won't simplify things and can be more confusing actually (IMO it is nice when DD API concepts and contents of DD tables match!).

Comment thread sql/dd/dd_table.cc
return dd::Index::IA_FULLTEXT;

case HA_KEY_ALG_VECTOR:
return dd::Index::IA_SE_SPECIFIC;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having said above I think the code can be improved by adding comment here explaining why we map VECTOR algorithm to IA_SE_SPECIFIC. Basically, explain that we don't want to break binary compatibility of DD tables definitions with Upstream by introducing new elements in index algorithm and types enum in DD indexes table.

Comment thread sql/dd/dd_table.cc
}

static dd::Index::enum_index_type dd_get_new_index_type(const KEY *key) {
if (key->flags & HA_VECTOR) return dd::Index::IT_MULTIPLE;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is a good idea to add similar comment here as well (perhaps minimal, referencing the previous comment).

@dlenev dlenev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Martin!

Here is the second part of my comments for your PR.

I expect that the remaining changes (i.e. to InnoDB SE) are to be reviewed and approved by @satya-bodapati .

# Start of Percona Server 8.4/9.7 error messages to be sent to client
#

ER_VECTOR_INDEX_NEEDS_PK

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we can.
Message for ER_NOT_SUPPORTED_YET looks like "This version of MySQL doesn't yet support '%s'", so we use string explaining what is not supported with it. And we can always construct custom error message for any error code (though this is not very user/search-friendly).

Comment thread sql/create_field.cc
const LEX_CSTRING *fld_comment, const char *fld_change,
List<String> *fld_interval_list, const CHARSET_INFO *fld_charset,
bool has_explicit_collation, uint fld_geom_type,
const LEX_CSTRING *fld_zip_dict_name, Value_generator *fld_gcol_info, Value_generator *fld_default_val_expr,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that this change is side-effect of applying clang-format to the whole modified file,
but I would prefer to avoid such kind of changes in code which we don't touch, since they might
complicate merging in future...

Comment thread sql/dd_table_share.cc

@return Whether any visible element belongs to a vector column.
*/
static bool dd_index_has_vector_column(const dd::Index &idx_obj) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I have said before, I think vector_index should be flag in Index::options() and not in Column::options(), this will make this function unnecessary (or perhaps replaceable with trivial helper) and code in this file/fill_index_from_dd() simpler...

Comment thread sql/handler.h
ANALYZE TABLE on it
*/
#define HA_ONLINE_ANALYZE (1LL << 56)
#define HA_CAN_VECTOR (1LL << 57)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to add formal Doxygen comment explaining what this flag means.
Something trivial like:
/** Supports vector indexes (Percona). */
will do.

Comment thread sql/handler.h
for details.
*/
[[nodiscard]] int ha_fast_update(THD *thd,
mem_root_deque<Item *> &update_fields,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I have said earlier, I think it is better to avoid changing parts of code, which we otherwise don't touch, to follow clang-format, as it might create conflicts during merges in future.

Not doing such changes makes the patch a bit more easier to look at/review/analyze as well :)

Comment thread sql/sql_table.cc
if (sql_field->sql_type == MYSQL_TYPE_VECTOR) {
if (sql_field->sql_type == MYSQL_TYPE_VECTOR &&
((key_info->flags & HA_VECTOR) == 0)) {
my_error(ER_NON_SCALAR_USED_AS_KEY, MYF(0), column->get_field_name());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should update text for ER_NON_SCALAR_USED_AS_KEY error as after your changes it sounds a bit misleading (e.g. we can say that vector columns do not support non-vector keys, or something similar)?

Comment thread sql/sql_table.cc
} else if (key->type == KEYTYPE_VECTOR) {
// VECTOR indexes are only allowed on VECTOR columns.
if (sql_field->sql_type != MYSQL_TYPE_VECTOR || sql_field->is_nullable) {
my_error(ER_INDEX_MUST_HAVE_COMPATIBLE_COLUMN, MYF(0), "VECTOR",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... Perhaps it makes sense to adjust error message for this error to make it sound a bit more generic?
Something like: "A %.20s index may only contain compatible columns."
So it covers non-NULLability check as well.
Or alternatively we can say "... only non-nullable VECTOR columns".
What do you think?

Comment thread sql/sql_table.cc

if (key->type == KEYTYPE_VECTOR) {
if (vector_key_number != 0U) {
my_error(ER_ONLY_SINGLE_VECTOR_INDEX_ALLOWED, MYF(0));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I have mentioned earlier I am not sure that is worth to introduce new error code and messages for temporary limitations. So perhaps it is better to use ER_NOT_SUPPORTED_YET with appropriate parameter instead?

Comment thread sql/sql_table.cc

// We allow VECTOR keys only with tables with PK
if (!primary_key && vector_key_number) {
my_error(ER_VECTOR_INDEX_NEEDS_PK, MYF(0));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Comment thread sql/sql_table.cc
const KEY &primary_info = *key_info_buffer[0];

if (primary_info.actual_key_parts > 1) {
my_error(ER_VECTOR_INDEX_NEEDS_PK, MYF(0));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here and a few lines below as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants