fix(batch): release epoch at the end of the distributed query if it contains lookup join #6997
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6997 +/- ##
==========================================
- Coverage 72.89% 72.89% -0.01%
==========================================
Files 1040 1040
Lines 167014 167032 +18
==========================================
+ Hits 121745 121758 +13
- Misses 45269 45274 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Even if we find out the stage of the lookup join, we can't release the pinned snapshot right after it gets scheduled. Previously we tried to release the pinned snapshot right after all scan iterators were created. I think as long as we hold the iterators, the storage can't release its corresponding epoch which is almost equivalent with releasing the epoch at the end of the query, because in most cases scan iterators end with the same time as the whole query. |
|
Makes sense. I've recalled it wrong since long ago, the table scan did not pin a specific version, and it's the frontend's responsibility to pin the snapshot for the whole process. 🤣 |
| @@ -247,14 +244,6 @@ impl QueryRunner { | |||
| stage_id | |||
| ); | |||
| self.scheduled_stages_count += 1; | |||
| stages_with_table_scan.remove(&stage_id); | |||
| if stages_with_table_scan.is_empty() { | |||
| // We can be sure here that all the Hummock iterators have been created, | |||
There was a problem hiding this comment.
Will there be race cases previously?
There was a problem hiding this comment.
It looks fine. I can't tell.
It is still possible to unpin the snapshot if there's no lookup join. |
|
Discussed with @chenzl25 offline. Lookup join will create storage iterators during its exeuction, not during scheduling. Based on the current interface, I cannot think of a way to do early epoch unpin in this case. To do that, we may need to do either of the following:
IMO, we can delay epoch unpin if the query has lookup join for now, given that we don't expect batch query to run for a long time in the serving use cases. We can optimize it when we do see long running batch queries in the future. |
This reverts commit 8874cdf.
| @@ -247,14 +244,6 @@ impl QueryRunner { | |||
| stage_id | |||
| ); | |||
| self.scheduled_stages_count += 1; | |||
There was a problem hiding this comment.
Suggest to add some comments for why remove the early unpin.
|
I have reimplemented in this way, only delay epoch unpin for lookup join. Please take a look again. @BugenZhao @BowenXiao1999 |
chenzl25
changed the title
|
@BowenXiao1999 |
mergify
bot
deleted the
dylan/release_epoch_at_the_end_of_distributed_query
branch
I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.
What's changed and what's your intention?
This section will be used as the commit message. Please do not leave this empty!
Please explain IN DETAIL what the changes are in this PR and why they are needed:
Checklist
./risedev check(or alias,./risedev c)Documentation
If your pull request contains user-facing changes, please specify the types of the changes, and create a release note. Otherwise, please feel free to remove this section.
Types of user-facing changes
Please keep the types that apply to your changes, and remove those that do not apply.
Release note
Please create a release note for your changes. In the release note, focus on the impact on users, and mention the environment or conditions where the impact may occur.
Refer to a related PR or issue link (optional)
#6979