DBZ-947 SQL Server snapshot.locking.modes overhaul

[grzegorz8]

https://issues.jboss.org/browse/DBZ-947

Exclusive mode: In order to provide a consistent snapshot, exclusive locks
are taken on all monitored tables during entire snapshot process duration.

Snapshot mode: The initial snapshot is executed in snapshot transaction
isolation level. This guarantees consistent snapshot as long as DDL
statements are not executed. In addition, neither table locks nor
row-level locks are acquired.

Repeatable read mode: The initial snapshot is executed in repeatable read
transaction level. Since phantom reads can occur, it does not fully
guarantee consistency.

None mode: Neither table locks nor row-level locks are taken. This way
other transactions are not affected by initial snapshot process.
However, snapshot consistency is not guaranteed. In addition, DDL
statements must not be executed during the snapshot.

[grzegorz8]

Related pull request: #36

[grzegorz8]

Taking locks here is misleading, since it suggests they are taken only for schema snapshot. I believe we should introduce yet another template method.

[gunnarmorling]

Yes, that's a good point. Can you file a JIRA issue so we can track it separately?

[gunnarmorling]

Hum, no, I think I'm taking that back. Before this PR, these locks should indeed only have been held while taking the schema snapshot, by virtue of a savepoint. Which makes me wonder why you removed it here?

[grzegorz8]

Rollback to savepoint makes no sense.
In both SNAPSHOT and R_R mode no locks are taken - no rollback needed.
For EXCLUSIVE, we want to keep locks until the end of transaction - rollback must not be executed.

[grzegorz8]

I meant that taking exclusive lock in lockTablesForSchemaSnapshot is misleading.

[gunnarmorling]

For EXCLUSIVE, we want to keep locks until the end of transaction - rollback must not be executed.

I think the original idea was to keep the lock only for the time of taking the schema snapshot. At least that was my motivation for the Oracle connector, which is where @jpechane copied this from. Then the actual data snapshot would be taken using R_R semantics, which indeed seems to be missing still, when looking at the master branch. @jpechane Correct me please, if I'm wrong.

[grzegorz8]

Yes, on master in EXCLUSIVE data snapshot is taken in database default isolation level.

I suggested to keep the locks during data snapshot as well due to this scenario: https://issues.jboss.org/browse/DBZ-947?focusedCommentId=13651038&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13651038

[grzegorz8]

To be honest, even having seen the logs, I still have no idea why the build failed.

[jpechane]

@grzegorz8 Unfortunately the build here will fail till the related PR gets merged.

[jpechane]

@grzegorz8 There is a conflict now after #36 was applied :-(

[grzegorz8]

@jpechane Rebase done.

[gunnarmorling]

Hey @grzegorz8, where are we with this one? On first thought I'm having doubts of using READ_COMITTED for snaphotting, as it may lead to concurrent changes being missed. What's the motivation for adding this in addition to REPEATABLE_READ?

[grzegorz8]

Hey @grzegorz8, where are we with this one? On first thought I'm having doubts of using READ_COMITTED for snaphotting, as it may lead to concurrent changes being missed. What's the motivation for adding this in addition to REPEATABLE_READ?

I'm waiting for review and suggestions how to structure the code (e.g. do we need to introduce a new template method for taking locks for the entire snapshot, not only for schema snapshot?).

Regarding isolation level, I know that Debezium tries to deliver consistent snapshot, which in fact is feasible with neither READ COMMITTED nor REPEATABLE READ in case of SQL Server. I presented an example for RR here: https://issues.jboss.org/browse/DBZ-947?focusedCommentId=13651038&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13651038
My concern was the fact that "none" mode in current form is misleading. It blocks other transactions from making updates, since in repeatable read isolation level row shared locks are taken until the end of transaction. So I concluded, that if we cannot deliver consistency, we should at least deliver truly "none" locking mode with "eventual consistency".

In addition, I encountered cases when blocking other transactions is unacceptable for DB owner, so read committed fits this case well.

[grzegorz8]

@jpechane @gunnarmorling The PR has been updated according to the suggestions here https://issues.jboss.org/browse/DBZ-947?focusedCommentId=13658596&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13658596

[gunnarmorling]

@grzegorz8 A few more comments inline. My key questions are:

• Do why need to keep NONE at all?
• Should CONSISTENT be renamed to REPEATABLE_READ and be the new default?

My answers would be "no", "yes" and "yes" :)

[gunnarmorling]

Yes, that's a good point. Can you file a JIRA issue so we can track it separately?

[grzegorz8]

@gunnarmorling

Do why need to keep NONE at all?

I'm not insisting. I'll remove it.

Should CONSISTENT be renamed to REPEATABLE_READ and be the new default?

I'll do that.

[gunnarmorling]

I'm not insisting. I'll remove it.

Was just talking to @jpechane a bit, and also re-read your comment above about REPEATABLE READ keeping locks. So I can see why someone wanted to use READ COMITTED, provided they are aware of the implications. So we can keep it, I'd just suggest to rename it into READ_COMMITTED then. And then it'd be a good idea to rename the option and enum into snapshot.isolation.mode.

[grzegorz8]

@gunnarmorling I believe I have solved all your suggestions (except Can you file a JIRA issue so we can track it separately?). Are there any issues with this PR?

[gunnarmorling]

Hey @grzegorz8, nope, no issues I think, I only was busy with some conference travel and other work recently. I'll get back to this one next week. Unless anything else pops up, the change will be part of the next 0.9.x release. Sorry for the delay and thanks your patience.

[grzegorz8]

Unfortunately, I haven't understood deep enough how read committed works before. It DOES take shared locks, but on very short periods. Deadlocks might occur, though.

org.apache.kafka.connect.errors.ConnectException: Snapshotting of table XXX.XXX failed
       at io.debezium.relational.HistorizedRelationalSnapshotChangeEventSource.createDataEventsForTable(HistorizedRelationalSnapshotChangeEventSource.java:315)
       at io.debezium.relational.HistorizedRelationalSnapshotChangeEventSource.createDataEvents(HistorizedRelationalSnapshotChangeEventSource.java:259)
       at io.debezium.relational.HistorizedRelationalSnapshotChangeEventSource.execute(HistorizedRelationalSnapshotChangeEventSource.java:132)
       at io.debezium.pipeline.ChangeEventSourceCoordinator.lambda$start$0(ChangeEventSourceCoordinator.java:77)
       at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
       at java.util.concurrent.FutureTask.run(FutureTask.java:266)
       at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
       at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
       at java.lang.Thread.run(Thread.java:745)
Caused by: com.microsoft.sqlserver.jdbc.SQLServerException: Transaction (Process ID 2316) was deadlocked on lock resources with another process and has been chosen as the deadlock victim. Rerun the transaction.
       at com.microsoft.sqlserver.jdbc.SQLServerException.makeFromDatabaseError(SQLServerException.java:259)
       at com.microsoft.sqlserver.jdbc.SQLServerResultSet$FetchBuffer.nextRow(SQLServerResultSet.java:6388)
       at com.microsoft.sqlserver.jdbc.SQLServerResultSet.fetchBufferNext(SQLServerResultSet.java:1647)
       at com.microsoft.sqlserver.jdbc.SQLServerResultSet.next(SQLServerResultSet.java:988)
       at io.debezium.relational.HistorizedRelationalSnapshotChangeEventSource.createDataEventsForTable(HistorizedRelationalSnapshotChangeEventSource.java:287)
       ... 8 more

I need to rethink usefulness of none mode :(

[jpechane]

I woonder how deadlock is possible - is it because we are snapshotting two tables and another TX is accessing those two tables but in the opposite order?

[grzegorz8]

@jpechane In this case I was fetching one huge table only (it failed after ~4 hours). I've read that while reading table in read committed a read lock or either individual row or a page is acquired. I'm trying now to track down the exact scenario.

[gunnarmorling]

Hey @grzegorz8 and @jpechane, it seems this one still needs some more work? Anything specific we should discuss?

[jpechane]

@gunnarmorling Yeah, I'd still give it a couple of more rounds. If the locks are on raw/page level then even on table level if a concurrent process will read two rows in different order than Debezium we can end in a deadlock.

[gunnarmorling]

Ok, thanks for the heads-up. I'll assign the issue to 0.9-next then.

[grzegorz8]

@gunnarmorling I doubt we can do anything about deadlocks. We can either decide to get rid of read_committed support or we can leave it and document it properly.

If we leave it, we should document tradeoffs between consistency and locking of each mode and mention that the best choice is SNAPSHOT isolation level and if it's not an option for a user, the user should accept disadvantages of other isolation level modes.

[jpechane]

Hi, could you please look at locking hints (https://docs.microsoft.com/en-us/sql/t-sql/queries/hints-transact-sql-table?view=sql-server-2017) - maybe TABLOCK would help to mitigate the issue. Also as you mentioned, in thius conetext lociknig mode NONE might mave sense again.

[grzegorz8]

@gunnarmorling @jpechane Let's try to conclude what to do with this :)

1. Now I think isolation level read_committed makes no sense. a) It is unreliable (deadlocks). b) adding a table lock contradicts with what I hoped to achieve with this isolation level. read_committed with table lock brings no benefit comparing to repeatable_read.
2. I'd propose to keep the name snapshot.isolation.mode with values exclusive, snapshot, repeatable_read.
3. What was additionally fixed here:
   a) rollback to a savepoint was removed. In exclusive mode we need to keep lock until the end of transaction. For snapshot and repeatable_read rollback to a savepoint has no effect in fact.
   b) Once the snapshot is finished, the default isolation level (read committed) is set.

[jpechane]

@grzegorz8 @gunnarmorling Based up the current observations I think dropping read_committed is fine, I'd still provide none for edge cases. Naming is fine for me.

[grzegorz8]

@jpechane How should none look like? I believe repeatable_read has the same semantics as the old none.

[jpechane]

@grzegorz8 none would be read _uncomitted.

[grzegorz8]

@jpechane Ok, I'll add that. Should I name it none or read_uncommitted?

[grzegorz8]

@jpechane @gunnarmorling How about this PR. If the overall idea of the PR is not good for you, let me create separate tasks (and PRs) for bugs fixes here:
a) rollback to a savepoint was removed. In exclusive mode we need to keep lock until the end of transaction. For snapshot and repeatable_read rollback to a savepoint has no effect in fact.
b) Once the snapshot is finished, the default isolation level (read committed) is set.

[jpechane]

@grzegorz8 Nobody is saying that the overall idea is not good. I understand this takes a little bit long already but as you've pointed before - not everything works as we all expected. To speed things up how baout having a chat or hangout session where we agree upon final solution? I think it is better than pushing back and forth. @gunnarmorling Do you agree?

[grzegorz8]

@jpechane I understand this is rather low-priority task if we consider other improvements, bugs or upcoming release candidate.
A quick chat is fine for me. We can schedule it at your convenience, once you have more free time :)

[gunnarmorling]

Yes, good idea with having a chat, so to agree on the way forward. And sorry for the delay, there's just (too) much going on. Would Friday morning, e.g. 10:00 a.m. CET, work for the two of you?

[jpechane]

@gunnarmorling Fine for me!

[grzegorz8]

Could it be 10:30, please?

[gunnarmorling]

Yes, of course. I'll send an invite with a Google Hangouts link.

[grzegorz8]

@gunnarmorling Thanks!

[gunnarmorling]

Sent an invite to your Google address.

[gunnarmorling]

@grzegorz8 Can you close this PR and open a new one against the main repository (after tomorrow's call, I reckon)? In the course of moving the SQL Server connector out of "Incubating" state, we've moved the connector over there. Thanks!