Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

backend: dynamically calculate the maximum auto-inc ID #227

Merged
merged 2 commits into from Aug 30, 2019

Conversation

kennytm
Copy link
Collaborator

@kennytm kennytm commented Aug 18, 2019

What problem does this PR solve?

Fix #222 (= TOOL-1472).

What is changed and how it works?

Remove the piece of code that pre-calculates the final AUTO_INCREMENT ID. Instead, let the encoder determines the actual value dynamically. This gives a tighter upper bound of the final AUTO_INCREMENT value and avoids overflowing AUTO_INCREMENT columns of shorter integer size (this doesn't work if PRIMARY KEY is not a handle column though, since _tidb_rowid will share the same AUTO_INCREMENT sequence)

Check List

Tests

  • Integration test

Side effects

Related changes

@kennytm kennytm added status/PTAL This PR is ready for review. Add this label back after committing new changes type/bug-fix Bug fix labels Aug 18, 2019
This makes sure the final AUTO_INCREMENT value is exactly the maximum value
of the auto-inc column. Fix #222.
@kennytm kennytm force-pushed the kennytm/dont-overflow-autoinc-if-provided branch from 7bd9d12 to 281ac50 Compare August 20, 2019 19:32
@kennytm
Copy link
Collaborator Author

kennytm commented Aug 20, 2019

/run-all-tests

@@ -179,6 +180,9 @@ func (kvcodec *tableKVEncoder) Encode(
return nil, logKVConvertFailed(logger, row, j, col.ToInfo(), err)
}
record = append(record, value)
if isAutoIncCol {
kvcodec.tbl.RebaseAutoID(kvcodec.se, value.GetInt64(), false)
Copy link
Contributor

@lance6716 lance6716 Aug 21, 2019

Choose a reason for hiding this comment

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

This RebaseAutoID is called at every row in my understanding, why could this
rebase-per-row gives tighter upper bound than rebase-chunk-max, I think they will eventually reach almost same auto-id(rebase4Unsigned, rebase4Signed, Rebase) and now it's more expensive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The change allowing a tighter bound is deleting

for _, engine := range cp.Engines {
for _, chunk := range engine.Chunks {
cp.AllocBase = mathutil.MaxInt64(cp.AllocBase, chunk.Chunk.RowIDMax)
}
}

since RowIDMax is normally much larger than the actual row number.

Yes this could become more expensive, but we should perform a measurement to ensure. I bet the CAS loop is still much cheaper than the actual KV encoding.

Copy link
Contributor

@lance6716 lance6716 Aug 22, 2019

Choose a reason for hiding this comment

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

Now i see rebased value is cast from rowId, which is smaller or equal than row's content. And I agree CAS is lightweight. I don't know if we could move this rebase-per-row outside, such as on chunk's EOF, so there's fewer repeat.

case io.EOF:
break outside

Copy link
Contributor

@lance6716 lance6716 Aug 29, 2019

Choose a reason for hiding this comment

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

CAS is lightweight so maybe we shouldn't bother moving it outside ⬆️

@kennytm
Copy link
Collaborator Author

kennytm commented Aug 29, 2019

PTAL @lance6716 @WangXiangUSTC

@lance6716
Copy link
Contributor

LGTM

@lance6716 lance6716 added status/LGT1 One reviewer already commented LGTM (LGTM1) and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Aug 29, 2019
Copy link
Collaborator

@IANTHEREAL IANTHEREAL left a comment

Choose a reason for hiding this comment

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

LGTM

@IANTHEREAL IANTHEREAL added status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) and removed status/LGT1 One reviewer already commented LGTM (LGTM1) labels Aug 30, 2019
@IANTHEREAL
Copy link
Collaborator

/run-all-tests

@kennytm kennytm merged commit d49fec6 into master Aug 30, 2019
@kennytm kennytm deleted the kennytm/dont-overflow-autoinc-if-provided branch August 30, 2019 03:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) type/bug-fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

why lightning change table auto_increment?为什么lightning会修改自增值的大小?
3 participants