-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: enable checkpoints for SQL files #7311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Removes *.sql from the excluded file patterns to allow checkpoint functionality for SQL script files. This fixes an issue where users could not use diff or restore features with .sql files. Fixes #7310
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in production - technically possible but morally questionable.
| "*.pdb", | ||
| "*.rdb", | ||
| "*.sql", | ||
| "*.sqlite", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good fix! Removing SQL files from exclusions makes sense since they're text-based scripts users actively edit. However, two considerations:
-
Large SQL dumps: Some SQL files can be multi-gigabyte database dumps. Users should add these to .gitignore if needed.
-
CSV consistency: I noticed CSV files (*.csv) are still excluded in this list, but they're also text files users might edit. Should we consider removing CSV from exclusions for consistency?
-
Documentation: Consider adding a comment above getDatabaseFilePatterns() explaining it excludes binary database files but not SQL scripts.
|
After reviewing this change, I need to reject it. SQL files were excluded from checkpoints for good reason - they can become extremely large (database dumps, migration scripts with seed data, etc.) and would significantly impact checkpoint performance. The exclusion pattern exists to protect the checkpoint system from these potentially massive files. The current behavior is intentional and should be maintained. I'll close this PR and provide more context on the issue. |
|
Closing this PR as SQL files should remain excluded from checkpoints due to performance considerations. |
Description
This PR attempts to address Issue #7310. Feedback and guidance are welcome.
Problem
Users reported that checkpoint functionality (diff/restore) was disabled for
.sqlfiles. When they changed the file extension to.py, checkpoints worked correctly.Solution
Removed
*.sqlfrom the database file exclusion patterns in the checkpoint service. SQL script files that users are actively working on should be eligible for checkpoint operations.Changes
*.sqlfromgetDatabaseFilePatterns()insrc/services/checkpoints/excludes.tsTesting
Impact
Users will now be able to:
.sqlfilesFixes #7310
Important
Enables checkpoint functionality for
.sqlfiles by removing them from exclusion patterns, fixing issue #7310..sqlfiles by removing*.sqlfrom exclusion patterns ingetDatabaseFilePatterns()inexcludes.ts..sqlfiles.excludes.spec.tsto verify.sqlfiles are not excluded from checkpoints..sqlfiles are included in checkpoint operations..sqlfile behavior with other code files for checkpoint operations.This description was created by
for 71e93f3. You can customize this summary. It will automatically update as commits are pushed.