"FOR UPDATE" sql command is in the wrong place

[santiagoguinle]

"FOR UPDATE" is not compatible with SQL SERVER or other several DB, this should be erased from here and use another strategy for locking rows

druid/server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java

Line 476 in d5e8d4d

"SELECT %1$s FROM %2$s WHERE %3$s = :key FOR UPDATE",

[FrankChen021]

I'm not familiar with SQLServer, according to its document, it seems that with(rowlock) in SQLServer is equivalent to FOR UPDATE in MySQL, right ?

[santiagoguinle]

@FrankChen021 thanks for your answer! i am not also a SQLServer expert but i found some explanations about this issue

Acording to this: https://stackoverflow.com/questions/1483725/select-for-update-with-sql-server

An aproximated solution for "FOR UPDATE" in SQLServer is using "WITH (UPDLOCK)" but in fact if you want to just lock some specific rows then you need to define an index and use it on the same SQL expression.

For example create an index like this:

CREATE INDEX TBLINDEX ON TBL ( id )

and then lock only row with id 10 doing this:

SELECT * FROM TBL WITH (UPDLOCK, INDEX(TBLINDEX)) WHERE id=10.

[FrankChen021]

I checked the 'FOR UPDATE' code change, it was added by #7661. And since SQLServer is supported by a community contributed extension, there's no integration tests so this problem was not exposed when the PR was done.

If this problem blocks your work, I'm suggesting you to fix it by your own if SQLServer is the only one option in your case. Or you could use MySQL/Postgre which are fully tested with Druid.

[santiagoguinle]

Well, at least for now i am following your recommendation.

i already made the test: i downloaded the code, changed it (just deleted the "FOR UPDATE" string), recompiled, configured to work with SQLServer and works perfect. Aparently is the only line of code that brings incompatibility. i even tryied some ingestion tasks and everything went great. Of course, probably it would be necesary to make a bigger change in order to maintain the functionality of locking rows.

I also notice that Druid indeed has several files in his core dedicated to deal with SQLServer.
But this "bug" was writed in a core class file, so an extension doesn't look like the place to fix it.
So, i am pretty sure that #7661 was a great improvement and fix some problems but also creates a incompatibility with a great DB Engine.

I dont do JAVA programming, so meaby im not the correct person for helping with this...
BUT i will be happy to help in order to keep improving Druid, so let me know if i can do something.

[FrankChen021]

A simplest but not the graceful way to fix it is to override compareAndSwap in SQLServer extension.

A better method is to add an abstract selectForUpdate function in SQLMetadataConnector to allow different database implementations to implement in their own ways.

I'm also thinking about if it's possible to use optimistic lock mechanism to address the problem globally.