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.

Sign up for GitHub

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

fix: prevent multiple pvm errors on migration #31332

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+6 −2
Open

fix: prevent multiple pvm errors on migration #31332

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bea58b7
add indexes to catalog perms
eschutho Dec 6, 2024
7fbaf99
add index to new migration
eschutho Dec 6, 2024
ea13294
add checks before dropping
eschutho Dec 6, 2024
fc45193
add duplicate view name entry error handling
eschutho Dec 7, 2024
8ba02ec
do not delete existing PVMs
eschutho Dec 10, 2024
5e2ef61
revert migration changes
eschutho Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion superset/migrations/shared/security_converge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import logging
from dataclasses import dataclass

import sqlalchemy as sqla
from sqlalchemy import (
Column,
ForeignKey,
Expand Down Expand Up @@ -94,7 +95,10 @@ def __repr__(self) -> str:

class PermissionView(Base): # type: ignore
__tablename__ = "ab_permission_view"
__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"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screenshot 2024-12-09 at 10 36 03

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)
id = Column(Integer, Sequence("ab_permission_view_id_seq"), primary_key=True)
permission_id = Column(Integer, ForeignKey("ab_permission.id"))
permission = relationship("Permission")
Expand Down
17 changes: 16 additions & 1 deletion superset/migrations/versions/2024-05-01_10-52_58d051681a3b_add_catalog_perm_to_tables.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,16 @@
downgrade_catalog_perms,
upgrade_catalog_perms,
)
from superset.migrations.shared.utils import add_column_if_not_exists
from superset.migrations.shared.utils import add_column_if_not_exists, table_has_index

# revision identifiers, used by Alembic.
revision = "58d051681a3b"
down_revision = "4a33124c18ad"

perm_table = "ab_permission_view"
perm_index_view_menu = "idx_permission_view_menu_id"
perm_index_permission_id = "idx_permission_permission_id"


def upgrade():
add_column_if_not_exists(
Expand All @@ -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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

op.create_index(op.f(perm_index_view_menu), perm_table, ["view_menu_id"])

if not table_has_index(perm_table, perm_index_permission_id):
op.create_index(op.f(perm_index_permission_id), perm_table, ["permission_id"])

upgrade_catalog_perms(engines={"postgresql"})


def downgrade():
downgrade_catalog_perms(engines={"postgresql"})
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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

op.drop_column("slices", "catalog_perm")
op.drop_column("tables", "catalog_perm")
51 changes: 51 additions & 0 deletions superset/migrations/versions/2024-12-06_15-19_8a23aa3bdd84_add_index_to_permission_view.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
"""Add index to permission view

Revision ID: 8a23aa3bdd84
Revises: 48cbb571fa3a
Create Date: 2024-12-06 15:19:00.030588

"""

from alembic import op

from superset.migrations.shared.utils import table_has_index

# revision identifiers, used by Alembic.
revision = "8a23aa3bdd84"
down_revision = "48cbb571fa3a"

perm_table = "ab_permission_view"
perm_index_view_menu = "idx_permission_view_menu_id"
perm_index_permission_id = "idx_permission_permission_id"


def upgrade():
if not table_has_index(perm_table, perm_index_view_menu):
op.create_index(op.f(perm_index_view_menu), perm_table, ["view_menu_id"])

if not table_has_index(perm_table, perm_index_permission_id):
op.create_index(op.f(perm_index_permission_id), perm_table, ["permission_id"])


def downgrade():
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)
Loading