Skip to content
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

Feature: make executor.finish() respect abort signal #5481

Closed
sundy-li opened this issue May 20, 2022 · 6 comments
Closed

Feature: make executor.finish() respect abort signal #5481

sundy-li opened this issue May 20, 2022 · 6 comments
Assignees
Labels
A-query Area: databend query C-feature Category: feature

Comments

@sundy-li
Copy link
Member

sundy-li commented May 20, 2022

Summary

Description for this feature.

Related #5448 #5472

For example NumbersSource, we should respect the abortable signal from context.

impl SyncSource for NumbersSource {
    const NAME: &'static str = "numbers";

    fn generate(&mut self) -> Result<Option<DataBlock>> {
        let source_remain_size = self.end - self.begin;

        match source_remain_size {
            0 => Ok(None),
            remain_size => {
                let step = std::cmp::min(remain_size, self.step);
                let column_data = (self.begin..self.begin + step).collect();

                self.begin += step;
                let column = UInt64Column::new_from_vec(column_data);
                Ok(Some(DataBlock::create(self.schema.clone(), vec![
                    Arc::new(column),
                ])))
            }
        }
    }
}
@sundy-li sundy-li added C-feature Category: feature A-query Area: databend query labels May 20, 2022
@zhang2014
Copy link
Member

zhang2014 commented May 20, 2022

We don't need to add it. We can abort executor(executor.finish() ).

@sundy-li sundy-li moved this from Todo to In Progress in Databend Query Engine May 20, 2022
@sundy-li sundy-li changed the title Feature: support abortable source Feature: make executor.finish() respect abort signal May 20, 2022
@TCeason
Copy link
Collaborator

TCeason commented May 20, 2022

I make a test on my local pc:

not use new processor

In session 1:

run a long query and then let it execute a few seconds,

next, try to interpret it.

root@mysqldb 12:44:19 [(none)]> SET enable_new_processor_framework=0;
Query OK, 0 rows affected (0.05 sec)
Read 0 rows, 0.00 B in 0.002 sec., 0 rows/sec., 0.00 B/sec.

root@mysqldb 12:45:05 [(none)]> select * from numbers(9999999999999);
^C^C -- query aborted
^C^C -- query aborted

In session 2:

execute show processlist to query the state. session 1 is Idle.

root@mysqldb 12:45:10 [(none)]> SET enable_new_processor_framework=0;
Query OK, 0 rows affected (0.06 sec)
Read 0 rows, 0.00 B in 0.002 sec., 0 rows/sec., 0.00 B/sec.

root@mysqldb 12:45:14 [(none)]> show processlist;
+--------------------------------------+-------+-----------------+------+-------+----------+--------------------------------------+--------------+------------------------+-------------------------+-------------------------+--------------------------+---------------------+
| id                                   | type  | host            | user | state | database | extra_info                           | memory_usage | dal_metrics_read_bytes | dal_metrics_write_bytes | scan_progress_read_rows | scan_progress_read_bytes | mysql_connection_id |
+--------------------------------------+-------+-----------------+------+-------+----------+--------------------------------------+--------------+------------------------+-------------------------+-------------------------+--------------------------+---------------------+
| 3b9fa9c1-7a74-4375-a98f-70987d215e55 | MySQL | 127.0.0.1:57648 | root | Query | default  | select * from numbers(9999999999999) |   4295000390 |                      0 |                       0 |                       0 |                        0 |                  10 |
| e2f7dcf3-b543-40b2-a357-5c544c875c20 | MySQL | 127.0.0.1:57646 | root | Query | default  | show processlist                     |            0 |                      0 |                       0 |                       0 |                        0 |                   9 |
+--------------------------------------+-------+-----------------+------+-------+----------+--------------------------------------+--------------+------------------------+-------------------------+-------------------------+--------------------------+---------------------+
2 rows in set (0.09 sec)
Read 2 rows, 452.00 B in 0.034 sec., 58 rows/sec., 12.80 KiB/sec.

-- After Press Ctrl C in session 1

root@mysqldb 12:45:22 [(none)]> show processlist;
+--------------------------------------+-------+-----------------+------+-------+----------+------------------+--------------+------------------------+-------------------------+-------------------------+--------------------------+---------------------+
| id                                   | type  | host            | user | state | database | extra_info       | memory_usage | dal_metrics_read_bytes | dal_metrics_write_bytes | scan_progress_read_rows | scan_progress_read_bytes | mysql_connection_id |
+--------------------------------------+-------+-----------------+------+-------+----------+------------------+--------------+------------------------+-------------------------+-------------------------+--------------------------+---------------------+
| 3b9fa9c1-7a74-4375-a98f-70987d215e55 | MySQL | 127.0.0.1:57648 | root | Idle  | default  | NULL             |            0 |                   NULL |                    NULL |                    NULL |                     NULL |                  10 |
| e2f7dcf3-b543-40b2-a357-5c544c875c20 | MySQL | 127.0.0.1:57646 | root | Query | default  | show processlist |            0 |                      0 |                       0 |                       0 |                        0 |                   9 |
+--------------------------------------+-------+-----------------+------+-------+----------+------------------+--------------+------------------------+-------------------------+-------------------------+--------------------------+---------------------+
2 rows in set (0.09 sec)
Read 2 rows, 415.00 B in 0.033 sec., 60.48 rows/sec., 12.25 KiB/sec.

So could we still need to support the abortable source?@sundy-li

@sundy-li
Copy link
Member Author

We already confirmed that it's related to enable_new_processor_framework =1, so let's support the kill session in this case.

@TCeason
Copy link
Collaborator

TCeason commented May 20, 2022

NumbersSource

I think I got this reason:

https://github.com/datafuselabs/databend/blob/2bd1f211f92511779d5cb4fb23f8361ba55953cd/query/src/table_functions/numbers_stream.rs#L115

numbers will generate a big vec to store the begin..end.

for part in partitions {
                let numbers_part = NumbersPartInfo::from_part(&part)?;
                let (begin, end) =
                    self.try_apply_top_n(numbers_part.part_start, numbers_part.part_end);

                let diff = end - begin;
                let block_nums = diff / block_size;
                let block_remain = diff % block_size;

                if block_nums == 0 {
                    blocks.push(BlockRange { begin, end });
                } else {
                    for r in 0..block_nums {
                        let range_begin = begin + block_size * r;
                        let mut range_end = range_begin + block_size;
                        if r == (block_nums - 1) && block_remain > 0 {
                            range_end += block_remain;
                        }
                        blocks.push(BlockRange {
                            begin: range_begin,
                            end: range_end,
                        });
                    }
                }
            }

But the kill query just set an abort tag not really kill the executing query.

It will wait for the next poll and then judge the is_abort tag, if true, really abort.

So in this case, if send a query select * from numbers(n) to databend server and n is so large.

Although I send kill query on other session or press Ctrl C in the current session. It looks like not useful.

And because the n is so large and then generates a very large vector that will make databend be oom killed.

@TCeason
Copy link
Collaborator

TCeason commented May 30, 2022

So maybe we can close this issue?

In the new processor we can abort executor(executor.finish()).

In the old processor, we find the query hang is generated a big meta vector in one poll, so the query can not be killed.

@zhang2014
Copy link
Member

Let's to close.

Repository owner moved this from In Progress to Done in Databend Query Engine May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query Area: databend query C-feature Category: feature
Projects
Development

No branches or pull requests

3 participants