From 70da95e7a34c5d11f04ed9f65f5c3850924e8698 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Thu, 14 Sep 2023 16:44:59 -0700 Subject: [PATCH 1/2] sql: wrap each planNode into DistSQL independently when collecting stats This commit adjusts the DistSQL physical planner to create a pair of `planNodeToRowSource` and `rowSourceToPlanNode` for each `planNode` whenever it's included into the DistSQL flow separately whenever the execution statistics are collected. This allows us to collect exec stats for each plan node (rather than see the execution time of the whole chain of `planNode`s and the number of output rows only of the first `planNode` to be wrapped). This should have negligible overhead. The only exception for when this wrapping is disabled is when the planNode implements `batchedPlanNode` interface since wrapping those types of planNodes breaks some assumptions. This was useful in a recent query latency investigation where multiple virtual table lookup joins (powered by the corresponding planNodes) were taking vast majority of the query execution, but since all of them were hidden behind a single pair of DistSQL adapters, it wasn't clear which particular vtable lookup join was the bottleneck. Release note: None --- pkg/sql/distsql_physical_planner.go | 25 ++++++++++++++++--- .../testdata/explain_analyze_plans | 8 +++++- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/pkg/sql/distsql_physical_planner.go b/pkg/sql/distsql_physical_planner.go index 28033ebe564f..24884d055bfd 100644 --- a/pkg/sql/distsql_physical_planner.go +++ b/pkg/sql/distsql_physical_planner.go @@ -465,6 +465,19 @@ func (dsp *DistSQLPlanner) mustWrapNode(planCtx *PlanningCtx, node planNode) boo return false } +func shouldWrapPlanNodeForExecStats(planCtx *PlanningCtx, node planNode) bool { + if !planCtx.collectExecStats { + // If execution stats aren't being collected, there is no point in + // having the overhead of wrappers. + return false + } + // Wrapping batchedPlanNodes breaks some assumptions (namely that Start is + // called on the processor-adapter) because it's executed in a special "fast + // path" way, so we exempt these from wrapping. + _, ok := node.(batchedPlanNode) + return !ok +} + // mustWrapValuesNode returns whether a valuesNode must be wrapped into the // physical plan which indicates that we cannot create a values processor. This // method can be used before actually creating the valuesNode to decide whether @@ -3617,9 +3630,15 @@ func (dsp *DistSQLPlanner) wrapPlan( } var err error // Continue walking until we find a node that has a DistSQL - // representation - that's when we'll quit the wrapping process and hand - // control of planning back to the DistSQL physical planner. - if !dsp.mustWrapNode(planCtx, plan) { + // representation - that's when we'll quit the wrapping process and + // hand control of planning back to the DistSQL physical planner. + // + // However, if we're collecting execution stats, then we'll surround + // each planNode with a pair of planNodeToRowSource and + // rowSourceToPlanNode adapters so that the execution statistics + // are collected for each planNode independently. This should have + // low enough overhead. + if !dsp.mustWrapNode(planCtx, plan) || shouldWrapPlanNodeForExecStats(planCtx, plan) { firstNotWrapped = plan p, err = dsp.createPhysPlanForPlanNode(ctx, planCtx, plan) if err != nil { diff --git a/pkg/sql/opt/exec/execbuilder/testdata/explain_analyze_plans b/pkg/sql/opt/exec/execbuilder/testdata/explain_analyze_plans index bc5359cdfe99..c210458b6fdc 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/explain_analyze_plans +++ b/pkg/sql/opt/exec/execbuilder/testdata/explain_analyze_plans @@ -383,9 +383,15 @@ quality of service: regular │ │ into: child(c, p) │ │ │ └── • buffer +│ │ nodes: +│ │ regions: +│ │ actual row count: 1 │ │ label: buffer 1 │ │ │ └── • values +│ nodes: +│ regions: +│ actual row count: 1 │ size: 2 columns, 1 row │ ├── • subquery @@ -451,5 +457,5 @@ quality of service: regular label: buffer 1 · Diagram 1 (subquery): https://cockroachdb.github.io/distsqlplan/decode.html#eJysU9FuGjsQfb9fYc0TSEbsEl2p8lNSQiUUAhEQpKpCyPFONhZee2vPFmjEZ_UH-mXV7mZTSJq0UesHWM-Mj4_PmbmH8NmAgOF4NpjO2XA8nzB1p03CFmej68GMtWLOWrPBaNCfs0zbVt5mH6aTS5ZLj5babeBgXYJjmWEA8Qli4PA_LDnk3ikMwfkyfF8VDZMtiIiDtnlBZXjJQTmPIO6BNBkEAWPXcXm3BxwSJKlNBYpbVAVpZxnpDAWLvn8LwOFGkrrDwFxBeUGCRcDBu83PQAzLPYd693BfIJkiiJMDgsNzEL09P-AYv85xLm8MTlEm6LvRMdNaltP6b5WvcQcc-s4UmQ2C5cBhlsvyswMcRjrTxErFLhbHL7tYMOUsoX3-6IsFq96YoHIJJqI5f7MjDMyjTAR7x97XwXR61WdKGhMe63KpfVNXyny56PdZIMyZcoUl1sItdbWltmBR9bq6AHH9UkEmtyzDzPkdk8Y4JamkFVUc_sKi-IlF0VssOktTj6kk57vxsUNn44-r8WS-Gl-PRq3TuGzgf99evSfc4yPuvxmBKYbc2YBHvF-6KXpyUyfeLzlgkmI9d8EVXuGVd6qqrbeTCqgKJBiozp7Um6FtUoE8yuxR-kOk-FWk3huQeq8iRc-RZCUAWKSN82tmJKFVu0fTmvhGajq2M8GAXkujv8rnXjfHKn89KtRfmtlqUs2ANbl6yJpshiHI9Kgg-sMeOtSncu_WuM1KJyAgelidX_w0C8oDMg1lC83u3KYSa77Lywa4lSYgh0u5xnMk9Jm2OpBWIMgXuN__9yMAAP__S7X0ow== -Diagram 2 (main-query): https://cockroachdb.github.io/distsqlplan/decode.html#eJyMUM3K2zAQvPcpxJwS0Eftq26lnwuG_JTY7aWYosqbRFSWXGlNUoIfqy_QJyu2E_pDC92DYGZ3Z0Z7Q_rioFDuquJQi3JX74U5W9eK968274pKrHIpVlWxKV7XorN-1a_Fm8N-K3odyfN6DQkfWtrpjhLUB-RoJPoYDKUU4kTd5oGyvUJlEtb3A090I2FCJKgb2LIjKLhgtBMmDJ5F9jKDREusrZuF6UpmYBu8YNuREtn3bwkSnzSbMyURBu4HVmLaiuHyk8jRjBILuvsm1ieCyn8JWj5DZaP8_6wHSn3wiX4L-S-n7A-np3xsJKg90XKgFIZo6G0MZp5d4H4WmomWEi_dfAGlf7QSR9LdEr-ROLpw-WhbKGT3evrL8yhMC_qUpo9V53CZZeuv_RTrqF0iia3-TM_EFDvrbWJroDgONI4vfgQAAP__Kmm20Q== +Diagram 2 (main-query): https://cockroachdb.github.io/distsqlplan/decode.html#eJy0ksGK2zAQhu99CjGnBLTE8t50K90UDLtJSdJeiilaeZyIypIrjZotwY_VF-iTFdu77dZ0AzmsDoL5JX3zIeYE8ZsFCcVqu9zsWLHarZk-GFuxT29vPy63bCY4m22Xt8t3O9YYN2vn7P1mfcdaFdDRfA4cnK9wpRqMID-DgJJDG7zGGH3oo9NwoageQGYcjGsT9XHJQfuAIE9AhiyCBOu1suy7sgkjyxYZcKiQlLEDGR9QJzLeMTINSpb9-hmBw70ifcDIfKI2kWT9q-CPfwMBZcdhrB4bR1J7BCmemRY3ILOOXyp7n-oaAxML8dqy-URWXC6rfXLE8kX-2q7XE9f8EtcNxta7iP9IvtQpm3S6El3JAas9jqMXfQoaPwSvh7tjuR5AQ1BhpPFUjEXhno4iBVTNn7l4ThJnSfnLJDEl5WdJ1-ecSg619ccvpgIJ2eO6-s_2tKB_oPax_-ztwR8H7O5H239VrWxEDnfqK94gYWiMM5GMBkkhYde9-R0AAP__XNhbxw== Diagram 3 (postquery): https://cockroachdb.github.io/distsqlplan/decode.html#eJy0lNFu4joQhu_PU4zmCiRXJNCLI1-1h1IpLYUKKDdHqHKdgfrg2Dm2o4IqHmtfYJ9slQS6hS7sdqXNBWgmv39_45n4Ff3_Gjkmg3FvNIFkMBmCfFY6hell_6E3hkbMoDHu9XvdCWTKNPImXI-Gd5ALRyY0m8jQ2JQGIiOP_F-MccYwd1aS99aVqddKkKQr5BFDZfIilOkZQ2kdIX_FoIIm5KitFBq8FAaeivmcHEStCBmmFITSlf2wCBwu2siQViSLoKyBoDLiEH394pHhkwjymTzYIuSltlzv7Mv3RIyzDcM62nL4IBaEPH4Hnlwhjzbs19mvlQ7kyLXifeA6z-EihmQMg-EEBg_9_h_hbx_wx5_hv7HKjEik5Frt_Qom65w49HvXE7gcTBK4GSYDZFgPwEX995gvaY0M-9Yuixz-s8qANWXVyLBrdZEZzyHHLQOUFZWl7GIfhNb7B3E7_RBLawKZj2d2O60MISVpU0pr49spPK0DeXAkUg5_wz91cjG674IUWvs3XS6U2-nK4u-m3S74QDlIW5gADVqFljKhybcDWQuIlscER5qbiRVklFm3BqHLaQ8lbVSh_bTx0dHGdw4a3_5M4-uPjpyzDtS8Psi41dmfgd8d1uPM5wfMnc8wj8jn1njagzy2U3Sw01m8mTGkdEH17eRt4STdOysrbR0OK6MqkZIP9du4DhKze-WDI5G93RXvneKTTu3jTvGhU_ukU-e4U_vQqXPS6fxUdTOGc21fHlWKHKPtc_aDn92D5QKx8GXbxs_2pbItrxKPfC60J4Z3YklXFMhlyigflEQeXEGbzV_fAgAA___v5iNJ From 78a405a1aad116c7596258e2cbe69dd3cf2d60bc Mon Sep 17 00:00:00 2001 From: Arjun Nair Date: Mon, 18 Sep 2023 18:56:09 -0400 Subject: [PATCH 2/2] kv/kvnemesis: copy value before holding a reference Epic: none Fixes: https://github.com/cockroachdb/cockroach/issues/110765 --- pkg/kv/kvnemesis/validator.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/kv/kvnemesis/validator.go b/pkg/kv/kvnemesis/validator.go index 28e456f6500b..33d2d6766526 100644 --- a/pkg/kv/kvnemesis/validator.go +++ b/pkg/kv/kvnemesis/validator.go @@ -1383,6 +1383,7 @@ func validReadTimes( // Handle a point key - put it into `hist`. valB, err := iter.ValueAndErr() + valB = append([]byte{}, valB...) if err != nil { panic(err) }