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

Remove writeoffset in txn table. #14553

Closed
wants to merge 2 commits into from

Conversation

fengttt
Copy link
Contributor

@fengttt fengttt commented Feb 5, 2024

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #14552

What this PR does / why we need it:

unnecessary caching of write offset is the root of all evils.

@mergify mergify bot added kind/bug Something isn't working kind/refactor Code refactor labels Feb 5, 2024
@matrix-meow matrix-meow added the size/XXL Denotes a PR that changes 2000+ lines label Feb 5, 2024
@fengttt fengttt marked this pull request as draft February 5, 2024 20:30
@fengttt fengttt marked this pull request as ready for review February 7, 2024 05:25
Split getTableWrites to two functions.

One can see its own writes, one cannot.  Ranges should not
see its own writes, but Rows(), which return affected rows
should.

This cannot be right.
@matrix-meow matrix-meow added size/S Denotes a PR that changes [10,99] lines size/M Denotes a PR that changes [100,499] lines and removed size/XXL Denotes a PR that changes 2000+ lines size/S Denotes a PR that changes [10,99] lines labels Feb 7, 2024
@fengttt fengttt force-pushed the fengttt-mo-14552 branch 2 times, most recently from 4439796 to 10ac705 Compare February 7, 2024 09:00
lots can go wrong.   mo_account wont be created.
@@ -729,12 +721,13 @@ func (tbl *txnTable) rangesOnePart(
}, uncommittedObjects...)
}

for _, entry := range tbl.writes {
// XXX By all means this should by calling ForEachTableWritesStable.
tbl.db.txn.ForEachTableWritesIncludingSelf(tbl.db.databaseId, tbl.tableId, func(entry *Entry) {
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not how it should be handled here. For the current SQL you just need to see the operation of the previous SQL. This operation may cause this SQL to see its own writes.

@fengttt fengttt closed this Feb 26, 2024
@fengttt fengttt deleted the fengttt-mo-14552 branch July 12, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working kind/refactor Code refactor size/M Denotes a PR that changes [100,499] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants