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
Remove unreferenced steps from isolation tests #2487
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2487 +/- ##
==========================================
+ Coverage 90.07% 90.19% +0.12%
==========================================
Files 212 212
Lines 34198 34148 -50
==========================================
- Hits 30805 30801 -4
+ Misses 3393 3347 -46
Continue to review full report at Codecov.
|
{ | ||
LOCK _timescaledb_catalog.continuous_aggs_invalidation_threshold | ||
IN ACCESS EXCLUSIVE MODE; | ||
} | ||
step "L1_unlock_threshold_table" |
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.
If the lock step is removed, then we shouldn't need the unlock either.
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.
removing the unlock does alter the test and leads to different test output
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.
It seems like the intention was to have "begin" and "commit" steps separate, so wouldn't it be more reasonable to add them to the permutation (and fix the step that has an extra "begin")?
step "S1" { SELECT count(*) from ts_device_table; } | ||
step "SC1" { SELECT count(*) from _timescaledb_internal._hyper_1_1_chunk; } | ||
step "SH" { SELECT total_chunks, number_compressed_chunks from hypertable_compression_stats('ts_device_table'); } | ||
step "Sc" {COMMIT;} |
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.
If this is missing from the sequences below, is not that an error in itself?
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.
this PR only removes dead code, and an isolation test session is not required to be in a single transaction, to check progress in a session it's actually required not to run in transaction due to MVCC
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.
Well... yes, but why was the commit there to begin with if it was not needed?
Some isolation tests had steps that were not referenced in any of the permutations so this patch removes those.
Some isolation tests had steps that were not referenced in
any of the permutations so this patch removes those.