Skip to content

Conversation

morgo
Copy link
Contributor

@morgo morgo commented Oct 18, 2024

Fixes #237

There are no tests currently, because I didn't see a straight forward way to test (the rules of if something is redundant is unchanged). I did perform a manual test:

2024-10-18 07:34:02 [WARN]  /tmp/skeema-tmp/localhost/XXX/XXX.sql:6: Index idx_XXX_id of table `XXX` is redundant to larger index unq_XXX.
                            In most cases it is safe to drop index idx_XXX_id, but consider making it INVISIBLE first. Redundant indexes waste disk space, and harm write performance.

Copy link

Coverage Status

coverage: 93.27% (+0.004%) from 93.266%
when pulling dd1bc45 on morgo:refine-redundant-index-message
into 47bfc0a on skeema:main.

@evanelias
Copy link
Contributor

Looks perfect, and thank you for also tackling the invisible/ignored part, this is great! Merging momentarily.

re: tests, yeah the existing integration test framework for the linter subpackage will already confirm the checker is triggered where expected (see internal/linter/testdata/validcfg/dupeidx.sql), but as you mentioned that aspect of the checker hasn't changed here anyway.

The testing framework currently just catches "is this checker flagging the things we expect, and not anything else" but it doesn't handle specific text matching yet. Regarding the invisible/ignored logic, we can be fairly confident your code is correct, since the test coverage for check_dupe_index.go has remained at 100%. That makes sense since GH Actions is currently configured to use MySQL 5.7 (no invisible/ignored), MySQL 8.0 (invisible) and MariaDB 10.11 (ignored) so all 3 cases there are indeed expected to be covered 👍

@evanelias evanelias merged commit 665f978 into skeema:main Oct 18, 2024
3 checks passed
@evanelias
Copy link
Contributor

Skeema v1.12.1 has been released, including this change. Thanks again for the excellent pull request!

svc-squareup-copybara pushed a commit to cashapp/misk that referenced this pull request Dec 3, 2024
| Package | Package file | Manager | Update | Change |
|---|---|---|---|---|
| [skeema](https://github.com/skeema/skeema) | misk/bin/hermit | hermit
| patch | `1.12.0` -> `1.12.1` |

---

### Release Notes

<details>
<summary>skeema/skeema (skeema)</summary>

### [`v1.12.1`](https://github.com/skeema/skeema/releases/tag/v1.12.1)

- **MySQL 9.0 and 9.1** support
([`7c94fcb`](skeema/skeema@7c94fcb),
[`47bfc0a`](skeema/skeema@47bfc0a),
[`a0a060a`](skeema/skeema@a0a060a))
- The new `VECTOR` column type is supported. In [`skeema
diff`](https://www.skeema.io/docs/commands/diff/) and [`skeema
push`](https://www.skeema.io/docs/commands/push/), altering a column
type between `VECTOR` and any other sufficiently-large binary type is
permitted as a [safe
operation](https://www.skeema.io/docs/features/safety/#unsafe-change-detection)
since the conversion is non-lossy.
- MySQL 9 finally processes "inline" foreign key definitions (that is,
`REFERENCES` clause in a column definition). These are supported as-is
in Skeema.
- **MariaDB 11.5 and 11.6** support
([`6165c90`](skeema/skeema@6165c90),
[`f24ad30`](skeema/skeema@f24ad30))
- MariaDB 11.5 changes the default collation for Unicode charsets to use
uca1400\_ai_ci collations, which are fully supported in Skeema.
- MariaDB 11.5 solves the `TIMESTAMP` col type's previous Y2K38
limitation, and Skeema's
[lint-has-time](https://www.skeema.io/docs/options/#lint-has-time)
annotation message has been adjusted accordingly.
- **`CHECK` constraint improvement**: When a diff only affects the
*name* of a `CHECK` constraint without modifying its check expression,
[`skeema diff`](https://www.skeema.io/docs/commands/diff/) and [`skeema
push`](https://www.skeema.io/docs/commands/push/) now ignore this
cosmetic change by default. This improves compatibility with external
[OSC tools](https://www.skeema.io/docs/features/osc/), which inherently
need to rename `CHECK` constraints as part of their operation. This new
behavior can be overridden by enabling the
[--exact-match](https://www.skeema.io/docs/options/#exact-match) option.
([`f000616`](skeema/skeema@f000616))
- **[Event](https://www.skeema.io/docs/features/events/) handling**
improvements and fixes ([Skeema
Premium](https://www.skeema.io/download/))
- When an event diff only included a change to the `DEFINER` clause, and
no other differences, the `ALTER EVENT` emitted by Skeema was not valid
SQL (despite conforming to syntax in the MySQL and MariaDB manuals). To
fix this situation, the SQL will now also include an additional no-op
clause, such as `ENABLE` for an event that is already enabled.
- Several dump normalizations for `CREATE EVENT` statements were
inadvertently omitted the first time an event was dumped by [`skeema
init`](https://www.skeema.io/docs/commands/init/) or [`skeema
pull`](https://www.skeema.io/docs/commands/pull/).
- If any workspace query failed (e.g. query timeout), and any events
were present in the \*.sql definitions, a panic would result instead of
the intended workspace query failure error message.
([#&#8203;229](skeema/skeema#229))
- **[SSH tunnel](https://www.skeema.io/docs/features/ssh/)
enhancements** ([Skeema Premium](https://www.skeema.io/download/))
- CAs, which are indicated in the known_hosts file using
`@cert-authority` lines, are now fully supported.
([skeema/knownhosts#8](skeema/knownhosts#8),
[skeema/knownhosts#9](skeema/knownhosts#9))
- known_hosts lines using non-default ports are now matched properly.
([skeema/knownhosts#10](skeema/knownhosts#10))
- If any hand-written \*.sql files use the optional **`CREATE OR
REPLACE` SQL syntax**, Skeema now parses and ignores the `OR REPLACE`
clause. Previously, use of this syntax would prevent Skeema from parsing
the statement.
([`6805737`](skeema/skeema@6805737))
- Enhancements for [Docker
workspaces](https://www.skeema.io/docs/features/workspaces/#docker-workspaces)
- Significant performance improvements for several common situations
([`d348249`](skeema/skeema@d348249),
[`ca85df7`](skeema/skeema@ca85df7),
[`7a40155`](skeema/skeema@7a40155))
- When using Percona Server 8.x, the Docker image / point release
selection logic has been improved
([`e09350c`](skeema/skeema@e09350c),
[`fe55d62`](skeema/skeema@fe55d62),
[`af1b3b5`](skeema/skeema@af1b3b5))
- When a redundant non-unique index is flagged by
[lint-dupe-index](https://www.skeema.io/docs/options/#lint-dupe-index),
the annotation message is now clearer (since false positives may be
possible) and suggests making the index be `INVISIBLE` / `IGNORED`
before dropping
([#&#8203;238](skeema/skeema#238),
[#&#8203;237](skeema/skeema#237))
- MariaDB's August 2024 point releases have changed the formatting of
compressed columns in `SHOW CREATE TABLE`, which affected Skeema's [diff
logic
safeguards](https://www.skeema.io/docs/features/safety/#table-introspection-validation).
This change is now handled and compressed columns are fully supported
again. ([`49aed41`](skeema/skeema@49aed41))
- Minor wording changes in log messages and help text, for consistency.
([`4f8fa44`](skeema/skeema@4f8fa44),
[`5f7598e`](skeema/skeema@5f7598e))

**Thank you** to all code contributors and issue reporters!

An [installation guide](https://www.skeema.io/docs/install/) and [full
documentation](https://www.skeema.io/docs/) are available on our website
[skeema.io](https://www.skeema.io/).

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 6pm every weekday,before 2am
every weekday" in timezone Australia/Melbourne, Automerge - At any time
(no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://github.com/renovatebot/renovate).

GitOrigin-RevId: 90a37c8a0c3c86b3fa245502cfeefd429129ebed
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.

Redundant Index checker is too confident
2 participants