Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

sql/opt/exec: perform implicit SELECT FOR UPDATE on UPDATE with index join #45511

Closed
nvanbenschoten opened this issue Feb 27, 2020 · 2 comments · Fixed by #45733
Closed

sql/opt/exec: perform implicit SELECT FOR UPDATE on UPDATE with index join #45511

nvanbenschoten opened this issue Feb 27, 2020 · 2 comments · Fixed by #45733
Assignees
Labels
A-sql-optimizer SQL logical planning and optimizations. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@nvanbenschoten
Copy link
Member

We should be able to use implicit row-level locking on UPDATEs with an index-join in its initial row scan like the following, which perform no gateway-side filtering:

--- Schema
CREATE TABLE t (
       id INT8 NOT NULL DEFAULT unique_rowid(),
       instance_id INT8 NULL,
       is_active INT8 NULL,
       created_at TIMESTAMP NOT NULL,
       updated_at TIMESTAMP NOT NULL,
       CONSTRAINT "primary" PRIMARY KEY (id ASC),
       INDEX idx (instance_id ASC, is_active ASC)
);

--- Update
EXPLAIN UPDATE t SET is_active = 0, updated_at = now() WHERE (is_active = 1) AND (instance_id = 15);

            tree            |    field    |      description
----------------------------+-------------+------------------------
                            | distributed | false
                            | vectorized  | false
  count                     |             |
   └── update               |             |
        │                   | table       | t
        │                   | set         | is_active, updated_at
        │                   | strategy    | updater
        │                   | auto commit |
        └── render          |             |
             └── index-join |             |
                  │         | table       | t@primary
                  │         | key columns | id
                  └── scan  |             |
                            | table       | t@idx
                            | spans       | /15/1-/15/2

Currently, we only match on the following patterns:

// Try to match the pattern:
//
// (Update
// $input:(Scan $scanPrivate:*)
// $checks:*
// $mutationPrivate:*
// )
//
// Or
//
// (Update
// $input:(Project
// (Scan $scanPrivate:*)
// $projections:*
// $passthrough:*
// )
// $checks:*
// $mutationPrivate:*
// )
//

@nvanbenschoten nvanbenschoten added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-optimizer SQL logical planning and optimizations. labels Feb 27, 2020
@nvanbenschoten nvanbenschoten self-assigned this Feb 27, 2020
@andy-kimball
Copy link
Contributor

If we want to match increasingly complex patterns, we'll probably want to fold the matching and analysis into the explorer rather than execbuilder. When I first thought about doing this, I was worried about the cost of recreating the Update subtree just to set the locking bit. Opt trees are immutable, so setting a locking flag on an input Scan operator requires a new Scan operator to be created.

A different idea which lets us integrate with the explorer but avoids the cost of recreating subtrees is to put a locking flag in the mutation operator. This flag would mean, "lock every scan in the scope of this subtree". When the execbuilder sees this flag, it'd set forceForUpdateLocking like it does today. The only difference would be that the code to decide when to set that flag would live in the optimizer rather than in execbuilder, which makes for better packaging, since we'd rather not do complex analysis in execbuilder.

@andy-kimball
Copy link
Contributor

All that said, I'm not opposed to adding this new pattern directly to execbuilder for 20.1.

nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Mar 4, 2020
… join

Fixes cockroachdb#45511.

This commit enables implicit row-level locking on UPDATEs with an index-join
in its initial row scan. To support this, two new patterns were needed in
`Builder.shouldApplyImplicitLockingToUpdateInput`:

```
(Update
    $input:(IndexJoin
        $input:(Scan $scanPrivate:*)
        $indexJoinPrivate:*
    )
    $checks:*
    $mutationPrivate:*
)

AND

(Update
    $input:(Project
        $input:(IndexJoin
            $input:(Scan $scanPrivate:*)
            $indexJoinPrivate:*
        )
        $projections:*
        $passthrough:*
    )
    $checks:*
    $mutationPrivate:*
)
```

These extra patterns are pushing the limits of this form of pattern matching,
but this is ok for the next release. If/when we want to make any more general,
we should strongly consider Andy's proposal to fold the patten matching and
analysis into the explorer rather than execbuilder.
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Mar 4, 2020
… join

Fixes cockroachdb#45511.

This commit enables implicit row-level locking on UPDATEs with an index-join
in its initial row scan. To support this, two new patterns were needed in
`Builder.shouldApplyImplicitLockingToUpdateInput`:

```
(Update
    $input:(IndexJoin
        $input:(Scan $scanPrivate:*)
        $indexJoinPrivate:*
    )
    $checks:*
    $mutationPrivate:*
)

AND

(Update
    $input:(Project
        $input:(IndexJoin
            $input:(Scan $scanPrivate:*)
            $indexJoinPrivate:*
        )
        $projections:*
        $passthrough:*
    )
    $checks:*
    $mutationPrivate:*
)
```

These extra patterns are pushing the limits of this form of pattern matching,
but this is ok for the next release. If/when we want to make any more general,
we should strongly consider Andy's proposal to fold the patten matching and
analysis into the explorer rather than execbuilder.
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Mar 6, 2020
… join

Fixes cockroachdb#45511.

This commit enables implicit row-level locking on UPDATEs with an index-join
in its initial row scan. To support this, two new patterns were needed in
`Builder.shouldApplyImplicitLockingToUpdateInput`:

```
(Update
    $input:(IndexJoin
        $input:(Scan $scanPrivate:*)
        $indexJoinPrivate:*
    )
    $checks:*
    $mutationPrivate:*
)

AND

(Update
    $input:(Project
        $input:(IndexJoin
            $input:(Scan $scanPrivate:*)
            $indexJoinPrivate:*
        )
        $projections:*
        $passthrough:*
    )
    $checks:*
    $mutationPrivate:*
)
```

These extra patterns are pushing the limits of this form of pattern matching,
but this is ok for the next release. If/when we want to make any more general,
we should strongly consider Andy's proposal to fold the patten matching and
analysis into the explorer rather than execbuilder.
RichardJCai pushed a commit to RichardJCai/cockroach that referenced this issue Mar 9, 2020
… join

Fixes cockroachdb#45511.

This commit enables implicit row-level locking on UPDATEs with an index-join
in its initial row scan. To support this, two new patterns were needed in
`Builder.shouldApplyImplicitLockingToUpdateInput`:

```
(Update
    $input:(IndexJoin
        $input:(Scan $scanPrivate:*)
        $indexJoinPrivate:*
    )
    $checks:*
    $mutationPrivate:*
)

AND

(Update
    $input:(Project
        $input:(IndexJoin
            $input:(Scan $scanPrivate:*)
            $indexJoinPrivate:*
        )
        $projections:*
        $passthrough:*
    )
    $checks:*
    $mutationPrivate:*
)
```

These extra patterns are pushing the limits of this form of pattern matching,
but this is ok for the next release. If/when we want to make any more general,
we should strongly consider Andy's proposal to fold the patten matching and
analysis into the explorer rather than execbuilder.
RichardJCai pushed a commit to RichardJCai/cockroach that referenced this issue Mar 9, 2020
… join

Fixes cockroachdb#45511.

This commit enables implicit row-level locking on UPDATEs with an index-join
in its initial row scan. To support this, two new patterns were needed in
`Builder.shouldApplyImplicitLockingToUpdateInput`:

```
(Update
    $input:(IndexJoin
        $input:(Scan $scanPrivate:*)
        $indexJoinPrivate:*
    )
    $checks:*
    $mutationPrivate:*
)

AND

(Update
    $input:(Project
        $input:(IndexJoin
            $input:(Scan $scanPrivate:*)
            $indexJoinPrivate:*
        )
        $projections:*
        $passthrough:*
    )
    $checks:*
    $mutationPrivate:*
)
```

These extra patterns are pushing the limits of this form of pattern matching,
but this is ok for the next release. If/when we want to make any more general,
we should strongly consider Andy's proposal to fold the patten matching and
analysis into the explorer rather than execbuilder.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-optimizer SQL logical planning and optimizations. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants