Skip to content

Add visible field to schema #1043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

chenhuiyeh
Copy link
Contributor

@chenhuiyeh chenhuiyeh commented Jun 5, 2025

This pull request introduces a new feature targeting MySQL version 8 and above, where the concept of invisible indexes has been introduced. Specifically, this PR adds a Visible field to the schema.Index struct, allowing users to easily filter out invisible indexes.

We encountered an issue with ghostferry where an invisible index was mistakenly selected as the shardingKeyIndexName. To address this, we initially applied a patch directly to the ghostferry project, as documented in ghostferry PR #383. This current PR seeks to integrate those changes into the upstream codebase for broader applicability. Part of Shopify/ghostferry#384.

@chenhuiyeh chenhuiyeh force-pushed the add-visible-field-to-schema branch from 3af6107 to c7a82aa Compare June 5, 2025 21:35
@chenhuiyeh chenhuiyeh marked this pull request as ready for review June 5, 2025 21:47
Copy link
Collaborator

@lance6716 lance6716 left a comment

Choose a reason for hiding this comment

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

rest lgtm

// - "8.0.25-0ubuntu0.20.04.1"
func isMySQL8OrAbove(version string) bool {

// Extract the version number before any additional info (like "-log", "-ubuntu", etc.)
Copy link
Collaborator

Choose a reason for hiding this comment

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

CompareServerVersions has been updated, the comment says "ignoring anything past - in version.". I think you can directly use it.

@@ -338,6 +391,14 @@ func (ta *Table) fetchIndexes(conn mysql.Executer) error {
colName, _ := r.GetString(i, 4)
currentIndex.AddColumn(colName, cardinality)
currentIndex.NoneUnique, _ = r.GetUint(i, 1)

// Only set to false if explicitly marked as invisible
if hasInvisibleIndex {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: considering MariaDB use case, we can check the columns returned by show index to know if there are Visible column.

}

func hasInvisibleIndexSupport(conn mysql.Executer) bool {
versionQuery := "SELECT VERSION()"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can use the version from the handshake and skip the extra roundtrip?

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.

3 participants