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

core: Start system table ids at 1 #1544

Merged
merged 1 commit into from
Jul 30, 2024
Merged

core: Start system table ids at 1 #1544

merged 1 commit into from
Jul 30, 2024

Conversation

kim
Copy link
Contributor

@kim kim commented Jul 25, 2024

Start all autoinc id columns within the system schema with 1 instead of
0.

Zero is the sentinel value to increment the autoinc sequence when
inserting, so using 0 as an actual value can lead to confusion and
subtle bugs.

Expected complexity level and risk

1

@kim kim added the abi-break A PR that makes an ABI breaking change label Jul 25, 2024
@kim kim force-pushed the kim/1-based-system-table-ids branch from a1c8a2c to dc1b372 Compare July 26, 2024 08:37
@kim
Copy link
Contributor Author

kim commented Jul 26, 2024

Updated to start all autoinc columns of system tables at 1.

@kim kim removed the abi-break A PR that makes an ABI breaking change label Jul 26, 2024
@bfops
Copy link
Collaborator

bfops commented Jul 29, 2024

Wait, is this not an ABI break?

@bfops bfops added the release-any To be landed in any release window label Jul 29, 2024
@kim
Copy link
Contributor Author

kim commented Jul 30, 2024

@bfops Sorry, no, it's not. I initially thought it was, but we're not storing any id in the commitlog, and user-defined tables start at 4097 (not system tables + 1).

@kim kim force-pushed the kim/1-based-system-table-ids branch 2 times, most recently from a25475c to 7cb0dde Compare July 30, 2024 08:05
Start all autoinc id columns within the system schema with 1 instead of
0.

Zero is the sentinel value to increment the autoinc sequence when
inserting, so using 0 as an actual value can lead to confusion and
subtle bugs.
@kim kim force-pushed the kim/1-based-system-table-ids branch from 7cb0dde to f9fa796 Compare July 30, 2024 08:47
@kim kim added this pull request to the merge queue Jul 30, 2024
Merged via the queue into master with commit 21058e0 Jul 30, 2024
7 checks passed
@Centril Centril deleted the kim/1-based-system-table-ids branch July 31, 2024 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants