Skip to content
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

Renaming/Removing columns clashes with index recreation #2129

Closed
ndm2 opened this issue Oct 25, 2022 · 0 comments
Closed

Renaming/Removing columns clashes with index recreation #2129

ndm2 opened this issue Oct 25, 2022 · 0 comments
Labels

Comments

@ndm2
Copy link
Contributor

ndm2 commented Oct 25, 2022

As hinted in #2126, (explicitly) renaming/removing columns clashes with the index recreation fix for SQLite added in #2040, as it will try to recreate indices on columns that do not exist anymore, and trigger errors accordingly.

The behavior of the other supported DBMS isn't consistent:

Single column indices

Renaming a column:

  • MySQL: Renames the column in the index
  • Postgres: Renames the column in the index
  • SQL Server: Renames the column in the index (and triggers a warning)

Removing a column:

  • MySQL: Removes the complete index
  • Postgres: Removes the complete index
  • SQL Server: Triggers an error

Composite indices

Renaming a column:

  • MySQL: Renames the column in the index
  • Postgres: Renames the column in the index
  • SQL Server: Renames the column in the index (and triggers a warning)

Removing a column:

  • MySQL: Removes the column from the index
  • Postgres: Removes the complete index
  • SQL Server: Triggers an error

Indices with expressions:

Renaming a column:

  • MySQL: Triggers an error
  • Postgres: Renames the column in the index

Removing a column:

  • MySQL: Triggers an error
  • Postgres: Removes the complete index

How should the SQLite adapter behave?

I would suggest that indices with expressions should trigger an error, IMHO it's not feasible to go full blown SQL parsing in order to support them. And since they can only be detected as belonging to a specific column when that column is not in an expression, I would say that any index with an expression should simply be ignored alltogether, so that they will cause an SQL error when trying to recreate them. People that are using such custom indices will simply have to deal with removing/adding them themselves, just like they'd currently have to do it with MySQL.

Renaming columns should simply rename the columns in the index, that should be easy enough. As noted in #2126, it seems that the rule for indices is that almost everything after CREATE [UNIQUE] INDEX is basically (syntactically correct) garbage in, garbage out, so a little regex foo might already do the job. For example:

CREate INdex test_idx On test (foo asc , aBs(bar)   DEsc )

is stored as-is, except for CREate INdex, meaning sqlite_master will contain/return:

CREATE INDEX test_idx On test (foo asc , aBs(bar)   DEsc )

And removing columns, I'm not sure, personally I'd be fine with either the behavior of MySQL or Postgres, ie either remove individual columns, or always remove the complete index, even if only one of multiple columns is being removed.

@ndm2 ndm2 added the bug label Oct 26, 2022
ndm2 added a commit to ndm2/phinx that referenced this issue Nov 1, 2022
ndm2 added a commit to ndm2/phinx that referenced this issue Nov 1, 2022
ndm2 added a commit to ndm2/phinx that referenced this issue Nov 1, 2022
ndm2 added a commit to ndm2/phinx that referenced this issue Dec 7, 2022
ndm2 added a commit to ndm2/phinx that referenced this issue Dec 7, 2022
ndm2 added a commit to ndm2/phinx that referenced this issue Dec 19, 2022
ndm2 added a commit to ndm2/phinx that referenced this issue Jan 7, 2023
ndm2 added a commit to ndm2/phinx that referenced this issue Jan 7, 2023
@ndm2 ndm2 closed this as completed Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant