DOCS(database): Add some crude high-level docs for DB #6928
Conversation
WalkthroughAdds docs/dev/ServerDatabase.md describing the server database architecture and operation. It details a two-layer design: generic wrappers in src/database and server-specific code in src/murmur/database, using SOCI for backend abstraction. It specifies transaction semantics (every READ/WRITE within a transaction), ensureTransaction in Database and Table, and a TransactionHolder. It documents exception wrapping via std::throw\_with\_nested. It explains migrations on startup based on DB\_SCHEME\_VERSION, table re-creation, and data migration from tables suffixed with Database::OLD\_TABLE\_SUFFIX. Guidance on changing the scheme and referencing BanTable.cpp is included. No public API changes. Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
docs/dev/ServerDatabase.md (8)
5-8: Tighten wording; minor grammar fixes.
- “managing the database convenient” → “managing the database conveniently”
- “encodes the exact identity of tables” is a bit vague; “defines the concrete tables and access methods” reads clearer.
Apply this diff:
-The database implementation consists of two separate parts. First, the general DB wrappers that allow managing the database convenient from within C++ -(mostly) without having to worry about the exact backend that is used. Second, the server-specific implementation which encodes the exact identity of +The database implementation consists of two separate parts. First, the general DB wrappers that allow managing the database conveniently from within C++ +(mostly) without having to worry about the exact backend that is used. Second, the server-specific implementation which defines the concrete identity of tables and ways to access them. The former implementation lives under `src/database` and is completely decoupled from the server and even mostly decoupled from Mumble as a whole. The latter lives under `src/murmur/database`.
10-10: List supported DB backends and any caveats.Please enumerate which SOCI backends are officially supported (e.g., SQLite, PostgreSQL, MySQL/MariaDB) and note any backend-specific limitations (e.g., SQL dialect, index/constraint differences). This will help contributors reason about portability.
15-21: Clarify transaction semantics (isolation level, nesting, commit/rollback owner).
- “Every READ and WRITE is encapsulated by a transaction” can be misread as guaranteeing atomicity for reads; actual guarantees depend on the isolation level.
- Please state the default isolation level per backend (or that the implementation relies on backend defaults).
- Document whether nested transactions are emulated via savepoints or simply reuse the outer transaction, and who is responsible for commit/rollback when
ensureTransactionis used.Proposed additions after Line 21:
+Note on isolation: The guarantees for reads depend on the database's isolation level (e.g., READ COMMITTED, REPEATABLE READ). We do not override the +backend default isolation level; contributors must consider backend behavior when relying on snapshot semantics. + +Nesting: `ensureTransaction` reuses an existing transaction if one is active; it does not create a nested transaction or savepoint. The owner that +created the outer transaction remains responsible for commit/rollback. Call sites that only ensured a transaction must not call `commit()` explicitly.
26-29: Add a short example for exception wrapping.A minimal code example improves consistency across contributions and shows the intended exception type mapping.
Suggested insertion after Line 29:
+Example: + +```cpp +try { + soci::rowset<soci::row> rs = (sql.prepare << "SELECT ..."); + // ... +} catch (const soci::soci_error &e) { + std::throw_with_nested(DatabaseError("DB operation failed")); +} +```
33-36: Migration process details: transaction scope, downtime, and safety.Re-creating all tables can imply locks/downtime and large transactions. Please document:
- Whether the migration runs inside a single transaction per backend, or per table.
- Expected impact on running servers (downtime) and recommended backup guidance.
- Any data-volume thresholds that suggest an offline/maintenance window.
38-41: Use “schema” in prose; keep “SCHEME” only where it matches identifiers.“Database scheme” should be “database schema” in English prose. Keep
DB_SCHEME_VERSIONas-is to match code.Apply this diff (and similarly throughout the doc where “scheme” refers to the layout, not the constant):
-## Changing the database scheme +## Changing the database schema @@ -The database scheme is a monotonically increasing number that acts as a version number for the database layout. +The database schema version is a monotonically increasing number that acts as a version number for the database layout.
45-46: Prefer stable links over commit hashes where possible.Linking to a specific commit is fine, but consider linking to a stable path (e.g., main branch permalink or adding context that the line number may drift).
49-53: Minor grammar fixes in migration instructions.Tweak plurals and phrasing for clarity.
Apply this diff:
- new table, without any required type transformations, it is sufficient to call the base implementation `Table::migrate` which will take care of - that. When implementing these function explicitly, it is important that while columns names in the new table should use the constants defined in - the `column` namespace, the names of the columns in the old table should always be written out **explicitly**. This is to ensure that even if the + new table, without any required type transformations, it is sufficient to call the base implementation `Table::migrate`, which will take care of + that. When implementing these functions explicitly, it is important that while column names in the new table should use the constants defined in + the `column` namespace, the names of the columns in the old table should always be written out **explicitly**. This is to ensure that even if the names change in newer versions of the table (i.e. the ones in the `column` namespace change), the migration code still works as expected. For an
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/dev/ServerDatabase.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build (ubuntu-22.04, shared, 64bit)
- GitHub Check: build (ubuntu-24.04, shared, 64bit)
- GitHub Check: CodeQL-Build (ubuntu-latest)
- GitHub Check: PR (Docs)
- GitHub Check: PR (Windows_x64)
- GitHub Check: PR (Linux)
- GitHub Check: PR (Translations)
- GitHub Check: PR (macOS)
- GitHub Check: pr-checks (ubuntu-latest)
- GitHub Check: freebsd
Krzmbrzl
force-pushed
the
docs-change-database
branch
from
a686089 to
0acc092
Compare
| Any changes to the database layout needs to be accompanied by the following steps: | ||
|
|
||
| 1. Increase the scheme version by one. The scheme version is defined by the constant `ServerDatabase::DB_SCHEME_VERSION` (see | ||
| [here](https://github.com/mumble-voip/mumble/blob/4ac51e86ff7b2243774d86a4d9fdac548127389a/src/murmur/database/ServerDatabase.h#L40). |
There was a problem hiding this comment.
Maybe instead of perma-linking to a specific version of that file and line, just link to the file on master instead without line? Could otherwise be confusing in the medium distant future.
There was a problem hiding this comment.
Yeah, I wondered that as well but with the perma-link, I can at least be sure that the user sees what I want them to see even if the respective code has been moved somewhere else by the time the user is clicking on the link. In case of non-permanent links, that scenario could lead the user to a file that doesn't do what they came to witness 🤔
There was a problem hiding this comment.
Well I think in that case the docs should be update, but it's just a small issue anyway. So feel free to ignore
|
TODO note for myself: The code (as well as the docs) use the term |
2 participants
Checks