fix: prevent multiple pvm errors on migration #31332

eschutho · 2024-12-06T23:32:14Z

SUMMARY

We're seeing some errors when running the catalog migration because of some PVMs that already exist. In that case, I'm not going to update the PVM.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

sadpandajoe · 2024-12-06T23:40:25Z

superset/migrations/versions/2024-05-01_10-52_58d051681a3b_add_catalog_perm_to_tables.py

@@ -45,10 +49,21 @@ def upgrade():
        "slices",
        sa.Column("catalog_perm", sa.String(length=1000), nullable=True),
    )
+
+    if not table_has_index(perm_table, perm_index_view_menu):


might be good to add these into a helper function call create_index that will do the check and add it like here #31303

Nice, I was thinking of a helper function. I don't want to create a conflict with that PR. Perhaps we can use his helper function after it's merged.

eschutho · 2024-12-07T00:36:15Z

superset/migrations/shared/catalogs.py

+                    "New permission %s already exists. Removing old one",
+                    new_perm,
+                )
+                session.delete(existing_pvm)


@betodealmeida not sure if you would prefer to return pvms to be deleted instead of deleting them here

Do we really need to delete anything here? Should we just rename the existing ones and create new ones for new catalogs? The old ones are attached to roles already and we probably want to maintain those associations.

@mistercrunch the current code does that, we get the current perm (where the catalog name is null, see how we compute current_perm in line 492) and change it to point to the new perm name new_perm, which includes the default catalog name (line 497). Then we simply rename the existing PVM to have the new name, so that existing associations like roles are preserved (line 508 on the left).

In regards to this PR, I don't think we should delete existing PVMs. First, because they should not exist... the only reason I can imagine why it would exist is bad upgrade-downgrade that didn't rename the PVMs correctly. If that happens, I think it's better to just keep the existing PVM, since it already has the correct name, and might be associated with roles.

eschutho · 2024-12-07T00:38:27Z

superset/migrations/versions/2024-05-01_10-52_58d051681a3b_add_catalog_perm_to_tables.py

+    if table_has_index(perm_table, perm_index_permission_id):
+        op.drop_index(op.f(perm_index_permission_id), table_name=perm_table)
+    if table_has_index(perm_table, perm_index_view_menu):
+        op.drop_index(op.f(perm_index_view_menu), table_name=perm_table)


@betodealmeida any thoughts whether you think this change should stay with the new migration or in a different PR? My thought is to keep them together so that tables don't get out of sync, but I also see the benefit of splitting them up.

mistercrunch · 2024-12-07T21:02:09Z

superset/migrations/shared/security_converge.py

-    __table_args__ = (UniqueConstraint("permission_id", "view_menu_id"),)
+    __table_args__ = (
+        sqla.Index("idx_permission_view_menu_id", "view_menu_id"),
+        sqla.Index("idx_permission_permission_id", "permission_id"),


haven't looked at the queries against that table, but could be that a compound index work well here. I saw some queries that predicate on view_menu_id and some on both view_menu_id and permission_id. If permission is never predicated by itself, the compound on view_menu_id and permission_id (in that order) would cover those

Adding more info here, FAB already has a compound index since it defines a unique constraint on these FK: https://github.com/preset-io/Flask-AppBuilder/blob/master/flask_appbuilder/security/sqla/models.py#L73

Checked the security converge code, and we have the following queries:

session.query(ViewMenu).filter(ViewMenu.name == view_name).one_or_none()

session.query(Permission).filter(Permission.name == permission_name).one_or_none()

session.query(PermissionView) .filter( PermissionView.view_menu_id == view_menu.id, PermissionView.permission_id == permission.id, )

session.query(PermissionView) .join(Permission) .join(ViewMenu) .filter(ViewMenu.name == view_name, Permission.name == permission_name)

session.query(PermissionView) .join(Permission) .filter(Permission.name == old_permission_name)

session.query(PermissionView) .join(ViewMenu) .filter(ViewMenu.name == old_view_name)

We have indexes to efficiently query all of these:

index on ViewMenu.name

index on Permission.name

Indexes on Permission.id and ViewMenu.id

Yet we can have more eficient joins by adding indexes on the FK for ab_permission_view_role and ab_permission_view
dpgaspar/Flask-AppBuilder#2293

Thanks @dpgaspar! I'll remove the changes here and we'll bring them in from FAB then.

betodealmeida · 2024-12-09T15:17:33Z

superset/migrations/shared/catalogs.py

+                    "New permission %s already exists. Removing old one",
+                    new_perm,
+                )
+                session.delete(existing_pvm)


@mistercrunch the current code does that, we get the current perm (where the catalog name is null, see how we compute current_perm in line 492) and change it to point to the new perm name new_perm, which includes the default catalog name (line 497). Then we simply rename the existing PVM to have the new name, so that existing associations like roles are preserved (line 508 on the left).

In regards to this PR, I don't think we should delete existing PVMs. First, because they should not exist... the only reason I can imagine why it would exist is bad upgrade-downgrade that didn't rename the PVMs correctly. If that happens, I think it's better to just keep the existing PVM, since it already has the correct name, and might be associated with roles.

betodealmeida · 2024-12-09T15:24:48Z

superset/migrations/shared/catalogs.py

+                    "New permission %s already exists. Deleting old one",
+                    new_name,
+                )
+                pvms_to_delete.append(pvm)


Same here, I would just leave the existing one, something like:

# check to see if the new name already exists if not session.query(ViewMenu).filter_by(name=new_name).one_or_none(): # new perm doesn't exist, rename existing one to the new name pvms_to_rename.append((pvm, new_name))

mistercrunch · 2024-12-09T17:16:08Z

Yes, deleting is yet more potential locks. I also think there are methods/CLI commands that will clean up perms that should not exist. I forgot whether it's a fab feature or not or what triggers it, but may even be on app startup (?)

mistercrunch

LGTM

eschutho added 3 commits December 6, 2024 15:22

add indexes to catalog perms

bea58b7

add index to new migration

7fbaf99

add checks before dropping

ea13294

eschutho requested review from mistercrunch, michael-s-molina and betodealmeida as code owners December 6, 2024 23:32

pull-request-size bot added the size/M label Dec 6, 2024

github-actions bot added the risk:db-migration PRs that require a DB migration label Dec 6, 2024

dosubot bot added the change:backend Requires changing the backend label Dec 6, 2024

sadpandajoe reviewed Dec 6, 2024

View reviewed changes

eschutho changed the title ~~Elizabeth/add indexes perms~~ fix: prevent deadlocks on catalogs migration Dec 7, 2024

github-actions bot added the preset-io label Dec 7, 2024

add duplicate view name entry error handling

fc45193

eschutho force-pushed the elizabeth/add-indexes-perms branch from 069b0fa to fc45193 Compare December 7, 2024 00:35

eschutho commented Dec 7, 2024

View reviewed changes

mistercrunch reviewed Dec 7, 2024

View reviewed changes

betodealmeida reviewed Dec 9, 2024

View reviewed changes

eschutho added 2 commits December 9, 2024 16:16

do not delete existing PVMs

8ba02ec

revert migration changes

5e2ef61

pull-request-size bot added size/XS and removed size/M labels Dec 10, 2024

eschutho changed the title ~~fix: prevent deadlocks on catalogs migration~~ fix: prevent multiple pvm errors on migration Dec 10, 2024

eschutho removed the risk:db-migration PRs that require a DB migration label Dec 10, 2024

mistercrunch approved these changes Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prevent multiple pvm errors on migration #31332

fix: prevent multiple pvm errors on migration #31332

eschutho commented Dec 6, 2024 •

edited

Loading

sadpandajoe Dec 6, 2024

eschutho Dec 7, 2024

eschutho Dec 7, 2024

mistercrunch Dec 7, 2024

betodealmeida Dec 9, 2024

eschutho Dec 7, 2024

mistercrunch Dec 7, 2024

dpgaspar Dec 9, 2024

eschutho Dec 10, 2024

betodealmeida Dec 9, 2024

betodealmeida Dec 9, 2024

mistercrunch commented Dec 9, 2024

mistercrunch left a comment

fix: prevent multiple pvm errors on migration #31332

Are you sure you want to change the base?

fix: prevent multiple pvm errors on migration #31332

Conversation

eschutho commented Dec 6, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch commented Dec 9, 2024

mistercrunch left a comment

Choose a reason for hiding this comment

eschutho commented Dec 6, 2024 •

edited

Loading