Alpha162 commented on issue #13399: URL: https://github.com/apache/cloudstack/issues/13399#issuecomment-5317853992
> [@Alpha162](https://github.com/Alpha162) are you creating a PR for this? @DaanHoogland Yes, happy to, but with one caveat: I'm an operator rather than a CloudStack dev, so I'd want to stick to the schema fix and leave the other two to someone who knows the codebase properly. One convention question before I open it. To widen `cloud_usage.usage_volume`'s `UNIQUE KEY id` from `(volume_id, created)` to `(volume_id, created, vm_id)`, there seem to be three ways to do it and I'd rather ask than guess: 1. Call the existing `` `cloud`.`IDEMPOTENT_DROP_UNIQUE_KEY` `` / `` `cloud`.`IDEMPOTENT_ADD_UNIQUE_KEY` `` with a schema-qualified `'cloud_usage.usage_volume'`. They build their DDL from `in_table_name` via `CONCAT`, so this should work the same way `'cloud.counter'` does in `schema-42100to42200.sql`. 2. Add `usage.idempotent_drop_unique_key.sql` / `usage.idempotent_add_unique_key.sql` under `db/procedures/`, matching the existing `cloud_usage` helpers, currently that schema only has `IDEMPOTENT_ADD_COLUMN`, `IDEMPOTENT_CHANGE_COLUMN` and `IDEMPOTENT_DROP_INDEX`. 3. Use `` `cloud_usage`.`IDEMPOTENT_DROP_INDEX` `` plus a plain `ALTER TABLE ... ADD UNIQUE KEY`. Which fits the project's conventions best? One detail that's relevant to all three: the key is *named* `id` but covers `(volume_id, created)`. That's because `setup/db/create-schema-premium.sql` declares it unnamed on `(id, created)`, so MySQL auto-named it after the first column, and when #5785 introduced auto-increment primary keys and the old `id` became `volume_id`, the index kept its generated name. So any `DROP KEY 'id'` is relying on an auto-generated name rather than a deliberate one. Happy to be told that's fine, just flagging it. Also happy to be told which branch to target, `main` already has `schema-42210to42300.sql`, but `4.22` has nothing after `schema-42200to42210.sql`. On the other two, the `finally` in `GenericDaoBase.persist()` and bounding the rewind in `parse()`, I'd rather not be the one touching `GenericDaoBase` given how much depends on it. I should also be straight with you that we're rebuilding the affected environment imminently to an earlier version, so I can't offer it as a test bed. Hopefully the analysis above should be enough to reproduce from scratch. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
