Skip to content

Fix bug making it possible to start a server with an inconsistent database schema - #349

Merged
CGodiksen merged 8 commits into
mainfrom
bug/schema-check-on-startup
Sep 9, 2025
Merged

Fix bug making it possible to start a server with an inconsistent database schema#349
CGodiksen merged 8 commits into
mainfrom
bug/schema-check-on-startup

Conversation

@CGodiksen

Copy link
Copy Markdown
Collaborator

Closes #347 by changing the start up process for server nodes to check that the tables are identical if they have the same name. Previously we only checked the name which meant it was possible to start a server node with a table that was inconsistent with the cluster database schema.

Note that we purposely wait to create any tables until we have checked the local tables to ensure we do not create any tables before we know the local database is valid.

Currently we just return an error if there are tables in the local database that does not exist in the remote database. When reviewing, please consider whether we should change it so we just drop the local table instead.

@CGodiksen CGodiksen self-assigned this Sep 7, 2025
@CGodiksen
CGodiksen requested a review from Copilot September 7, 2025 18:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a critical bug where servers could start with inconsistent database schemas by implementing comprehensive table validation during server startup. Previously, only table names were checked, allowing schema mismatches to go undetected.

  • Enhanced table validation to compare both normal table schemas and time series table metadata between local and remote data folders
  • Refactored startup logic to validate existing tables before creating any new ones
  • Added PartialEq implementations to enable metadata comparison

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
crates/modelardb_types/src/types.rs Added PartialEq derive to TimeSeriesTableMetadata for metadata comparison
crates/modelardb_server/src/manager.rs Implemented comprehensive table validation logic and refactored startup process

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread crates/modelardb_server/src/manager.rs
Comment thread crates/modelardb_server/src/manager.rs

@skejserjensen skejserjensen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we just return an error if there are tables in the local database that does not exist in the remote database. When reviewing, please consider whether we should change it so we just drop the local table instead.

I think returning an error is the best solution as it allows users to make a decision instead of automatically dropping their local tables which may contain data and automatically dropping tables which may contain data seems like it would make data loss too easy.

@chrthomsen

Copy link
Copy Markdown
Contributor

Currently we just return an error if there are tables in the local database that does not exist in the remote database. When reviewing, please consider whether we should change it so we just drop the local table instead.

I think returning an error is the best solution as it allows users to make a decision instead of automatically dropping their local tables which may contain data and automatically dropping tables which may contain data seems like it would make data loss too easy.

I agree

@CGodiksen
CGodiksen merged commit dacfdc5 into main Sep 9, 2025
4 checks passed
@CGodiksen
CGodiksen deleted the bug/schema-check-on-startup branch September 9, 2025 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handle schema inconsistencies between local and remote database on startup

4 participants