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

source-oracle-flashback: order first, then limit row count #2229

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

mdibaiee
Copy link
Member

@mdibaiee mdibaiee commented Dec 23, 2024

Description:

  • We were using ROWNUM to limit the number of rows, but that filter is applied before ordering, and that would lead to inconsistencies if ROWIDs were not in the order of insertion (sometimes ROWIDs can be reused if they have been freed up due to deletion / resize of data in data segments), a common pitfall of ROWNUM filtering which I fell for
  • This was not covered by our test cases because reproducing this behaviour is hard, we need ot somehow force the database to allocate an "older" ROWID to a new row
  • Instead of using ROWNUM filtering we can use FETCH NEXT %d ROWS ONLY which has been introduced in Oracle 12c (released 2013). An alternative implementation is using a subquery like SELECT * FROM (SELECT ...) WHERE ROWNUM < 50000 for older versions, but I'm not sure about its performance implications, and after talking with Dave we haven't had customers ask for Oracle versions below 12c yet. I put a note in the code for that so we can revisit if necessary.
  • I tried to write a test to emulate this scenario by deleting rows and inserting rows to see if the database would reuse a ROWID smaller than the largest ROWID, but I was not successful. I'm not sure how the database makes the decision yet (probably need to understand how large are the data segments and how much space needs to be freed before it will be re-allocated to newer rows)

Workflow steps:

(How does one use this feature, and how has it changed)

Documentation links affected:

(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)

Notes for reviewers:

(anything that might help someone review this PR)


This change is Reviewable

Copy link
Member

@psFried psFried left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'm assuming that none of our existing customers are using oracle < 12c

@mdibaiee mdibaiee merged commit 6c149fd into main Dec 23, 2024
74 of 81 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants