From 7369f12702e61f6b1ede60179f2189aa943736d9 Mon Sep 17 00:00:00 2001 From: Drew Kimball Date: Thu, 23 Feb 2023 09:12:56 +0000 Subject: [PATCH] physicalplan: don't combine render nodes into one processor During planning for a render node, the physical planning logic checks whether the last created processor rendered its own expressions, adds its render expressions to the processor if not. The check is necessary to handle the case when expressions in a parent render reference those from a child render. However, render nodes also handle projecting input columns: both removing them and reordering them. Correct planning of the processors depends on the columns at each step being in a particular order. Since physical planning didn't check whether the existing processor projected its columns in some way, the parent render could be combined with its child. This could lead to panics in the parent render expressions when the columns weren't projected as expected. This patch fixes the issue by adding the projection check before allowing the render planning code to add to the previous processor. Fixes #95633 Release note (bug fix): Fixed a rare bug existing since before 22.1 that could cause a projected expression to replace column references with the wrong values. --- .../logic_test/multi_region_query_behavior | 2 +- .../logictest/testdata/logic_test/pg_catalog | 5 +++ .../execbuilder/testdata/distsql_merge_join | 4 +-- pkg/sql/physicalplan/physical_plan.go | 8 ++--- pkg/sql/physicalplan/physical_plan_test.go | 33 ++++++++++++++++++- 5 files changed, 44 insertions(+), 8 deletions(-) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_query_behavior b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_query_behavior index 8b33f9d0f7e2..9677a5cb94a3 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_query_behavior +++ b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_query_behavior @@ -410,7 +410,7 @@ vectorized: true table: orders@orders_pkey spans: [/'ap-southeast-2'/'94e4b847-8f2f-4ac5-83f1-641d6e3df727' - /'ap-southeast-2'/'94e4b847-8f2f-4ac5-83f1-641d6e3df727'] [/'us-east-1'/'94e4b847-8f2f-4ac5-83f1-641d6e3df727' - /'us-east-1'/'94e4b847-8f2f-4ac5-83f1-641d6e3df727'] · -Diagram: https://cockroachdb.github.io/distsqlplan/decode.html#eJyslM1uGjEQx-99CmsuaSMT9otAVorkKKUtEYUUIrVSjdBmPSRuwN7a3nariFOfre9V7S4hISpUSXsA2-P5M_Mbz3AL9uscYuh-Ou-f9Abk5eve-GL8of-KjLv97ukF0UagmUpBSWb0QtuDalkZZIrkZFxvpt-lu9a5m1YOd7f7REib6ly5Tcfai7wZDd_XISw5G_YGqyBkeLc7kGqmCeO554W48qxtH991R911euSY7B1FGF12onajMwtmjShJW41OOPMbh5EvDjEUs3bQ3gMKSgscJAu0EH-GCCYUMqNTtFab0nRbOfREAbFHQaosd6V5QiHVBiG-BSfdHCGGi-RyjiNMBJqmBxQEukTOq5-tM2X1Ms1u8AdQONXzfKFsvFFUmSIlJRFQGGdJed3kwDg0OXBeHEWcF1h-XXbecl50Zs2zXz85L2a-4LzwhTouD-09Dk2PJEoQn2h3jQYmSwo6d_fpW5dcIcT-kj4P0f-_iCWM9yzMrWjBVrR7IotGJnOSqypDFBtQk-UfajDQDZ01g036vlxIR_ytqXhPqXJPfUPjUJxpqdA0w81Q9SSwepmWdZxKUQBdy7pFZghrrceEhQ9f4n5gV08wzF1MmE9ZQFlIWbSVIXwKQ5n7qlGiXfmvGqWv9U2ekS9aKqJVTFgpGg4Ia_8V4-4fZQ00QiXQVEyEtShhQfnZZ4dbyaKnkI3QZlpZfNQp2959QgHFFdZdZ3VuUjw3Oq3C1MdhpasMAq2rb4P60FPVVTWkD8X-v4iDneJwQ-w9Foc7xdFucbRT3Hoknixf_A4AAP__TIEqRA== +Diagram: https://cockroachdb.github.io/distsqlplan/decode.html#eJyslN1u2koQx-_PU6zmJudES_AnEEuRNsqhLRGFFCK1Uhchxzsk28Cuu163riKu-mx9r8o2-SCqqUh7Aeudmb9nfrOzvoPs8xIi6H-4GJ4ORuTf_wfTy-m74X9k2h_2zy6JNgLNXApKUqNXOjuqlo1BJkhOp_XD_Ku0Nzq38yrg3ntIhMwSnSu7HVhHkVeT8ds6RUbOx4PRJgkZ3z8dSbXQhPHccXzcRNa292_6k_5DeeSEHBwHGFz1gm6rt_AWrSBOwlbPX7itTuCKDvpi0fW6B0BBaYGjeIUZRB8hgBmF1OgEs0yb0nRXBQxEAZFDQao0t6V5RiHRBiG6AyvtEiGCy_hqiROMBZq2AxQE2lguq9fWlbJ6mae3-A0onOllvlJZtNVUmSAlJRFQmKZx6W5zYBzaHDgvjgPOCyz_rnqvOS96i_b5j--cFwtXcF64Qp2Um-4Bh7ZDYiWIS7S9QQOzNQWd28fyMxtfI0Tumr4M0f27iCWM8yLMRjSvEe2RKEMj4yXJVVUhii2o2foXPRjplk7b3jb9UK6kJW5jKc4-XR6oL2gsinMtFZq2v52qvgmsXuZlH-dSFEAfZP0iNYSFD9eE-U9P4vHCbo5gnNuIMJcyjzKfsqCRwd-Hoax9MyjBrvo3gzLU-jZPySctFdEqIqwUjUeEdX-Lcf9FaQIKKetQ1m3ECvbBqg8_3CaaoBJoqpyEBZQwr_wdsrAxZ7hPzglmqVYZPhvNpkGbUUBxjfWYZzo3CV4YnVRp6u240lUGgZmtvV69GajKVX0VnordPxF7O8X-lth5LvZ3ioPd4mCnONwtDneKO8_Es_U_PwMAAP__O2xhIQ== # Regression test for #74890. Code should not panic due to distribution already # provided by input. diff --git a/pkg/sql/logictest/testdata/logic_test/pg_catalog b/pkg/sql/logictest/testdata/logic_test/pg_catalog index 9f5d3417d4fb..83070bdea659 100644 --- a/pkg/sql/logictest/testdata/logic_test/pg_catalog +++ b/pkg/sql/logictest/testdata/logic_test/pg_catalog @@ -4780,3 +4780,8 @@ WHERE AND t.typname LIKE 'myt%'; ---- 0 + +# Regression test for a panic after physical planning combined rendering steps +# (#95633). +statement ok +SELECT d.datname, pg_catalog.pg_get_userbyid(d.datdba) FROM pg_catalog.pg_database d; diff --git a/pkg/sql/opt/exec/execbuilder/testdata/distsql_merge_join b/pkg/sql/opt/exec/execbuilder/testdata/distsql_merge_join index 76d39e5bf0d0..9d09d69b262b 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/distsql_merge_join +++ b/pkg/sql/opt/exec/execbuilder/testdata/distsql_merge_join @@ -823,7 +823,7 @@ vectorized: true table: outer_c1@outer_c1_pkey spans: FULL SCAN · -Diagram: https://cockroachdb.github.io/distsqlplan/decode.html#eJzMll9r4kAUxd_3Uwz3SbcjyeSPtYGFFNcuFqtdtbCwSEmTuyprM9lJhC3F774kqbiGNpMwKr5IMpnf3DnnXDN5hfjPChzo_bgfXPeHpPG1P5lOvg-aZNIb9LpT8pncjEd3hK8TFI8RIzcPgwG5HfWHb0M-Iw-T_vAbaUTLgDWBQsgDHHrPGIPzExhQMICCCRQsoGDDjEIkuI9xzEU65TUD-sFfcHQKyzBaJ-nwjILPBYLzCskyWSE4MPWeVjhGL0ChpesGmHjLVVZmuz13e_EY_cYXoNDlq_VzGDsk3R0lkZeCk8hLhzS9pdnECwPCCE8WKGC2oelSb1vYVX56IQsvXuzXdBnMNjMKceLNERy2oR9I2a3DRYACg-I6F2nh3ax1-N68rFbRkjsUc7zlyxCFdrm_7PQlQiePa_Qw7Y2z0IDCCn8lDZddNL-I5XyRXwKFMYYBCod0R9eD3qTba7iMumaTEtegxLUocW1K3HbBop18s4L8GsKGvMUjjbHCzPdrW3u1WfUuMg7RRXZLY_qR2oidvo06Z9RGRvUozUNEyfSWxo71SjBOn-XVGWVpVs_SOkiWdkszjvW_NE-fJdPPKEyrepj6AcJsaccKsoYQ-z0h_laI_5EQf_trUOLviVKSYXwo40T92D6jdpR8OI4xjngYY6WPCT0VjsEccy9jvhY-3gvuZ2Xy21HGZcdzgHGSP2Vvd_0wf5busAatAlsqcFsF7pTDrAjr_8NGOWyUwmyf1ou0qRSWhFaBJWGVw5KwymFJWJZKWLaS3RJaBZbYXQ5L7C6HJXa3Vey-VLJbQqvAErvLYYnd5bDE7o6K3VdKdktoFVhidzkssbsclr35ax2WRl1cibaU6LYS3ZHQ9U7M2ebTvwAAAP__BTlqQg== +Diagram: https://cockroachdb.github.io/distsqlplan/decode.html#eJzEVu9r4koU_f7-iuF-0teRZPLD2sCDFJ99WKz2qYWFRUqa3FVZm8lOImwp_u9LTMTV1YxhjH4Rk8yZc-49J3fyCfGPBTjQ-fLcu-_2Se3f7mg8-r9XJ6NOr9Mek7_Jw3DwRPgyQfEaMfLw0uuRx0G3n9_yGXkZdfv_kVo0D1gdKIQ8wL73jjE4X4EBBQMomEDBAgo2TChEgvsYx1ykSz7XgG7wExydwjyMlkl6e0LB5wLB-YRkniwQHBh7bwscoheg0NJ9A0y8-WJNs5Hnbv68Rt_xAyi0-WL5HsYOSdVREnkpcBR56S1Nb2g28cKAMMKTGQqYrGi6VS5hy_z2QWZePNvldBlMVhMKceJNERy2okdK2e7DRYACg_19blLi7apleGjdmmu_JU8opvjI5yEK7XZ32_FHhE5m1-Bl3BmuTQMKC_yW1Fx2U_9HzKez7C9QGCwTh7iMugZ1Tepa1LWp29zryLZa82i1B1T2eYNHGjN2BQ4xDFA4pD2473VG7U4tZTfrlLgGJa5FiWtTUiDBOqHhJVqZi2zurTzMbe9ws9Nza5wjt3ZDY3pFwWWXD27resGVOJdnwqwyuMbp4THPER6mNzRW1dgzLp-eu-ulR2Jdnh6ryvSYp6fHOkt67IZmVDV7zMunh-nXi4_Euzw-dpXxsU6Pj36G-DS0qqJTohD7UCH-phD_WCH-5tegxN8pSqkM42gZF3oDmtd7ASSm5S8Au9Rn4wEJQ4wjHsZ40kehnrYagylm7sV8KXx8Ftxf02SXgzVu_eERYJxkT5mdXXXD7FmqsARaBazE3FKSLdHN9tH672hjB6zvg41CsFnMbBaC2W0xtaVktQStAlZillgtkS3RbRc2vFnc76aK1bdKZknQKmAlZolZEtkS3a3Cft8Vm3WnYhZTm6IyuBJajVtimEy5THrxKGWSWcqUhin7Y5qWM00CV0KrcctMkyiXSS91jhhl4UpoNe6WmnKZ9OLDhElOE1buOJms_voVAAD__whhgYA= query T EXPLAIN (VERBOSE) SELECT * FROM outer_gc1 FULL OUTER JOIN outer_c1 USING (pid1, cid1, cid2) @@ -883,7 +883,7 @@ vectorized: true table: outer_c1@outer_c1_pkey spans: FULL SCAN · -Diagram: https://cockroachdb.github.io/distsqlplan/decode.html#eJyckt9um0wQxe-_p1jNlf15InvB6R-kShu5pCIidgpYqlRZEWWnFJWwdBekRpbfvQJax6DYcXuD2LNzht8ZZgvmRw4OuJ_u_CtvyUbvvTAKP_pjFrq-u4jY_-w6WN0yVVek79OEs-u177Oblbf8rSWcrUNv-YGNykxyZMmfpzUGhEJJWsYPZMD5DBwQLmGDUGqVkDFKN_K2LfLkT3BmCFlR1lUjbxASpQmcLVRZlRM4sFQXqpzagCCpirO8LdshqLp6MpkqTgmc-Q4PGvPTjaP4S04BxZL0dNZrD_vgYv92X36nR0BYqLx-KIzDhsGRpd0hTeImc1jGTdkFHIPl_wrLn4Pds56FeiaidRTxiawulJakSQ7_0Mslz-S8JZ3SjcoK0lOrnzN6LMnpNnG1jtyg3UdAyOlrNRJ8gsKaoLAn43c6S7_1JUAIqJCkHbZYXfluuHBHgqN4NcYDwULxuifYKN6MkYk5MnGJTLw9Oim7N6kXVjogU6rC0Fk7PWvGRDKlbuxG1TqhO62S9jPdcdX6WkGSqbrbeXfwiu6qATw085Nmu2fmQ7P1F2ZraLZPmmcD7M3uv18BAAD__7KcfNU= +Diagram: https://cockroachdb.github.io/distsqlplan/decode.html#eJyck1Fvm0AMx9_3KU5-ShZXyQFpO6RJV2V0oqKhC4k0aYoqBh5Doxw7DmlVlO8-AVsaooamfUH4f_6bn7FvA-XvDGxwvt55V-6cDT65wTL44g1Z4HjObMnes-uFf8tkpUndJxFn1yvPYze-O_-nRZytAnf-mQ2KNObIov9PYwgIuYxpHj5QCfY34IAwhTVCoWREZSlVLW-aJDf-A_YEIc2LStfyGiGSisDegE51RmDDXJ7JYmwBQkw6TLMmbYsgK_1kKnWYENjTLe4V5v2Fl-H3jBYUxqTGk0552DUudm_3xS96BISZzKqHvLTZYePIkjZIorDuOSjCOu0MjsHyt8Ly52B3rCehnohoHEV8IqtyqWJSFB9O6OWUZ_q8JZXQjUxzUmOj2-fysSC73UR_tXQWzT4CQkY_9EDwEQpjhMIcDT-qNPnZlQDBr7TNBEdhoDBRWCimKM5RXKC4RPHh6D8wXzOmdlnNLviC8piUzWb-lecEM2dQU5wPcU8wUFx0BBPF5RCZsJCJKbIePqvD98JlWlBZyLykk27TpB4QxQm1Ay9lpSK6UzJqPtOGfuNrhJhK3Z5O28DN26MacN_Me81mx8wPzcYrzMah2ew1W_3YVq95cmBeb9_9DQAA__8XQ7Wb query T EXPLAIN (VERBOSE) SELECT * FROM outer_c1 LEFT OUTER JOIN outer_p1 USING (pid1) WHERE pid1 >= 0 AND pid1 < 40 diff --git a/pkg/sql/physicalplan/physical_plan.go b/pkg/sql/physicalplan/physical_plan.go index 25ea559f30d2..3832b6dcdd9d 100644 --- a/pkg/sql/physicalplan/physical_plan.go +++ b/pkg/sql/physicalplan/physical_plan.go @@ -607,11 +607,11 @@ func (p *PhysicalPlan) AddRendering( } post := p.GetLastStagePost() - if len(post.RenderExprs) > 0 { + if len(post.RenderExprs) > 0 || len(post.OutputColumns) > 0 { post = execinfrapb.PostProcessSpec{} - // The last stage contains render expressions. The new renders refer to - // the output of these, so we need to add another "no-op" stage to which - // to attach the new rendering. + // The last stage contains render expressions, or is projecting input columns. + // The new renders refer to the output of these in a particular order, so we + // need to add another "no-op" stage to which to attach the new rendering. p.AddNoGroupingStage( execinfrapb.ProcessorCoreUnion{Noop: &execinfrapb.NoopCoreSpec{}}, post, diff --git a/pkg/sql/physicalplan/physical_plan_test.go b/pkg/sql/physicalplan/physical_plan_test.go index c0729ea2900b..b330b716a0f3 100644 --- a/pkg/sql/physicalplan/physical_plan_test.go +++ b/pkg/sql/physicalplan/physical_plan_test.go @@ -160,7 +160,6 @@ func TestProjectionAndRendering(t *testing.T) { ); err != nil { t.Fatal(err) } - }, expPost: execinfrapb.PostProcessSpec{ @@ -169,6 +168,38 @@ func TestProjectionAndRendering(t *testing.T) { }, expResultTypes: "B,D,C", }, + + // Rendering after projection. + { + post: execinfrapb.PostProcessSpec{ + Projection: true, + OutputColumns: []uint32{3, 1, 4, 2}, + }, + resultTypes: "A,B,C,D", + + action: func(p *PhysicalPlan) { + if err := p.AddRendering( + context.Background(), + []tree.TypedExpr{ + &tree.IndexedVar{Idx: 0}, + &tree.IndexedVar{Idx: 1}, + &tree.IndexedVar{Idx: 2}, + }, + fakeExprContext{}, + []int{2, 0, 3, 1}, + []*types.T{strToType("C"), strToType("A"), strToType("D")}, + execinfrapb.Ordering{}, + ); err != nil { + t.Fatal(err) + } + }, + + expPost: execinfrapb.PostProcessSpec{ + Projection: true, + OutputColumns: []uint32{4, 3, 2}, + }, + expResultTypes: "C,A,D", + }, } for testIdx, tc := range testCases {