From 7672be3c15bad387dc3a8249b139b09fd4472a77 Mon Sep 17 00:00:00 2001 From: Drew Kimball Date: Thu, 23 Feb 2023 01:12:56 -0800 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 6307471dbd81..d1117c83930a 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 938d6c8c7418..b0eaf5da2968 100644 --- a/pkg/sql/logictest/testdata/logic_test/pg_catalog +++ b/pkg/sql/logictest/testdata/logic_test/pg_catalog @@ -4610,3 +4610,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 89c41eeadfff..10194bf0102d 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 f3733a56c7ae..b97453ab5221 100644 --- a/pkg/sql/physicalplan/physical_plan.go +++ b/pkg/sql/physicalplan/physical_plan.go @@ -662,11 +662,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 67e075944726..4275da7c22c3 100644 --- a/pkg/sql/physicalplan/physical_plan_test.go +++ b/pkg/sql/physicalplan/physical_plan_test.go @@ -163,7 +163,6 @@ func TestProjectionAndRendering(t *testing.T) { ); err != nil { t.Fatal(err) } - }, expPost: execinfrapb.PostProcessSpec{ @@ -172,6 +171,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 {