Creating and dropping indexes from migration scripts

[stronk7]

Hi,

one of the missing details that I've found when playing with Piccolo, is the inability to create and maintain composite (multiple columns/fields) indexes when defining tables.

I've been reading #984 , #172 , #175 and haven't found any way to do so (other than using raw sql in migrations).

In the other side, I've seen that there are some "internal" functions providing composite index creation and deletion (only not-unique ones, but that's better than nothing). So I've tried this simple migration script. The problem is that I get forwards() working, but backwards() does not, and I cannot imagine why.

Here it is the testing migration code:

from piccolo.apps.migrations.auto.migration_manager import MigrationManager

from myapp.tables import TestTable

ID = "2024-10-28T20:07:36:439746"
VERSION = "1.21.0"
DESCRIPTION = "test-add-drop-index"


async def forwards():
    manager = MigrationManager(
        migration_id=ID, app_name="", description=DESCRIPTION,
    )

    async def run():
        await TestTable.create_index(
            columns = ["col1", "col2"], # or ([TestTable.col1, TestTable.col2])
            if_not_exists = True
        ).run()

    manager.add_raw(run)
    return manager

async def backwards():
    manager = MigrationManager(
        migration_id=ID, app_name="", description=DESCRIPTION
    )

    async def run():
        await TestTable.drop_index(
            columns=["col1", "col2"], # or ([TestTable.col1, TestTable.col2])
            if_exists=True
        ).run()

    manager.add_raw(run)
    return manager

When I run piccolo migrations forwards myapp I successfully get the test_table_col1_col2 index created, but when I execute the corresponding backwards command (piccolo migrations backwards myapp), piccolo tells me that the migration step has been executed ok (and the check command confirms that the step is pending execution), but the index has not been really dropped.

I've tried some basic debugging, adding logging lines here and there and, somehow, it seems that the backwards() function isn't ever executed, and it's driving me crazy.

Am I missing anything? (note I'm completely new to Piccolo, so I may be missing something obvious!)

It would really great to support (composite, both normal and unique) index managing from tables definition, wow!

TIA and ciao :-)

[sinisaos]

@stronk7 Yes, you are right. I think the problem is in this line, because with that line

piccolo/piccolo/apps/migrations/commands/backwards.py

Line 90 in 448a818

response = await migration_module.forwards()

forwards method is always called even if we use backwards in the cli. For now I can only drop the indexes if I make another empty migration and pass Table.drop_index in the forwards method.

I played around with this a bit and made these changes to the source code.
First, I added a backwards method to Jinja template like this

# all previous code is unchanged
async def backwards():
    manager = MigrationManager(migration_id=ID, app_name="{{ app_name }}", description=DESCRIPTION)
    
    def run():
        print(f"running {ID}")

    manager.add_raw(run)
    
    return manager

Second, I change these lines
with this

response = await migration_module.backwards()

if isinstance(response, MigrationManager):
    if self.preview:
        response.preview = True
    await response.run()

After that I was able to drop the indexes using piccolo migrations backwards your_app_name your_migration_id. I don't know if this has any bad implications (migrations are hard to get right), but you can try to patch a local Piccolo installation with this changes. I hope that makes sense

[stronk7]

Many thanks for your explanations, @sinisaos.

I've (blindly - I certainly don't know what I'm doing!) applied your suggested changes and can confirm that, now, the backwards code is being executed ok (and the index is being dropped, as expected).

$ piccolo migrations forwards myapp
2024-10-29T18:16:02.478Z INFO [piccolo.engine.base] Running Postgres version 13.16
👍 2 migrations already complete
⏩ 1 migration not yet run
🚀 Running 1 migration:
  - 2024-10-29T20:07:36:439746 [forwards]... ok! ✔️

$ piccolo migrations backwards myapp
2024-10-29T18:16:29.854Z INFO [piccolo.engine.base] Running Postgres version 13.16
Reverse 1 migration? [y/N] y
  - 2024-10-29T20:07:36:439746 [forwards]... ok! ✔️

Two notes and a question:

• Not sure what the jinja changes are doing. In fact I forgot to save them and got it also working at first attempt, with the very same output than later, once saved.
• Only a detail, the backwards execution is showing [forwards] too, fun.

And the question, should this be reported as bug? Does it affect to ALL backwards migrations, or only "particular" ones, like my one above?

Again, many thanks for the help, ciao :-)

[sinisaos]

• Not sure what the jinja changes are doing. In fact I forgot to save them and got it also working at first attempt, with the very same output than later, once saved.

That's strange. With that change we add a backwards function to the migration template (when we run piccolo migrations new app_name command)

And the question, should this be reported as bug?

Why not. Probably the author of the library has a better solution for this problem.

Does it affect to ALL backwards migrations, or only "particular" ones, like my one above?

Unfortunately not. These changes only affect the specific migration with migration_id.

[stronk7]

That's strange. With that change we add a backwards function to the migration template (when we run piccolo migrations new app_name command)

Ah, got it. that's the template! As far as I already had my migration step created, I did not need to create a new one.

Why not. Probably the author of the library has a better solution for this problem.

#1122 created

Thanks!

[Jacky56]

hello all,

I also did the exact same way implementing the backwards method, but it seems this is the actual way to implement the backwards method is here.

looks like nesting run_backwards in forwards and using manager.add_raw_backwards suppose to work? but I think its kinda not very straight forward as suppose to the implementation shown on the description.

[sinisaos]

@Jacky56 Yes, you are right. I didn't see it at all. Everything works with manager.add_raw_backwards(run_backwards) without intervention in the source code. We only need to do this (like in docs) to empty migration (piccolo migrations new home)

from piccolo.apps.migrations.auto.migration_manager import MigrationManager
from home.tables import TestTable


ID = "2024-11-10T12:04:39:118866"
VERSION = "1.22.0"
DESCRIPTION = ""


async def forwards():
    manager = MigrationManager(
        migration_id=ID, app_name="", description=DESCRIPTION
    )

    async def run():
        await TestTable.create_index(
            columns = ["col1", "col2"], # or ([TestTable.col1, TestTable.col2])
            if_not_exists = True
        ).run()

    manager.add_raw(run)

    # method for backwards migration
    async def run_backwards():
        await TestTable.drop_index(
            columns=["col1", "col2"], # or ([TestTable.col1, TestTable.col2])
            if_exists=True
        ).run()

    manager.add_raw_backwards(run_backwards)

    return manager

and everything works :).

Creating indexes with piccolo migrations forwards home
Result is:

testdb=# \d test_table
                                    Table "public.test_table"
 Column |          Type          | Collation | Nullable |                Default                 
--------+------------------------+-----------+----------+----------------------------------------
 id     | integer                |           | not null | nextval('test_table_id_seq'::regclass)
 col1   | character varying(255) |           | not null | ''::character varying
 col2   | character varying(255) |           | not null | ''::character varying
Indexes:
    "test_table_pkey" PRIMARY KEY, btree (id)
    "test_table_col1_col2" btree (col1, col2)  <-- created indexes

Dropping indexes:

piccolo migrations backwards home
Reverse 1 migration? [y/N] y
  - 2024-11-10T12:04:39:118866 [backwards]... ok! ✔️

Result is:

testdb=# \d test_table
                                    Table "public.test_table"
 Column |          Type          | Collation | Nullable |                Default                 
--------+------------------------+-----------+----------+----------------------------------------
 id     | integer                |           | not null | nextval('test_table_id_seq'::regclass)
 col1   | character varying(255) |           | not null | ''::character varying
 col2   | character varying(255) |           | not null | ''::character varying
Indexes:
    "test_table_pkey" PRIMARY KEY, btree (id)
    # indexes dropped

[stronk7]

Wow, now I'm lost. So, which is the correct way to write custom migrations, specifically backwards ones:

1. forwards() (calling to add_raw) and backwards() (calling to add_raw_backwards?)`.
2. Only forwards() with both run() and run_backwards(), each one calling to their add_raw_xxx
3. ...

[Jacky56]

1. 2. yes, it seems like so, honestly your implementation in the description is the most logical sense ngl.

[sinisaos]

I think (thanks @Jacky56 for pointing out that method in the docs, I didn't see it at all) that the second option is better, because we can use it without intervening in the source code. We can also use the first option, but it requires changes to the source code, so I think the second option is better for now. The usage is as described in the previous comment.