fix(schema): Add critical performance indexes to resolve create_namespace latency >30s - #3939
Conversation
…pace latency Add database schema v5 with performance-critical indexes that resolve create_namespace operation timeouts from 30+ seconds to under 2 seconds. Changes: - idx_grants_realm_grantee on grant_records(realm_id, grantee_id) - idx_grants_realm_securable on grant_records(realm_id, securable_id) - idx_entities_catalog_id_id on entities(catalog_id, id) These indexes eliminate sequential scans on grant_records table during permission checks and optimize bulk entity lookups with large IN clauses. Fixes apache#3685
dimas-b
left a comment
There was a problem hiding this comment.
Thanks for your contribution, @machov ! The change LGTM 👍
schema diff against v4 for reference:
19,21c19,21
< -- Changes from v2:
< -- * Added `events` table
< -- * Added `idempotency_records` table for REST idempotency
---
> -- Changes from v4:
> -- * Added performance-critical indexes for grant_records table to fix create_namespace latency (Issue #3685)
> -- * Added optimized index for entities bulk lookups
31c31
< VALUES ('version', 4)
---
> VALUES ('version', 5)
59a60,61
> -- Additional index for bulk entity lookups (Issue #3685)
> CREATE INDEX IF NOT EXISTS idx_entities_catalog_id_id ON entities (catalog_id, id);
98a101,107
>
> -- Performance-critical indexes for grant_records (Issue #3685)
> -- These indexes resolve create_namespace latency from 30+ seconds to under 2 seconds
> CREATE INDEX IF NOT EXISTS idx_grants_realm_grantee
> ON grant_records (realm_id, grantee_id);
> CREATE INDEX IF NOT EXISTS idx_grants_realm_securable
> ON grant_records (realm_id, securable_id);
Given that quite a few people are involved in JDBC persistence, let's give this PR a few extra days in review.
|
@machov : Do you rely on having this fix an a released version soon? Note: 1.4.0 is in the works ATM. |
|
Can you elaborate what else needs to be done? I see all tests passed |
|
@machov : The PR is good to merge from my POV, I merely wanted it to have some more time in review in case other interested people have opinions on the new indexes. The question about 1.4.0 was basically to check whether you need this fix in the 1.4.0 release or you're ok with merging it after 1.4.0. |
|
couple of feedbacks :
|
I'm personally fine with adding the new indexes to v4 DDL files. However, from a more rigorous perspective, it makes sense to version the schema every time there is a material change. This way, it is easier to track how the Polaris database is expected to behave... For example, we could (hypothetically) deny the expensive operations with a v4 schema. I do not mean to do that in current PR, just exposing options to consider 🙂 |
flyrain
left a comment
There was a problem hiding this comment.
Thanks for the change. Echo @singhpk234, we probably reuse the v4 as 1.4.0 isn't release yet.
|
Good point - I missed that v4 schema was added after 1.3.0. Let's update v4 in this PR and merge it before 1.4.0 then. Adding to milestone. |
|
Do we need a v5 (since this is “just” additional DDL)? Do we need to ship these indexes vs recommending in docs/code? Do we need this in 1.4? |
|
Hi @machov, I think we all agreed on introducing a new schema version for this change. Since 1.4.x has not been released yet, we could simply include this PR as part of the 1.4.0 release. That way there is no need for a backport, and all future releases will automatically include the indexes as well. WDYT? |
|
@machov : just for my own understanding: Do you use releases of do you deploy Polaris from |
|
No need to change the branch ATM, as the 1.4.0 release branch hasn't cut yet. Hi @adnanhemani, can we include this PR in the 1.4.0? |
|
@machov : I generally support your point about introducing schema v5 in this case. However, as @flyrain mentioned, the v4 schema has never been released. It was added after 1.3.0 and 1.4.0 is still in early stages (there's no branch for it ATM). All in all, I think in this particular case it is preferable to add the new indexes to the v4 schema and release them in 1.4.0 (soon). Downstream users who pull directly from |
|
Yes, I think this is reasonable to put in 1.4.0, given we are still waiting on some last license and notice stuff. And agreed with @dimas-b, @flyrain, and @singhpk234 - given that v4 was never in a public release, we should just merge the index changes onto that. I will hold the release until we are able to get this PR sorted (if possible within this week)! Thank you for your great work, @machov! |
|
Once we push this into the 1.4 I will update our instalation and re-enable one of the problematic catalogs that trigger the issue, but i suspect this will not be enough as we did applied those indexes and still the response time decreased dramatically but still was over 30s in some cases. Let's wait and test again after the release. @machov thanks for the fix. |
86769f8 to
ce2f27d
Compare
adnanhemani
left a comment
There was a problem hiding this comment.
LGTM, thank you @machov!
jbonofre
left a comment
There was a problem hiding this comment.
Overall correct, I just have a few questions about the index accurancy.
| COMMENT ON COLUMN grant_records.privilege_code IS 'privilege code'; | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_grants_realm_grantee | ||
| ON grant_records (realm_id, grantee_id); |
There was a problem hiding this comment.
The index should include grantee_catalog_id no ?
| CREATE INDEX IF NOT EXISTS idx_grants_realm_grantee | ||
| ON grant_records (realm_id, grantee_id); | ||
| CREATE INDEX IF NOT EXISTS idx_grants_realm_securable | ||
| ON grant_records (realm_id, securable_id); |
There was a problem hiding this comment.
This one seems already covered by the primary key index.
If we want this index, I think we should include securable_catalog_id, no ?
|
|
||
| -- TODO: create indexes based on all query pattern. | ||
| CREATE INDEX IF NOT EXISTS idx_entities ON entities (realm_id, catalog_id, id); | ||
| CREATE INDEX IF NOT EXISTS idx_entities_catalog_id_id ON entities (catalog_id, id); |
There was a problem hiding this comment.
Is it not already covered by idx_entities index ?
jbonofre
left a comment
There was a problem hiding this comment.
My previous comments are not blocker.
…hDB schemas The CockroachDB schema resources were forked from the PostgreSQL v4 schema in apache#3352. One week later, apache#3939 added three performance indexes to fix the realm-wide `grant_records` scan reported in apache#3685, but only to the H2 and PostgreSQL v4 schemas. apache#5086 then derived the v5 schemas from their v4 ancestors, carrying the omission forward. CockroachDB has therefore never declared them: idx_grants_realm_grantee ON grant_records (realm_id, grantee_id) idx_grants_realm_securable ON grant_records (realm_id, securable_id) idx_entities_catalog_id_id ON entities (catalog_id, id) This matters most for `idx_grants_realm_grantee`. The `grant_records` primary key leads with the securable columns, so `loadAllGrantRecordsOnGrantee` - issued on every resolver cache miss for a principal, principal role, or catalog role - has `realm_id` as its only usable key prefix and scans every grant record in the realm. On CockroachDB the primary key is also the physical storage and range split key, so that scan fans out across nodes. Diffing the two dialect files shows the change restores parity with the PostgreSQL schema the CockroachDB one was forked from: apart from the header and footer comments and the deliberate `INTEGER`/`INT` to `INT4` substitution, these three indexes were the only difference. `idx_locations` is byte-identical in both. No schema version bump: the indexes affect query planning only, every statement is `CREATE INDEX IF NOT EXISTS`, and no column, constraint, or query changes. This mirrors how apache#3939 itself landed - its commit message records that a proposed new schema version was moved into the existing file during review. `SchemaIndexParityTest` guards against this class of drift recurring. It compares only `(index name, table name)` pairs per schema version, across the dialects that ship that version. Column lists are deliberately not compared, because dialects legitimately differ - `idx_locations` is a partial index on PostgreSQL but a plain index on H2. Without the schema change the test fails on v4 parity, v5 parity, and the CockroachDB grant-record assertion, naming the three missing indexes. Because Polaris has no automated schema migrations, an existing CockroachDB database only picks these up when a new realm is bootstrapped, since bootstrapping is the only production path that runs the schema script. The CHANGELOG and the Relational JDBC metastore documentation carry the one-time `CREATE INDEX` statements for databases bootstrapped by an earlier release. Related to apache#3685
The CockroachDB schema resources were forked from the PostgreSQL v4 schema in apache#3352. apache#3939 then added three performance indexes to fix the realm-wide `grant_records` scan reported in apache#3685, but it was opened before the CockroachDB schema landed on main, so it only covered the H2 and PostgreSQL v4 schemas. The v5 schemas were later derived from their v4 ancestors, carrying the omission forward. CockroachDB has therefore never declared them: idx_grants_realm_grantee ON grant_records (realm_id, grantee_id) idx_grants_realm_securable ON grant_records (realm_id, securable_id) idx_entities_catalog_id_id ON entities (catalog_id, id) `idx_grants_realm_grantee` is the one that fixes an access path. After `realm_id`, the `grant_records` primary key continues with the securable columns, so `loadAllGrantRecordsOnGrantee` - issued on every resolver cache miss for a principal, principal role, or catalog role - has `realm_id` as its only usable key prefix and scans every grant record in the realm. On CockroachDB the primary key is also the physical storage and range split key, so that scan fans out across nodes. The other two are included so the dialects agree, not because a query needs them: `loadAllGrantRecordsOnSecurable` constrains a full primary key prefix, and `idx_entities` already leads with `realm_id` for the only query shaped like `idx_entities_catalog_id_id`. Diffing the two dialect files shows the change restores parity with the PostgreSQL schema the CockroachDB one was forked from: apart from comments, whitespace and the deliberate `INTEGER`/`INT` to `INT4` substitution, these three indexes were the only difference. No schema version bump. Schema v4 and v5 already declare these indexes for PostgreSQL and H2, so the CockroachDB files are brought in line with the version they already claim rather than a new version being introduced; a v6 would instead say the indexes are new, and leave CockroachDB v5 permanently short of its own contract. The trade-off is that v4 and v5 have both shipped, so a CockroachDB database bootstrapped by an earlier release reports the same version without these indexes. Because Polaris has no automated schema migrations, such a database only picks them up when a new realm is bootstrapped, since bootstrapping is the only production path that runs the schema script. The CHANGELOG and the Relational JDBC metastore documentation carry the one-time `CREATE INDEX` statements for that case. `SchemaIndexParityTest` guards against this class of drift recurring. It compares only `(index name, table name)` pairs per schema version, across the dialects that ship that version and declare standalone indexes. Column lists are deliberately not compared, so it proves the dialects agree on which indexes exist by name, not that they cover the same columns - today they do not, since `idx_locations` is declared on different columns on H2 than on PostgreSQL and CockroachDB. Related to apache#3685
Problem
Issue #3685 reports critical performance degradation where
create_namespaceAPI operations consistently timeout (>30s), causing cascading failures with 504 Gateway Timeouts.Root Cause Analysis
Database analysis revealed:
grant_recordsSolution
Added index to existing schema v4 with three performance-critical indexes:
idx_grants_realm_granteeongrant_records(realm_id, grantee_id)idx_grants_realm_securableongrant_records(realm_id, securable_id)idx_entities_catalog_id_idonentities(catalog_id, id)Performance Impact
Based on issue reporter's testing:
Database Compatibility
IF NOT EXISTSfor safe deploymentFixes #3685