Repository navigation
Keep the data through a table rebuild, and leave a table that cannot be rebuilt as it was [all-db] - #516
Merged
Conversation
…be rebuilt as it was [all-db] A rebuild is meant to keep what the table holds. Three things contradicted that: - SQL Server, PostgreSQL, MySQL and Oracle left AutoIncrement columns out of the copy, so the rebuilt table numbered its rows again. Every dialect now copies them: SQL Server with IDENTITY_INSERT and a reseed past the old last value, PostgreSQL with setval past the old sequence, Oracle by restarting the identity after it. MySQL keeps the values and its counter carries on after the highest one, since a reseed would need a user variable the connection does not allow. - Indexes only the database had were dropped with the old table. They are now created again on the rebuilt table once it has its final name (RebuildSchemaFactory.GetDatabaseOnlyIndexes). - A column of a type the framework does not map made every rebuild of its table throw part way. The plan now refuses such a rebuild before any statement runs, naming the column, and the table stays as it was. RebuildPreservationDbTests checks on every database that a rebuild keeps the rows, their numbers and a database-only index, and that the next row does not take a number a delete freed; that a rebuild whose copy fails on the existing data leaves the table and its rows unchanged, twice; and, on SQLite, the refusal for an unmapped column. .claude/rules/database.md now states the table upgrade principles: keep the data, upgrade to the current TableSchema, never need a second upgrade, and stop and report when the data blocks the upgrade. Closes #513
…ks an upgrade The schema upgrade guide said what an upgrade does and how a failing ALTER stage rolls back, but not what an upgrade promises about the data, nor where a failure is recorded. It now has: - a section on the upgrade principles: keep the existing data, upgrade to the current TableSchema, need no second upgrade, and stop and report when the data blocks the upgrade; - a section on a failed upgrade: the table stays as it was (including the temporary table MySQL and Oracle may leave), a column the rebuild cannot recreate is refused up front, and where the error is recorded (db upgrade's per-table line and exit code, TableUpgradeReport, or the exception of a single-table upgrade), with the steps to resolve it. The rebuild section points to it instead of repeating it. The zh-TW translation follows.
…new AutoIncrement afresh Review of the previous commit: - Recreating an index only the database has was not faithful. The providers read back its columns but not its filter, included columns, prefix lengths or kind, so a unique index could come back enforcing more or less than before, and a recreate that failed ran after the old table was dropped (MySQL, Oracle), leaving a half-done upgrade. A SQLite UNIQUE column constraint read back as a reserved sqlite_autoindex name that could not be created at all. The plan now refuses such a rebuild before anything runs, naming the indexes, so a person declares them in the definition or drops them (RebuildSchemaFactory.ThrowIfRebuildWouldLoseParts). - A plain column that becomes AutoIncrement copied its old values, often all 0, into an identity column. Such a column is now left out of the copy and numbered afresh, as before (RebuildSchemaFactory.GetUncopiedFieldNames). RebuildPreservationDbTests checks the refusal on every database, with the table, its index and its rows left as they were. The guide and the rule describe both.
jeff377
enabled auto-merge
November 15, 2026 05:10
SonarCloud S3267 on the pull request: the loops in DdlFragments.BuildInsertSelectStatement and RebuildSchemaFactory.GetUncopiedFieldNames only filter and map, so they read as Where/Select. No behaviour changes.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #513
Principles
This pull request makes the rebuild follow the table upgrade principles. They are now stated in the schema upgrade guide and in
.claude/rules/database.md:TableSchemais the target.What changes
The rebuild keeps AutoIncrement numbers on every database
SQL Server, PostgreSQL, MySQL and Oracle left AutoIncrement columns out of the copy, so the rebuilt table numbered its rows again. Every dialect now copies the values. The table below shows how each one continues the sequence afterwards:
sqlite_sequencecarried over (from #514)SET IDENTITY_INSERTsys.identity_columns.last_valuethrough dynamicDBCC CHECKIDENTGENERATED BY DEFAULT)setvalpast the old sequence's last valueGENERATED BY DEFAULT)LAST_NUMBER; a few cached numbers may be skippedA plain column that becomes AutoIncrement is not copied. Its rows are numbered afresh, as before, since old values (often all
0) cannot serve as identity values.A rebuild that would lose part of the table is refused before anything runs
Two cases are refused at plan time:
In both cases the error names the index or column and the table is left as it was. A person then declares them in the definition or drops them, and upgrades again (
RebuildSchemaFactory.ThrowIfRebuildWouldLoseParts).Documentation
database-schema-upgrade.md(en, zh-TW) gains two sections:tmp_table on MySQL and Oracle), where the error is recorded (db upgrade's per-table line and exit code,TableUpgradeReport, the exception of a single-table upgrade), and the steps to resolve it.Tests
RebuildPreservationDbTestsruns on every database. It checks:New unit tests cover the plain-to-AutoIncrement copy and the index refusal.
Review
An adversarial review of the first commit found the problems with recreating database-only indexes:
INCLUDEunique indexes changing meaning;sqlite_autoindex_*names.It also found the copy of a plain column into a new AutoIncrement. The third commit replaces the recreation with the refusal and fixes the copy.
Verification
Run locally:
dotnet build Polhem.slnx -c Releaseis clean.Polhem.Db.UnitTestspasses on SQL Server, PostgreSQL, MySQL, Oracle and SQLite (1677 tests), after the rebase onto Gate the table upgrade on idempotency and fix the loops it finds [all-db] #515's merge../test.shbefore the rebase failed only:Generate_iOS_Builds, because the local Xcode does not match the iOS workload;