Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

SQL: show local node processlist #15455

Closed
wants to merge 1 commit into from
Closed

SQL: show local node processlist #15455

wants to merge 1 commit into from

Conversation

a6802739
Copy link
Contributor

@knz, This PR was used to show local node processlist, see #7003. And I just refer to you PR #10317.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@a6802739 a6802739 requested review from jordanlewis, knz and nvanbenschoten and removed request for jordanlewis April 28, 2017 08:23
@knz
Copy link
Contributor

knz commented Apr 28, 2017

Thanks for taking #10317 over. However, the points raised in there still hold: the current code is subject to a race condition, when queries to the new crdb_internal read the state of the session concurrently with execution. The race condition makes the values in crdb_internal potentially unsound, and will cause our race detection tests to complain.

The proper approach, suggested earlier by Andrei, is to make a copy of the useful Session fields after each statement or each transaction, and only read from that copy in the code that populates the virtual table. The copy would be protected by a mutex.

@a6802739
Copy link
Contributor Author

a6802739 commented May 2, 2017

@knz , You mean we should lock the field of session when we use them to create table crdb_internal.sessions?

@knz
Copy link
Contributor

knz commented May 2, 2017

Locking the fields used in Session directly is a non-starter: there are so many uses of these fields that 1) it would be very hard to guarantee they are locked everywhere then 2) the performance overhead would be terrible.

The proper approach is to copy all the fields in Session that are useful to the process list in between the execution of two statements. Then the copy would be dedicated to the process list generator. The copy needs to be locked because it can be accessed concurrently both by the process list generator and in-between executes. Also, care must be taken to do this properly when parallel statement execution is also enabled, because then the idea of "in between execution" is not well-defined any more.

@a6802739
Copy link
Contributor Author

a6802739 commented May 2, 2017

@knz , you mean like this? when we create table crdb_internal.sessions

            s.Lock()
            queryInfo := QueryInfo{
                Database : s.Database,
                User : s.User,
                ...
            }
            s.Unlock()

@a6802739
Copy link
Contributor Author

a6802739 commented May 2, 2017

@knz, Oh, I see. You mean when we begin execute a statement in the session, we should make a copy of the field of this session, and then when we create table crdb_internal.sessions, we could just use this copy as the process list generator?

So we should add a QueryInfo field in struct Session, And When we execute the statement, we could copy some field of session into QueryInfo.

@knz
Copy link
Contributor

knz commented May 2, 2017

Yes, that is the basic idea.

However there is some additional complexity because there may be multiple statements executing in parallel in the session.

@knz knz requested a review from vivekmenezes May 2, 2017 13:15
@a6802739
Copy link
Contributor Author

a6802739 commented May 2, 2017

@knz, hou could multiple statements executing in parallel in the session this happen?

@knz
Copy link
Contributor

knz commented May 2, 2017

@a6802739
Copy link
Contributor Author

a6802739 commented May 3, 2017

@knz, I just add the statement when we execute the statement in execStmt( and execStmtInParallel(.

@a6802739
Copy link
Contributor Author

a6802739 commented May 8, 2017

@knz, could you help me a review. Thank you very much.

@tamird
Copy link
Contributor

tamird commented Jun 20, 2017

@a6802739 how does this relate to #16072 ? If it's still relevant, could you rebase it?

cc @knz @itsbilal

@knz
Copy link
Contributor

knz commented Jul 20, 2017

Dear songhao, we have implemented this feature already in a different way, using the new statements SHOW JOBS and SHOW QUERIES. Soon we will also implement PAUSE/CANCEL for them.
I believe this PR can be closed. If you wish us to enhance this feature in any way, please let us know.

@knz knz closed this Jul 20, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants