Improve database management documentation #593

withinfocus · 2025-05-21T21:55:16Z

🎟️ Tracking

https://bitwarden.slack.com/archives/C065AUDER62/p1747770311208779

📔 Objective

Improves documentation given a first pass at #165 as well as some corrections started at #592. We can organize and separate EF and MSSQL better as well as include some additional tips and gotchas regularly-asked in chat. Additionally, "manual" migrations are not currently in place and may never be desired at this point so remove that content.

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

cloudflare-workers-and-pages · 2025-05-21T21:55:17Z

Deploying contributing-docs with Cloudflare Pages

Latest commit:	`abf861d`
Status:	✅ Deploy successful!
Preview URL:	https://f942c397.contributing-docs.pages.dev
Branch Preview URL:	https://db-docs.contributing-docs.pages.dev

View logs

github-actions · 2025-05-21T21:57:44Z

Checkmarx One – Scan Summary & Details – d385e846-8dd4-4649-b600-20123b4ca53f

Great job, no security vulnerabilities found in this Pull Request

audreyality · 2025-05-21T22:16:11Z

docs/contributing/database-migrations/mssql.md

+  before falling back to the old location
+- continued updating of the old data columns since in case of a rollback no data should be lost
+
+### Non-backwards compatible migration


🤔 I think we should use affirmative language here. The 'non-backwards compatible migration' is talking about a 'breaking change'.

Suggested change

### Non-backwards compatible migration

### Breaking change migration

I like this, but let's make sure to update everywhere, not just this heading

I support the current term. The point of this documentation is that your change can't go backwards. It may be "breaking" to some, but the intention of the documentation is about direction.

We can use affirmative language to describe that. The EDD page uses "finalization migration" as its term for this phase.

Suggested change

### Non-backwards compatible migration

### Finalization migration

audreyality · 2025-05-21T22:18:59Z

docs/contributing/database-migrations/mssql.md

+
+## SQL database project
+
+The separate database definitions in `src/Sql/.../dbo` serve as a "master" reference for the


⛏️ No need for scare quotes here.

Suggested change

The separate database definitions in `src/Sql/.../dbo` serve as a "master" reference for the

The separate database definitions in `src/Sql/.../dbo` serve as the source of truth for the

It's an analogy to a master copy of something, like an album recording. This content just moved its position by the way.

Yes, I know. "The source of truth" is both more inclusive than "master reference", and more accurate as a description.

audreyality · 2025-05-21T22:21:04Z

docs/contributing/database-migrations/mssql.md

+required. With the use of beta testing, partial roll-outs, [feature flags](../feature-flags.md),
+etc. the often-chosen path is to spread a change across several major releases with a calculated
+future state that can perform a "cleanup" migration that is backwards-compatible but still
+represents an overall-_incompatible_ change beyond the boundaries of what we need for individual


⛏️ Our communication guidelines indicate we should avoid italics and prefer bold because international fonts don't render italics well.

Suggested change

represents an overall-_incompatible_ change beyond the boundaries of what we need for individual

represents an overall-**incompatible** change beyond the boundaries of what we need for individual

Can you link me to that? I honestly don't agree and there's a difference between bold and italic in intention. We use italics all the time on this site.

It's in the Technical Writers Style Guide.

audreyality · 2025-05-21T22:21:56Z

docs/contributing/database-migrations/mssql.md

+Please take care to ensure:
+
+- any existing stored procedure accepts the same input parameters and that new parameters have
+  nullable defaults
+- when a column is renamed the existing stored procedures first check (coalesce) the new location
+  before falling back to the old location
+- continued updating of the old data columns since in case of a rollback no data should be lost


👍🏻 Love that you're talking about specific concerns to keep aware of!

docs/contributing/database-migrations/mssql.md

docs/contributing/database-migrations/ef.md

docs/contributing/database-migrations/mssql.md

MGibson1 · 2025-05-21T23:16:05Z

docs/contributing/database-migrations/mssql.md

+required. With the use of beta testing, partial roll-outs, [feature flags](../feature-flags.md),
+etc. the often-chosen path is to spread a change across several major releases with a calculated
+future state that can perform a "cleanup" migration that is backwards-compatible but still
+represents an overall-_incompatible_ change beyond the boundaries of what we need for individual
+release safety.


I don't follow what you're going for with this sentence. If a migration is performed that leaves the DB in an incompatible state, it is by definition a finalization migration for the initial transitional migration. If the DB is always backwards compatible with the N-1th state, you can't build up migrations such that it's incompatible with the N-Mth.

Are you saying that our process is a cleanup so long after transitions that they are effectively unrelated and the finalization paradigm isn't apt?

It's not that they are unrelated -- they definitely are -- but, yes, the finalization need in this process is unnecessary. This is what we are actually doing 99% of the time here.

docs/contributing/database-migrations/mssql.md

MGibson1 · 2025-05-21T23:19:54Z

docs/contributing/database-migrations/mssql.md

+  before falling back to the old location
+- continued updating of the old data columns since in case of a rollback no data should be lost
+
+### Non-backwards compatible migration


I like this, but let's make sure to update everywhere, not just this heading

docs/contributing/database-migrations/mssql.md

eliykat · 2025-05-22T01:57:50Z

docs/contributing/database-migrations/mssql.md

+     compatible with the changes to `DbScripts`. In order to achieve this we only keep a single
+     migration, which executes all backwards incompatible schema changes.


Does this mean that DbScripts_finalization only ever contains 1 file that everyone appends to? I'm not necessarily opposed to the requirement but I don't recall seeing it before.

Is the finalization script added at the same time as the backwards-compatible change? So there is only 1 finalization script at a time, it is always moved to DbScripts after the next rc cut date? What if you need a longer timeframe on your finalization?

(not being overly critical - just trying to understand this process given we've never really done it)

Yes. It's always been mentioned.

You break up a pure DB non backwards compatible change into two steps. An example would be the column rename example in https://contributing.bitwarden.com/contributing/database-migrations/edd#migrations-1.

Initial migration would:

Add a new column

Add logic that keeps old and new in sync.

Transition migration would:

Migrate all data from old to new column.

Finalization migration (run at next deploy):

Removes the old now unused column.

Updates sprocs to remove the fallback logic.

Do note we technically never have to remove the old column but not doing so adds database debt since we have

Unnecessary data in our database.

More complex sprocs.

This is why we'd like to automate it since relying on human elements here is dangerous since I could easily be OOO for a release or two or forget about cleaning it up.

Finalization migration (run at next deploy):

This actually needs to be run 2 deploys away from the initial migration to maintain our self-host server guarantees. We can apply it earlier on our infrastructure.

Do note we technically never have to remove the old column...

Unless doing so would cross the the 8kb maximum record length or 1,024 column limit. I'd expect something will cross the 8kb maximum record length if we never remove columns. It'd be much harder to hit the column limit.

I could've been clearer in my question - my previous understanding was that you might have several scripts in DbScripts_finalization as different teams add different finalization scripts. But this way, you have 1 finalization script at a time, which all teams append to. It's been a while since I read about this process so just trying to get my head around it again.

That said, it is unusual that multiple teams would append to the same file so this might be worth stating more explicitly, e.g.

Add your migration to the file in DbScripts/finalization, or create the file if it doesn't exist. Name it YYYY-0M-FinalizationMigration.sql [what should this date be?]. There should only be 1 finalization file at a time.

Rather than "Write a new migration, put it here, name it this" but then saying "we only keep a single migration".

Or if I've misunderstood please let me know.

docs/contributing/database-migrations/mssql.md

Hinton

We absolutely do need dbo_finalization for any automation to work. dbo must at all time accurately represent the state that running all DbScripts results in. The workflow relies on an automation running which moves content from _finalization back just having DbScripts will result in the automation failing due to inconsitencies between dbo and DbScripts.

I've created #594 which rebases this on ps/migrations to make it more clear how they differ.

audreyality · 2025-05-22T12:37:09Z

docs/contributing/database-migrations/edd.mdx

🎨 One of these files should note how our database version policy intersects with our self-hosted server support policy. Linking from these docs to the help site improves discoverability.

Does it though? My concern is that changes the scope of all this -- everything here is about a single release, forward or backward.

The original scope of this document addressed the support policy by ensuring finalization can run at any future point after release. If you remove finalization, then you must talk about how we guarantee our self-hosted server maintains its N-2 release compatibility.

Either way, the pages should at minimum be cross-linked.

withinfocus · 2025-05-22T12:57:13Z

I addressed a few comments on the smaller things and left responses to the rest. I am gonna move over to server and finish the finalization work because there's been a strong reaction to keeping it (and not documenting how things are today, albeit incomplete to reintroduce later), and if that works out can restore the couple sentences about it as well as further bolster the "why".

withinfocus · 2025-05-22T19:32:51Z

Relates to bitwarden/server#5855 where I'm making the remaining EDD changes.

withinfocus · 2025-05-22T20:25:27Z

I have no more changes planned for this and open comments are either needing responses or something I am not currently comfortable changing further.

* confirm passwords match between .env and secrets.json * add tip to test database connectivity * move confirmation steps into tabs * align headers with #593; add link to migrations page

withinfocus mentioned this pull request May 21, 2025

Remove unused evolutionary database design processes #592

Closed

withinfocus force-pushed the db-docs branch from 717e06c to 4797be6 Compare May 21, 2025 21:59

Improve database management documentation

23249a8

withinfocus force-pushed the db-docs branch from 4797be6 to 23249a8 Compare May 21, 2025 22:03

withinfocus marked this pull request as ready for review May 21, 2025 22:07

withinfocus requested a review from a team as a code owner May 21, 2025 22:07

audreyality reviewed May 21, 2025

View reviewed changes

MGibson1 reviewed May 21, 2025

View reviewed changes

eliykat reviewed May 22, 2025

View reviewed changes

Hinton requested changes May 22, 2025

View reviewed changes

withinfocus mentioned this pull request May 22, 2025

Revive old migrations PR #165

Open

audreyality reviewed May 22, 2025

View reviewed changes

withinfocus added 3 commits May 22, 2025 08:40

Link to Getting Started subsection

9b228ca

Untwist description of change need

8dc9c0b

Don't say please

aa93e62

withinfocus marked this pull request as draft May 22, 2025 12:48

withinfocus added 3 commits May 22, 2025 15:47

Add folder names to EDD steps

03bd4c6

Restore reference to finalization folders since that's been completed

5955f6e

Additional content changes suggested from a prior PR

e8fb3e4

withinfocus mentioned this pull request May 23, 2025

confirm passwords match between .env and secrets.json #597

Merged

audreyality added a commit that referenced this pull request May 23, 2025

align headers with #593; add link to migrations page

b9d5797

withinfocus added 2 commits May 23, 2025 09:39

Merge remote-tracking branch 'origin' into db-docs

2e3ef06

Fix an oversight from #596

654bbe2

Merge remote-tracking branch 'origin' into db-docs

f1eea3a

withinfocus added 2 commits May 23, 2025 11:13

Fix bungled merge from the wrong ref

77ea379

Merge remote-tracking branch 'origin' into db-docs

abf861d

	### Non-backwards compatible migration
	### Breaking change migration

	### Non-backwards compatible migration
	### Finalization migration


		## SQL database project

		The separate database definitions in `src/Sql/.../dbo` serve as a "master" reference for the

	represents an overall-_incompatible_ change beyond the boundaries of what we need for individual
	represents an overall-incompatible change beyond the boundaries of what we need for individual

		compatible with the changes to `DbScripts`. In order to achieve this we only keep a single
		migration, which executes all backwards incompatible schema changes.

Improve database management documentation #593

Are you sure you want to change the base?

Improve database management documentation #593

Conversation

withinfocus commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

cloudflare-workers-and-pages bot commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying contributing-docs with Cloudflare Pages

Uh oh!

github-actions bot commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Great job, no security vulnerabilities found in this Pull Request

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eliykat May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hinton May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hinton May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

audreyality May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eliykat May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Hinton left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

withinfocus commented May 21, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented May 21, 2025 •

edited

Loading

github-actions bot commented May 21, 2025 •

edited

Loading

eliykat May 22, 2025 •

edited

Loading

Hinton May 22, 2025 •

edited

Loading

Hinton May 22, 2025 •

edited

Loading

audreyality May 22, 2025 •

edited

Loading

eliykat May 23, 2025 •

edited

Loading

Hinton left a comment •

edited

Loading

audreyality May 22, 2025 •

edited

Loading