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

postgres watch: checkpoints should move the high watermark revision #2140

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

vroldanbet
Copy link
Contributor

In #2139 we introduced the emission of checkpoint RevisionChanges when the postgres snapshot moves up out of band (outside of application changes).

The fix missed to also move the locally-cached high watermark revision. As a consequence, on a PG-backed SpiceDB were the pg snapshot does not have an associated SpiceDB transaction, the Watch API will emit checkpoints at the same revision over and over, because its own internal high watermark hasn't moved.

This changes the code so that currentTxn, which keeps track of the last checked revision, moves as well with an out-of-band snapshot revision.

@vroldanbet vroldanbet requested a review from a team as a code owner November 25, 2024 11:53
@vroldanbet vroldanbet self-assigned this Nov 25, 2024
@github-actions github-actions bot added the area/datastore Affects the storage system label Nov 25, 2024
@vroldanbet vroldanbet force-pushed the pg-move-current-transaction-on-checkpoints branch from e3102a9 to c5298e6 Compare November 25, 2024 13:30
@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Nov 25, 2024
@vroldanbet vroldanbet enabled auto-merge November 25, 2024 13:31
@vroldanbet vroldanbet force-pushed the pg-move-current-transaction-on-checkpoints branch 3 times, most recently from b7a7bec to 0ac4956 Compare November 25, 2024 14:16
In #2139 we introduced
the emission of checkpoint RevisionChanges when the postgres
snapshot moves up out of band (outside of application changes).

The fix missed to also move the locally-cached high watermark revision.
As a consequence, on a PG-backed SpiceDB were the pg snapshot does not
have an associated SpiceDB transaction, the Watch API will emit checkpoints
at the same revision over and over, because its own internal high watermark
hasn't moved.

This changes the code so that `currentTxn`, which keeps track of the last
checked revision, moves as well with an out-of-band snapshot revision.
@vroldanbet vroldanbet force-pushed the pg-move-current-transaction-on-checkpoints branch from 0ac4956 to 385542c Compare November 25, 2024 15:33
Copy link
Member

@josephschorr josephschorr left a comment

Choose a reason for hiding this comment

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

LGTM

@vroldanbet vroldanbet added this pull request to the merge queue Nov 25, 2024
Merged via the queue into main with commit 7877329 Nov 25, 2024
22 checks passed
@vroldanbet vroldanbet deleted the pg-move-current-transaction-on-checkpoints branch November 25, 2024 15:54
@github-actions github-actions bot locked and limited conversation to collaborators Nov 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/datastore Affects the storage system area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants