Skip to content

changefeedccl: fix rangefeed resume off-by-one timestamp#79525

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
samiskin:rangefeed-resume-off-by-one
Apr 8, 2022
Merged

changefeedccl: fix rangefeed resume off-by-one timestamp#79525
craig[bot] merged 1 commit intocockroachdb:masterfrom
samiskin:rangefeed-resume-off-by-one

Conversation

@samiskin
Copy link
Copy Markdown
Contributor

@samiskin samiskin commented Apr 6, 2022

Resolves #78993

Fixes a small off-by-one error in RangeFeedCheckpoint handling

Release justification: low risk off-by-one bug fix

Release note: None

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@samiskin samiskin marked this pull request as ready for review April 6, 2022 18:45
@samiskin samiskin requested a review from a team as a code owner April 6, 2022 18:45
@samiskin samiskin requested a review from miretskiy April 6, 2022 18:45
Resolves cockroachdb#78993

Fixes a small off-by-one error in RangeFeedCheckpoint handling

Release justification: low risk off-by-one bug fix

Release note: None
@samiskin samiskin force-pushed the rangefeed-resume-off-by-one branch from cd060ad to e65f89a Compare April 8, 2022 16:48
@samiskin
Copy link
Copy Markdown
Contributor Author

samiskin commented Apr 8, 2022

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Apr 8, 2022

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Apr 8, 2022

Build succeeded:

@craig craig bot merged commit 237b8c0 into cockroachdb:master Apr 8, 2022
ajwerner added a commit to ajwerner/cockroach that referenced this pull request Nov 2, 2022
It turns out that two commits occurred about two months apart to address some
off-by-one errors due to disagreements regarding the inclusivity or exclusivity
of bounds of time intervals. In cockroachdb#79525 we added a next call to compensate for
the catch-up scan occurring at an inclusive time. In cockroachdb#82451 we made the catch-
up scan act exclusively, like the rest of the kvserver code has assumed. The
end result is that we now actually do the catch up scan one tick later than
we had intended.

This resulted in some flakey tests, and in cases where the closed timestamp
pushed a writing transaction, may have resulted in missing rows. This was
uncovered deflaking cockroachdb#90764. With some added logging we see:

```
I221102 01:31:44.444557 1509 kv/kvclient/kvcoord/dist_sender_rangefeed.go:667  [nsql1,rangefeed=lease,dest_n=1,dest_s=1,dest_r=53] 3882  RangeFeedEvent: span:<key:"\376\222\213" end_key:"\376\222\214" > resolved_ts:<wall_time:166735270430458388 >
E221102 01:31:44.445042 1509 kv/kvclient/kvcoord/dist_sender_rangefeed.go:653  [nsql1,rangefeed=lease,dest_n=1,dest_s=1,dest_r=53] 3886  RangeFeedError: retry rangefeed (REASON_RANGE_SPLIT)
I221102 01:31:44.480676 2388 sql/internal.go:1321  [nsql1,job=810294652971450369,scExec,id=106,mutation=1] 3947  txn committed at 1667352704.380458388,1
I221102 01:31:44.485558 1509 kv/kvclient/kvcoord/dist_sender_rangefeed.go:420  [nsql1,rangefeed=lease] 3965  RangeFeed /Tenant/10/Table/{3-4} disconnected with last checkpoint 105.097693ms ago: retry rangefeed (REASON_RANGE_SPLIT)
```

Notice that the commit for the schema change occurred at
`1667352704.380458388,1` and the resolved event was at `1667352704.380458388`.
As the code was before, we'd perform the catch-up scan at
`1667352704.380458388,2` and miss the write we needed to see.

Fixes cockroachdb#90764.

Release note (bug fix): Fixed a bug which, in rare cases, could result in a
changefeed missing rows which occur around the time of a split in writing
transactions which take longer than the closed timestamp target duration
(defaults to 3s).
craig bot pushed a commit that referenced this pull request Nov 2, 2022
91019: sqlliveness: encode region in session id r=JeffSwenson a=JeffSwenson

Encode a region enum in the sqlliveness session id. The region will be used to support converting the sqlliveness and sql_instances table to regional by row tables.

This change creates a custom encoding for the session id. The encoding is convenient, as it allows adding a region to the session id without requiring modifications to the jobs table or the
crdb_internal.sql_liveness_is_alive built in.

The enum.One value is a work around for the fact the system database does not include a region enum by default. In the absence of a region enum, enum.One will be used in the session.

Part of #85736

Release note: None

91116: kvcoord: DistSender rangefeed bookkeeping had an off-by-one r=ajwerner a=ajwerner

It turns out that two commits occurred about two months apart to address some off-by-one errors due to disagreements regarding the inclusivity or exclusivity of bounds of time intervals. In #79525 we added a next call to compensate for the catch-up scan occurring at an inclusive time. In #82451 we made the catch- up scan act exclusively, like the rest of the kvserver code has assumed. The end result is that we now actually do the catch up scan one tick later than we had intended.

This resulted in some flakey tests, and in cases where the closed timestamp pushed a writing transaction, may have resulted in missing rows. This was uncovered deflaking #90764. With some added logging we see:

```
I221102 01:31:44.444557 1509 kv/kvclient/kvcoord/dist_sender_rangefeed.go:667  [nsql1,rangefeed=lease,dest_n=1,dest_s=1,dest_r=53] 3882  RangeFeedEvent: span:<key:"\376\222\213" end_key:"\376\222\214" > resolved_ts:<wall_time:166735270430458388 >
E221102 01:31:44.445042 1509 kv/kvclient/kvcoord/dist_sender_rangefeed.go:653  [nsql1,rangefeed=lease,dest_n=1,dest_s=1,dest_r=53] 3886  RangeFeedError: retry rangefeed (REASON_RANGE_SPLIT)
I221102 01:31:44.480676 2388 sql/internal.go:1321  [nsql1,job=810294652971450369,scExec,id=106,mutation=1] 3947  txn committed at 1667352704.380458388,1
I221102 01:31:44.485558 1509 kv/kvclient/kvcoord/dist_sender_rangefeed.go:420  [nsql1,rangefeed=lease] 3965  RangeFeed /Tenant/10/Table/{3-4} disconnected with last checkpoint 105.097693ms ago: retry rangefeed (REASON_RANGE_SPLIT)
```

Notice that the commit for the schema change occurred at `1667352704.380458388,1` and the resolved event was at `1667352704.380458388`. As the code was before, we'd perform the catch-up scan at `1667352704.380458388,2` and miss the write we needed to see.

Fixes #90764.

Release note (bug fix): Fixed a bug which, in rare cases, could result in a changefeed missing rows which occur around the time of a split in writing transactions which take longer than the closed timestamp target duration (defaults to 3s).

Co-authored-by: Jeff Swenson <swenson@cockroachlabs.com>
Co-authored-by: Andrew Werner <awerner32@gmail.com>
blathers-crl bot pushed a commit that referenced this pull request Nov 11, 2022
It turns out that two commits occurred about two months apart to address some
off-by-one errors due to disagreements regarding the inclusivity or exclusivity
of bounds of time intervals. In #79525 we added a next call to compensate for
the catch-up scan occurring at an inclusive time. In #82451 we made the catch-
up scan act exclusively, like the rest of the kvserver code has assumed. The
end result is that we now actually do the catch up scan one tick later than
we had intended.

This resulted in some flakey tests, and in cases where the closed timestamp
pushed a writing transaction, may have resulted in missing rows. This was
uncovered deflaking #90764. With some added logging we see:

```
I221102 01:31:44.444557 1509 kv/kvclient/kvcoord/dist_sender_rangefeed.go:667  [nsql1,rangefeed=lease,dest_n=1,dest_s=1,dest_r=53] 3882  RangeFeedEvent: span:<key:"\376\222\213" end_key:"\376\222\214" > resolved_ts:<wall_time:166735270430458388 >
E221102 01:31:44.445042 1509 kv/kvclient/kvcoord/dist_sender_rangefeed.go:653  [nsql1,rangefeed=lease,dest_n=1,dest_s=1,dest_r=53] 3886  RangeFeedError: retry rangefeed (REASON_RANGE_SPLIT)
I221102 01:31:44.480676 2388 sql/internal.go:1321  [nsql1,job=810294652971450369,scExec,id=106,mutation=1] 3947  txn committed at 1667352704.380458388,1
I221102 01:31:44.485558 1509 kv/kvclient/kvcoord/dist_sender_rangefeed.go:420  [nsql1,rangefeed=lease] 3965  RangeFeed /Tenant/10/Table/{3-4} disconnected with last checkpoint 105.097693ms ago: retry rangefeed (REASON_RANGE_SPLIT)
```

Notice that the commit for the schema change occurred at
`1667352704.380458388,1` and the resolved event was at `1667352704.380458388`.
As the code was before, we'd perform the catch-up scan at
`1667352704.380458388,2` and miss the write we needed to see.

Fixes #90764.

Release note (bug fix): Fixed a bug which, in rare cases, could result in a
changefeed missing rows which occur around the time of a split in writing
transactions which take longer than the closed timestamp target duration
(defaults to 3s).
blathers-crl bot pushed a commit that referenced this pull request Nov 11, 2022
It turns out that two commits occurred about two months apart to address some
off-by-one errors due to disagreements regarding the inclusivity or exclusivity
of bounds of time intervals. In #79525 we added a next call to compensate for
the catch-up scan occurring at an inclusive time. In #82451 we made the catch-
up scan act exclusively, like the rest of the kvserver code has assumed. The
end result is that we now actually do the catch up scan one tick later than
we had intended.

This resulted in some flakey tests, and in cases where the closed timestamp
pushed a writing transaction, may have resulted in missing rows. This was
uncovered deflaking #90764. With some added logging we see:

```
I221102 01:31:44.444557 1509 kv/kvclient/kvcoord/dist_sender_rangefeed.go:667  [nsql1,rangefeed=lease,dest_n=1,dest_s=1,dest_r=53] 3882  RangeFeedEvent: span:<key:"\376\222\213" end_key:"\376\222\214" > resolved_ts:<wall_time:166735270430458388 >
E221102 01:31:44.445042 1509 kv/kvclient/kvcoord/dist_sender_rangefeed.go:653  [nsql1,rangefeed=lease,dest_n=1,dest_s=1,dest_r=53] 3886  RangeFeedError: retry rangefeed (REASON_RANGE_SPLIT)
I221102 01:31:44.480676 2388 sql/internal.go:1321  [nsql1,job=810294652971450369,scExec,id=106,mutation=1] 3947  txn committed at 1667352704.380458388,1
I221102 01:31:44.485558 1509 kv/kvclient/kvcoord/dist_sender_rangefeed.go:420  [nsql1,rangefeed=lease] 3965  RangeFeed /Tenant/10/Table/{3-4} disconnected with last checkpoint 105.097693ms ago: retry rangefeed (REASON_RANGE_SPLIT)
```

Notice that the commit for the schema change occurred at
`1667352704.380458388,1` and the resolved event was at `1667352704.380458388`.
As the code was before, we'd perform the catch-up scan at
`1667352704.380458388,2` and miss the write we needed to see.

Fixes #90764.

Release note (bug fix): Fixed a bug which, in rare cases, could result in a
changefeed missing rows which occur around the time of a split in writing
transactions which take longer than the closed timestamp target duration
(defaults to 3s).
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.

cdc,kv: Correctly track resume timestamp in rangefeed

4 participants