meta,structure: modify structure hash API to avoid conflict on auto table ID key #25221
Conversation
ti-chi-bot
added
the
size/M
structure/hash.go
Outdated
| @@ -145,6 +156,11 @@ func (t *TxStructure) updateHash(key []byte, field []byte, fn func(oldValue []by | |||
|
|
|||
| // HLen gets the number of fields in a hash. | |||
| func (t *TxStructure) HLen(key []byte) (int64, error) { | |||
| if t.ignoreHashMeta { | |||
There was a problem hiding this comment.
If TiDB doesn't use HLen, how about remove all logic about it?
There was a problem hiding this comment.
I worry that someone could call HLen in future.
There was a problem hiding this comment.
HLen API is reasonable, you can find the structure package provides API similar to Redis, and Redis also has HLen.
This package design is general, it can be used by outside projects.
I think it is better to provide an option instead of limit its ability to TiDB only.
There was a problem hiding this comment.
Without this PR, it means whether you use HLen or not, you pay for the cost.
With this PR, it means you can choose to pay for exactly what you want.
If you wipe out HLen, it means this functionality is missing, you don't have choice.
I prefer providing flexibility to users, and let them do their choices.
|
Rest LGTM. Thank you very much! I can continue my test based on it. |
|
@hicqu: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
structure/hash.go
Outdated
| @@ -127,6 +134,10 @@ func (t *TxStructure) updateHash(key []byte, field []byte, fn func(oldValue []by | |||
| return errors.Trace(err) | |||
| } | |||
|
|
|||
| if t.ignoreHashMeta { | |||
There was a problem hiding this comment.
When TiDB downgrades to an older version that reads and writes hash meta, but now the hash meta is all wrong, what will happen?
There was a problem hiding this comment.
It doesn't matter, even in the downgrade case the data is wrong, old TiDB never use those data.
There was a problem hiding this comment.
I don't think so. For example, HDel checks meta hash, and if meta.IsEmpty(), it will report an error.
There was a problem hiding this comment.
So we can highlight the downgrade compatibility? there are many new feature, if TiDB upgrade and use the new feature, then downgrade, there will be a problem. Any suggestion?
|
Why not remove HLen directly? Do |
|
Since so many of you dislike the PTAL @AilinKid @djshow832 |
ti-chi-bot
added
size/L
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
status/LGT2
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: bc88557
|
ti-chi-bot
added
the
status/can-merge
|
/run-integration-common-test |
|
@tiancaiamao Ah, I find this is a breaking change so it can be hard to backport... |
What problem does this PR solve?
Issue Number: close #25197
Problem Summary:
Insert into different tables should not conflict on the table meta key, but it does!
GenAutoTableID()use the structureHIncAPI, that API is not scalable because the hash structure maintain a meta key to store the hash table size.What is changed and how it works?
What's Changed:
Provide a
IgnoreHashMetaoption for the structure API.How it Works:
If
IgnoreHashMetais called, the structure API will ignore the hash related things.That means
HLenAPI is not available any more with this option.TiDB do not use
HLen, so it's set to get rid of the meta key conflict.Check List
Tests
Release note