Remove N+1 in sync #37439
Merged
Remove N+1 in sync #37439
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
qnkhuat
added
the
backport
|
…es won't be returned
qnkhuat
commented
Jan 15, 2024
| @@ -353,6 +353,27 @@ | |||
| (testing "normally, ::fake-schema should be filtered out (because it does not exist)" | |||
| (is (not (contains? (schemas) fake-schema-name))))))))))))) | |||
|
|
|||
| (deftest sync-materialized-views-test | |||
There was a problem hiding this comment.
added this as I noticed we didnt' have one
qnkhuat
commented
Jan 15, 2024
| @@ -107,44 +106,77 @@ | |||
| :schema (.getString rset "TABLE_SCHEM") | |||
| :description (when-let [remarks (.getString rset "REMARKS")] | |||
| (when-not (str/blank? remarks) | |||
| remarks))})))))) | |||
| remarks)) | |||
| :type (.getString rset "TABLE_TYPE")})))))) | |||
There was a problem hiding this comment.
current-user-table-privileges does not return privileges for external tables on redshift, and foreign table on postgres, so we need to use the select method on them, so to check select privilege for these tables we need to use the old method of trying a select query. In order to do that we need to return the type so we know what's the type of table we're dealing with.
see line 134.
qnkhuat
commented
Jan 15, 2024
| (let [schema-filter-prop (driver.u/find-schema-filters-prop driver) | ||
| database (db-or-id-or-spec->database db-or-id-or-spec)] | ||
| (if (some? schema-filter-prop) | ||
| (let [prop-nm (:name schema-filter-prop) |
There was a problem hiding this comment.
I've removed the branch were database is nil. doesn't make any sense to call describe-database on a database does not exist.
There was a problem hiding this comment.
Did you check that all the callers of driver/describe-database don't break because of this change? I'm wondering why the code was written like this in the first place
There was a problem hiding this comment.
I think it was designed so that it can take a spec as argument. I don't see anywhere that's used in production, only in tests.
but to be safe I will revert this change and follow up on this later. don't want another PR to have unnesscary change then turns out it's nessceary like #37620 :)
qnkhuat
commented
Jan 15, 2024
There was a problem hiding this comment.
we already have tests for syncing view and foreign table in this namespace.
qnkhuat
commented
Jan 15, 2024
calherries
reviewed
Jan 15, 2024
calherries
approved these changes
Jan 15, 2024
| (let [schema-filter-prop (driver.u/find-schema-filters-prop driver) | ||
| database (db-or-id-or-spec->database db-or-id-or-spec)] | ||
| (if (some? schema-filter-prop) | ||
| (let [prop-nm (:name schema-filter-prop) |
There was a problem hiding this comment.
Did you check that all the callers of driver/describe-database don't break because of this change? I'm wondering why the code was written like this in the first place
test/metabase/sync/sync_metadata/sync_table_privileges_test.clj
Outdated
Show resolved
Hide resolved
| (when-not (.getAutoCommit conn) | ||
| (.rollback conn)) | ||
| false)))) | ||
| (execute-select-probe-query driver conn sql-args) |
There was a problem hiding this comment.
Indentation was fine before
metamben
reviewed
Jan 15, 2024
calherries
reviewed
Jan 16, 2024
qnkhuat
commented
Jan 16, 2024
| table-names (->> (jdbc/query conn-spec "SHOW TABLES" {:as-arrays? true}) | ||
| (drop 1) | ||
| (map first))] | ||
| (for [[table-name privileges] (table-names->privileges (privilege-grants-for-user conn-spec "CURRENT_USER()") | ||
| (:name database) | ||
| db-name |
There was a problem hiding this comment.
@calherries, This was a bug that we didn't catch in our BE tests.
we should use the db from details, not the display name. I only catch it because the e2e tests use a different display name on its test database.
see my change in test/metabase/driver/mysql_test.clj to make sure we covered this in tests.
|
@qnkhuat Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
This was referenced Feb 8, 2024
qnkhuat
added a commit
that referenced
this pull request
Feb 19, 2024
using the resutl from `driver/current-user-table-privileges` method. But we see some users reported tables are disabled during sync, while I haven't found out the root cause of that, I think we should added a fallback to using the old privilege check if the new method return `false`. This way we still get the performance boost but is safer when introducing this new method. The only downside is it's hard to know why the new method give false positive results. With this we either rely on someone see our warn log and log an issue, or maybe we'll collect some telemetry using snowplow.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #37250
During sync metadata process, we need to check whether or not we have select permissions against a table.
Currently we do that by making a
select true from table limit 1on the table. This is inefficient if we have a lot of tables.In this PR, I've changed so that we rely on the information from
driver/current-user-table-privilegesto do the check.This method was added recently in #32978.
Currently, it only supports Postgres,redshift, and MySQL, so only those drivers will benefit from this. We can later add support for more drivers by implementing that method.
Technical note: I've looked into using the getTablePrivileges from JDBC, but I decided to not using them because it does not return usage grant. So if someone has select privilege but not usage privilege, we still think they can select, but in reality, they can't.