fix(copy): respect auto_checkpoint setting on COPY FROM (#755) - #763
Open
adsharma wants to merge 1 commit into
Open
fix(copy): respect auto_checkpoint setting on COPY FROM (#755)#763adsharma wants to merge 1 commit into
adsharma wants to merge 1 commit into
Conversation
Previously, COPY FROM unconditionally forced a checkpoint regardless of the auto_checkpoint setting. This change adds a guard so that setForceCheckpoint() is only called when autoCheckpoint is true. When auto_checkpoint=false, COPY FROM still succeeds but no longer triggers an automatic checkpoint, allowing the user to control checkpointing behavior via the existing configuration knobs. Test: copy_no_auto_checkpoint.test validates both cases: - auto_checkpoint=false: COPY succeeds, setting preserved - auto_checkpoint=true (default): COPY succeeds
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.
Fixes #755
COPY FROMunconditionally forced a checkpoint, ignoringDBConfig::autoCheckpoint(theCALL auto_checkpoint=...setting) andDBConfig::checkpointThreshold. The forced branch intransaction_manager.cpp:169ran first, so the auto-checkpoint branch never got a chance. The result was a large per-statement cost on everyCOPYthat the user could not turn off.Change (
src/main/client_context.cpp, 1-line guard):getDBConfig()is already in scope;autoCheckpointis the boolean behind theCALL auto_checkpoint=...setting. The default behaviour is unchanged. Whenauto_checkpoint=false,COPYstill succeeds but no longer triggers a checkpoint, so the data lives in the WAL until the next auto-checkpoint or a manualCALL db_checkpoint();.Test (
test/test_files/transaction/copy_no_auto_checkpoint.test):CopyNoAutoCheckpointSmokeTest—CALL auto_checkpoint=false;thenCOPY foo FROM (UNWIND ...), asserts success and that the setting is preserved. This is enough to detect a regression of the original bug.CopyWithDefaultAutoCheckpoint— defaultauto_checkpoint=true,COPYsucceeds.Test would benefit from a stronger WAL-visibility assertion once a
wal_replay_info-style public API is available; none exists today.Test results: new tests pass; all 67 transaction tests continue to pass.
Perf note: a best-effort
perfprofile against the tinysnbCOPYtest shows the runtime is dominated by CSV parsing (regex_traits::transform,collate::do_transform, ~77%), not the COPY kernel. The dataset is too small to draw conclusions about the non-checkpointCOPYoverhead. A larger benchmark would be the right next step for the part-2 measurement in the issue.