feat: optimize catalog permission sync

[betodealmeida]

SUMMARY

For performance reasons, change the SyncPermissionsCommand to only sync permissions for non-default catalogs when at least one of these conditions is true:

1. Cross-catalog queries are supported (for example, in BigQuery, but not RDS). When cross-catalog queries it's good to create t/he permissions for non-default catalogs so that the admin can manage access to them.
2. Multi-catalog is enabled in the database. When multi-catalog is enabled the admin needs to have the permissions for the same reason.

This means that for a database like RDS or Postgres, when we sync the permissions we will use only the default catalog, unless multi-catalog is enabled.

Fixes #32993.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

Added tests.

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

[korbit-ai]

Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment /korbit-review.

Your admin can change your review schedule in the Korbit Console

[korbit-ai]

I've completed my review and didn't find any issues.

Files scanned

​

File Path	Reviewed
superset/db_engine_specs/doris.py	✅
superset/commands/database/sync_permissions.py	✅
superset/db_engine_specs/snowflake.py	✅
superset/db_engine_specs/postgres.py	✅
superset/db_engine_specs/databricks.py	✅
superset/db_engine_specs/bigquery.py	✅
superset/db_engine_specs/presto.py	✅
superset/db_engine_specs/base.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

• You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.
• You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.
• Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.
• On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
• Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

• Check out our docs on how you can make Korbit work best for you and your team.
• Customize Korbit for your organization through the Korbit Console.

Current Korbit Configuration

General Settings

​

Setting	Value
Review Schedule	Automatic excluding drafts
Max Issue Count	10
Automatic PR Descriptions	❌

Issue Categories

​

Category	Enabled
Documentation	✅
Logging	✅
Error Handling	✅
Readability	✅
Design	✅
Performance	✅
Security	✅
Functionality	✅

Feedback and Support

• Tell us what you think of Korbit
• Schedule a call with our team
• Email us @ support@korbit.ai

Note

Korbit Pro is free for open source projects 🎉

Looking to add Korbit to your team? Get started with a free 2 week trial here

[arafoperata]

There's a similar check here:

superset/superset/commands/database/sync_permissions.py

Lines 143 to 147 in db959a6

	catalogs = (
	self._get_catalog_names()
	if self.db_connection.db_engine_spec.supports_catalog
	else [None]
	)

Can these two conditions be consolidated?

[Vitor-Avila]

I believe that block is actually calling this method, so I think we're good? Would defer to @betodealmeida to confirm

[betodealmeida]

Yeah, those are different.

Basically with my changes _get_catalog_names returns all relevant catalogs in a given database. This could be all catalogs, or just the default.

When a database doesn't support catalogs, then we need to use None as the catalog, hence the [None].

[Vitor-Avila]

Based on @arafoperata's feedback, we probably want to say "like Postgres" as it's not tied to RDS

[Vitor-Avila]

LGTM! Thanks for working on this improvement

[arafoperata]

@betodealmeida ,

I've tested your changes and the issue is still occurring. It seems there are two different places where all catalogs are iterated. Once is in sync_permissions, but the other happens here.

This gets invoked on the CreateDatabaseCommand.

A similar change is required there. Is there any harm in moving the logic to return only the default catalog into the underlying get_all_catalog_names method(superset/models/core.py)?