Schema-level locking for multiuser scenarios #8724
Conversation
…tId and add additive schema change lock functionality
…ma changes and update tests to verify lock behavior
…d await property retrieval for user properties
…hods to schema table lock for clarity
…for trivial and data transformation schema updates
…onsistency and clarity; update element properties in multiple tests
…schema transformation
…/itwinjs-core into rschili/schema-level-locks
- Improved schema locking logic in IModelDb to handle schema imports more effectively with table locks. - Added configuration option for permissive schema table locks in IModelHost. - Updated schema lock tests to remove focus and skip tests that are not yet supported.
…/itwinjs-core into rschili/schema-level-locks
rschili
changed the title
| } | ||
| }; | ||
|
|
||
| function isDataTransformRequiredError(error: unknown): error is ITwinError { |
There was a problem hiding this comment.
@khanaffan @ColinKerr
This feels messy. I cannot use ITwinError typesafe as shown below. We rethrow a new error sometimes which only preserves the "errorNumber" field which was accessed on :any.
Using ITwinError.isError with a string seems like a better solution, or maybe having an overload that takes an error number...
I would prefer some standard way for doing this.
There was a problem hiding this comment.
we should stop using ErrorNumber everywhere. Throw an iTwinError from native code.
There was a problem hiding this comment.
We have inconsistencies across the stack, we need to document best practices and code so we are consistent. Created a new issue to track larger issue and linked in one other existing issue on same topic. #8774
@rschili please add this inconsistency to the larger issue and link it here before you resolve this.
…bles and change tracking information
| @@ -0,0 +1,131 @@ | |||
| # iModel File Format | |||
There was a problem hiding this comment.
@ColinKerr @khanaffan Please review this thoroughly.
I did not write all of this by hand, instead I had copilot help with it, though, it produced a ton of nonsense which I then rewrote/removed.
It's supposed to be a start, not a complete documentation. We didn't have anything existing to begin with so we can add to this as we go.
|
This pull request is now in conflicts. Could you fix it @rschili? 🙏 |
|
I don't see how you can ever perform a schema upgrade that requires data changes without first acquiring the schema lock. How will that work? |
@kabentley the current plan for 2026 is to introduce a high level version of changesets which is not tied to the binary state of an imodel but instead contains a higher level representation of the changes, so when applying, each briefcase can replay the changes and make necessary data changes. We'll also explore ways to store data transformations happening during schema updates, and then replay them during rebase, which may turn out not feasible. At first glance this seems quite messy, but it's much less work. |
I'd have to have this explained in detail, but it sounds extremely complicated and unnecessary. Schema changes that require data changes should be rare and happen only by admins. I strongly suggest you make them only while the project is "offline". Anything else will be a risky mess. For example, any schema change that remaps/removes/changes properties will undoubtedly require new versions of all apps that can use them. It will be impossible for a user to "pull changes" in an active session that require new versions of the very apps that made their local changes they need to rebase. That is why major database upgrades happen with the system offline. People expect that, it's not an undue burden, and it can't possibly rise to a high enough priority to work on given all the other real problems we need to address. |
…ify locking necessity
The current change allows us to make minor schema changes without taking a full db lock while also ensuring those schema changes are done in tandem with data upgrades to the data in a channel. This simplifies channel upgrades that include both application schema and related data changes. These changes happen regularly as the application is updated, they may be deferred but are necessary to use new application features. In 2026 we hope to investigate supporting minor schema changes that result in table mapping changes without a full db lock. This changes are minor and compatible changes from a schema point of view but result in changes to the internal table mapping used. Major schema changes would still require a full db lock. |
| /** The Id of the dictionary model. */ | ||
| public static readonly dictionaryId: Id64String = "0x10"; | ||
| /** The Id representing the schema element for locking. */ | ||
| public static readonly schemaElementId: Id64String = "0x2"; |
There was a problem hiding this comment.
If I understand correctly, this refers to a non-existent element. Your PR description claims this is a short-term solution. Will the long-term solution also involve the use of pretend elements?
If no, why make this element Id part of the public API?
If yes, does everybody who wants to interact with locks now need to properly handle locked "elements" that don't refer to real elements? (simple example: a UI displaying the name/guid/code/whatever of all of the elements the briefcase has locked).
There was a problem hiding this comment.
Good point.
I'll flag this as internal for now.
We don't know about the long-term solution yet, so I can't say if it will involve further use of that mechanism.
Whether to show this in a UI... I'm not sure, that is a good question. I'll bring it up in the next multi-user meeting.
Edit: flagging done
There was a problem hiding this comment.
Whether to show this in a UI
I'm not specifically asking about UI. I'm saying that this new "element" breaks all kinds of assumptions that all pre-existing code has been written against. e.g., that Ids refer to real elements; that all element Ids except the root subject have a parent element Id; etc. If this fake element Id starts showing up in such contexts it may break things.
Did the BIS folks discuss this, and whether or not there may be a case for using an actual element instead of a pretend one?
There was a problem hiding this comment.
Please listen to Paul's advice. The "lock" you need isn't an element lock at all - it's a lock on the ClouldSqlite schemaDb, right? Why use element based "locks: at all? Presuming you can use a new static elementId is confusing, risky and unnecessary.
|
This pull request is now in conflicts. Could you fix it @rschili? 🙏 |
As part of the effort for making schema merges less disruptive to OS+, described here: https://github.com/iTwin/itwinjs-backlog/issues/1703
We want to make importing schemas not require a full exclusive lock over the entire imodel. We will introduce a new schema-level lock instead.
Using this lock, we prevent multiple people from importing schemas, but if a schema import is not affecting mappings (does not move data around), we want to allow it while people keep making data changes to their imodels.
The goal is to first attempt the import with the schema-level lock, and if moving data is required, the attempt will fail, itwin-js will then obtain the full exclusive lock as it did before and re-attempt the import.
This isn't ideal, but we currently have no good way to tell if a schema import will require data changes as it depends on many different factors (type of schema edits, current mappings, existing data, table size limits, available columns...)
This is a short term solution timeframed around end of december 2025.
Closes: https://github.com/iTwin/itwinjs-backlog/issues/1737