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

ddl: check allocator's existence before allocate/rebase #18932

Merged
merged 4 commits into from
Aug 10, 2020

Conversation

tangenta
Copy link
Contributor

@tangenta tangenta commented Aug 3, 2020

What problem does this PR solve?

Issue Number: close #18931

Problem Summary:

After #18326, rowid allocator is not necessarily existed in every table. Before allocate/rebase values, a nil check for allocators is missed.

What is changed and how it works?

What's Changed and how it works: add a nil check for allocators before using them.

Related changes

Check List

Tests

  • Unit test
  • Integration test

Side effects: N/A

Release note

  • No release note

@tangenta tangenta added the sig/sql-infra SIG: SQL Infra label Aug 3, 2020
@codecov
Copy link

codecov bot commented Aug 3, 2020

Codecov Report

Merging #18932 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #18932   +/-   ##
===========================================
  Coverage   79.4424%   79.4424%           
===========================================
  Files           547        547           
  Lines        149551     149551           
===========================================
  Hits         118807     118807           
  Misses        21252      21252           
  Partials       9492       9492           

}
} else {
if err = tb.RebaseAutoID(nil, tbInfo.AutoRandID-1, false, tp); err != nil {
if alloc := allocs.Get(tp); alloc != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the manual rebase will be just ignored when alloc==nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This is consistent with MySQL.

Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 4, 2020
go.mod Outdated Show resolved Hide resolved
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REST LGTM

Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Aug 4, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 4, 2020
@tangenta
Copy link
Contributor Author

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 10, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit e46a222 into pingcap:master Aug 10, 2020
tangenta added a commit to ti-srebot/tidb that referenced this pull request May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create table with table option 'auto_increment' reports nil pointer dereference
4 participants