-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Partially revert 'Remove N+1 in sync' #38549
Conversation
@@ -15,8 +15,7 @@ | |||
[database :- (mi/InstanceOf :model/Database)] | |||
(let [driver (driver.u/database->driver database)] | |||
(when (and (not= :redshift driver) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove the redshift exclusion now and the comment below, so this is just
(when (driver/database-supports? driver :table-privileges database) ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally updated the comment and left this so we didn't unintentionally enable it if we roll the other changes forward again.
Are we sure we want to revert this for PG and redshift too? see my comment |
|
Co-authored-by: Cal Herries <39073188+calherries@users.noreply.github.com>
We have decided against this approach, as it would revert much needed Redshift sync optimization, and we are not aware of any defects in its table-privileges implementation. |
Refences:
Description
This PR reverts some components of #37439
metabase.driver.sql-jdbc.sync.interface/active-tables
which added a new argument. We will need to maintain backwards compatibility for 3 releases, so will need to introduce a new multi-method instead.:table-privileges
for Redshift for now, but leave the queries.