vitessio / vitess

Vitess is a database clustering system for horizontal scaling of MySQL.
http://vitess.io
Apache License 2.0
18.71k stars 2.1k forks source link

Use proper keyspace when updating the query graph of a reference DML #17226

Closed frouioui closed 5 days ago

frouioui commented 1 week ago

Description

This PR fixes a regression introduced by https://github.com/vitessio/vitess/pull/15107, when a user is using (use ks) a sharded keyspace that has a reference table with its source in an unsharded keyspace, the query planner would not set the target keyspace correctly when rewriting the query graph, leading to the following plan:

        {
          "QueryType": "UPDATE",
          "Original": "update ambiguous_ref_with_source set done = true where id = 1;",
          "Instructions": {
            "OperatorType": "Update",
            "Variant": "Reference",
            "Keyspace": {
              "Name": "user",
              "Sharded": true
            },
            "TargetTabletType": "PRIMARY",
            "Query": "update ambiguous_ref_with_source set done = true where id = 1",
            "Table": "ambiguous_ref_with_source"
          },
          "TablesUsed": [
            "main.ambiguous_ref_with_source",
            "user.ambiguous_ref_with_source"
          ]
        }

This plan ultimately leads to a failure in the engine as we cannot execute updates with a reference OpCode: https://github.com/vitessio/vitess/blob/e3d2e89ab4d1b661ff935bbee79c4d6647e19621/go/vt/vtgate/engine/update.go#L65-L73

This PR makes sure we are setting the target keyspace correctly.

Must be backported to 20 and 21.

vitess-bot[bot] commented 1 week ago

Review Checklist

Hello reviewers! :wave: Please follow this checklist when reviewing this Pull Request.

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 67.39%. Comparing base (f6067e0) to head (30dae54). Report is 6 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #17226 +/- ## ========================================== - Coverage 67.40% 67.39% -0.02% ========================================== Files 1570 1570 Lines 252903 252934 +31 ========================================== - Hits 170460 170454 -6 - Misses 82443 82480 +37 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


🚨 Try these New Features:

systay commented 1 week ago

We should add an end-to-end test that asserts that this works well

frouioui commented 5 days ago

We should add an end-to-end test that asserts that this works well

Added via 30dae54