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

Auto-created index name have a big chance to clash with the table name or name of some structure in the database. #990

Open
metakot opened this issue May 20, 2024 · 3 comments

Comments

@metakot
Copy link

metakot commented May 20, 2024

So I have this two tables in postgres:

from piccolo.columns import *
from piccolo.table import Table

class EmployeeRole(Table):
    name = Varchar()

class Employee(Table):
    name = Varchar()
    role = ForeignKey(EmployeeRole, index=True)

The migration file is created successfully, but the migration itself ends with the error:
asyncpg.exceptions.DuplicateTableError: relation "employee_role" already exists

It turns out that piccolo generates the following SQL:
CREATE INDEX employee_role ON "employee" USING btree ("role");
And this clashes with the name of the table.

The code for index name lives here:

    @classmethod
    def _get_index_name(cls, column_names: t.List[str]) -> str:
        """
        Generates an index name from the table name and column names.
        """
        return "_".join([cls._meta.tablename] + column_names)

To prevent that kind of name conflicts, can we add the suffix to the index name, like _idx_XXXX there X is the random hex symbol?

UPD: however, in order to be able to gracefully undo the migration backwards the migration file must store the index name somethere or look it up from the database.

@dantownsend
Copy link
Member

I think we should probably add an index_name argument for situations like this.

The downside is migrations would have to be aware of this, and if the user changes index_name we need to change it in the database. It's not too complex though:

ALTER INDEX name RENAME TO new_name

@Real-Gecko
Copy link

Any news on this issue?

@metakot
Copy link
Author

metakot commented Dec 4, 2024

@Real-Gecko Any news on this issue?

I made a small workaround:

import random

from piccolo.table import Table


@classmethod
def _get_index_name(cls, column_names: list[str]) -> str:
    """
    Generates a semi-unique index name from the table name and column names.
    """
    hex_range = range(16)
    random_symbols = ''.join([f'{random.choice(hex_range):01x}' for i in range(4)])
    return '_'.join([cls._meta.tablename] + column_names + ['idx', random_symbols])

Table._get_index_name = _get_index_name

However, in this case you will not be able to correctly undo the migration and will have to delete the index manually if necessary.
OR just add "idx" part and forget about randomness, maybe.

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

No branches or pull requests

3 participants