Remove parallel safe from partialize_agg #5172
Merged
Conversation
This file contains 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
Codecov Report
@@ Coverage Diff @@
## main #5172 +/- ##
==========================================
- Coverage 89.27% 89.27% -0.01%
==========================================
Files 225 225
Lines 51756 51755 -1
==========================================
- Hits 46206 46205 -1
Misses 5550 5550
Continue to review full report at Codecov.
|
fabriziomello
force-pushed
the
remove_parallel_safe_from_partialize_agg
branch
from
8de4184
to
beaebf6
Compare
|
@sb230132, @nikkhils: please review this pull request.
|
akuzm
reviewed
Jan 12, 2023
tsl/test/sql/partialize_finalize.sql
Outdated
| SET parallel_setup_cost = 0; | ||
|
|
||
| -- Compare the results (should return NO ROWS) | ||
| (SELECT |
There was a problem hiding this comment.
Slightly confused by this test -- which subselect is the broken one? Maybe we could generate stable pseudo-random data and just show the results of both subselects. For pseudo-random we can use something like this: https://github.com/timescale/timescaledb/blob/main/tsl/test/sql/dist_param.sql#L20-L33
There was a problem hiding this comment.
Also might be good to have an explain query so that we are absolutely sure it runs in parallel, because parallel execution is notoriously hard to achieve sometimes.
There was a problem hiding this comment.
You're correct will update the tests.
There was a problem hiding this comment.
Tests updated
akuzm
approved these changes
Jan 12, 2023
akuzm
reviewed
Jan 12, 2023
akuzm
reviewed
Jan 12, 2023
erimatnor
approved these changes
Jan 12, 2023
fabriziomello
force-pushed
the
remove_parallel_safe_from_partialize_agg
branch
4 times, most recently
from
29d902a
to
7544b0a
Compare
Previous PR timescale#4307 mark `partialize_agg` and `finalize_agg` as parallel safe but this change is leading to incorrect results in some cases. Those functions are supposed work in parallel but seems is not the case and it is not evident yet the root cause and how to properly use it in parallel queries so we decided to revert this change and provide correct results to users. Fixes timescale#4922
fabriziomello
force-pushed
the
remove_parallel_safe_from_partialize_agg
branch
from
7544b0a
to
d733b09
Compare
sb230132
added a commit
to sb230132/timescaledb
that referenced
this pull request
Jan 18, 2023
This release contains bug fixes since the 2.9.1 release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#5180 Fix dist_cagg flaky test * timescale#5170 Fix CAgg on CAgg variable bucket size validation * timescale#5114 Fix issue with deleting data node and dropping database * timescale#5172 Remove parallel safe from partialize_agg * timescale#5162 Fix telemetry_stats test in PG15 * timescale#5130 Fix CAgg on CAgg variable bucket size validation * timescale#5133 Fix CAgg on CAgg different column order * timescale#5152 Fix adding column with NULL constraint * timescale#5136 Fix SELECT from partial compressed chunks * timescale#5181 Fix ChunkAppend, ConstraintAwareAppend child subplan
Closed
sb230132
added a commit
to sb230132/timescaledb
that referenced
this pull request
Jan 18, 2023
This release contains bug fixes since the 2.9.1 release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#5180 Fix dist_cagg flaky test * timescale#5170 Fix CAgg on CAgg variable bucket size validation * timescale#5114 Fix issue with deleting data node and dropping database * timescale#5172 Remove parallel safe from partialize_agg * timescale#5162 Fix telemetry_stats test in PG15 * timescale#5133 Fix CAgg on CAgg different column order on the original hypertable * timescale#5152 Fix adding column with NULL constraint to compressed hypertable * timescale#5136 Fix SELECT from partial compressed chunks * timescale#5181 Fix ChunkAppend, ConstraintAwareAppend child subplan
sb230132
added a commit
to sb230132/timescaledb
that referenced
this pull request
Jan 19, 2023
This release contains bug fixes since the 2.9.1 release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#5180 Fix dist_cagg flaky test * timescale#5170 Fix CAgg on CAgg variable bucket size validation * timescale#5114 Fix issue with deleting data node and dropping database * timescale#5172 Remove parallel safe from partialize_agg * timescale#5162 Fix telemetry_stats test in PG15 * timescale#5133 Fix CAgg on CAgg different column order on the original hypertable * timescale#5152 Fix adding column with NULL constraint to compressed hypertable * timescale#5136 Fix SELECT from partial compressed chunks * timescale#5181 Fix ChunkAppend, ConstraintAwareAppend child subplan * timescale#5193 Fix repartition behavior when attaching data node
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.
Previous PR #4307 mark
partialize_aggandfinalize_aggas parallel safe but this change is leading to incorrect results in some cases.Those functions are supposed work in parallel but seems is not the case and it is not evident yet the root cause and how to properly use it in parallel queries so we decided to revert this change and provide correct results to users.
Fixes #4922