SQL server: consistent snapshot and lsn selection #32979
Conversation
martykulma
force-pushed
the
sql-server-resume-lsn
branch
3 times, most recently
from
8ca2ca9 to
ba8b692
Compare
martykulma
changed the title
martykulma
force-pushed
the
sql-server-resume-lsn
branch
3 times, most recently
from
a4eb8f0 to
e502e99
Compare
martykulma
force-pushed
the
sql-server-resume-lsn
branch
from
e502e99 to
831bb01
Compare
| @@ -140,13 +181,57 @@ impl<'a> CdcStream<'a> { | |||
| // the upstream DB is ready for CDC. | |||
| self.wait_for_ready().await?; | |||
|
|
|||
| tracing::info!("Upstream is ready"); | |||
There was a problem hiding this comment.
nit: might want to add some additional context here and also are we sure we want this at info level?
There was a problem hiding this comment.
ah I see! would we ever see these concurrently on the same node? would it be clear which database is ready?
There was a problem hiding this comment.
🤦 - just realized i didn't include ?tables
| tracing::trace!(%schema, %table, "locking table"); | ||
| fence_txn.lock_table_exclusive(&*schema, &*table).await?; | ||
| } | ||
| tracing::info!(?tables, "Locked tables"); |
There was a problem hiding this comment.
nit: same comment as above.
| let mut fencing_client = self.client.new_connection().await?; | ||
| let mut fence_txn = fencing_client.transaction().await?; | ||
|
|
||
| // TODO (maz): we should consider a timeout or a lock + snapshot per-table instead of collectively |
There was a problem hiding this comment.
what telemetry do we need to be able to make decision about the necessity of locking per table?
I would guess we need to know whether the table would have been locked for less time that way?
There was a problem hiding this comment.
follow up: maybe we can batch with a configurable number of tables per batch? we might want to track with a histogram/metric roughly how long we are locking a tables.
seems like this only becomes a problem if there's a very large number of tables or a very slow lock acquisition.
The current mechanism for snapshot and LSN selection is not correct due to the asynchronous nature of MS SQL CDC and the implementation details of
SNAPSHOTisolation.This implementation is similar to the MySQL implementation in that it locks the tables for a brief period to establish a point in time where a consistent view of the DB can be observed. Once the
LSNand transaction sequence number (XSN), the table locks are dropped, and we have a consistent version of the table(s) to read from as well as a correctLSNto process CDC events.Motivation
Fixes https://github.com/MaterializeInc/database-issues/issues/9476
Tips for reviewer
You can look at the commits to see the history, but the actual fix is not until the end.
I didn't squash my commits because the bug that this fixes was created in response to this PR (at the time a draft that was just a test change to show the race condition). It will get squashed on merge!
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.