As skip locked is always used in a for update clause, would it be better to modify the existing forUpdate functions to support the following usage?

query.forUpdate(skipLocked = true)

Generated SQL:

select ... for update skip locked

It will be great if can also support nowait:

query.forUpdate(nowait = true)

Generate SQL:

select ... for update nowait

And:

query.forUpdate(wait = 3)

SQL:

select ... for update wait 3

To implement this, we can deprecate the forUpdate property in SelectExpression, replacing its type as a structured data class instead of Boolean.

What do you think?

(...)

To implement this, we can deprecate the forUpdate property in SelectExpression, replacing its type as a structured data class instead of Boolean.

What do you think?

Having parameters in this way does sound better, yes!

Up until the last paragraph though, I was thinking of just overloading the forUpdate method in the Postgres-module with extra parameters, but then you wrote that about a structured data class - I'm not entirely sure if I understand what you mean by that in this context?

I mean it would be better if this PR can implement more for update options like nowait, wait, not just skip locked. We can modify the SelectExpression like this:

public data class SelectExpression(
    val columns: List<ColumnDeclaringExpression<*>> = emptyList(),
    val from: QuerySourceExpression,
    val where: ScalarExpression<Boolean>? = null,
    val groupBy: List<ScalarExpression<*>> = emptyList(),
    val having: ScalarExpression<Boolean>? = null,
    val isDistinct: Boolean = false,
    val forUpdate: SelectForUpdateOptions = SelectForUpdateOptions.None,
    override val orderBy: List<OrderByExpression> = emptyList(),
    override val offset: Int? = null,
    override val limit: Int? = null,
    override val tableAlias: String? = null,
    override val extraProperties: Map<String, Any> = emptyMap()
) : QueryExpression()

public sealed class SelectForUpdateOptions {
    public object None : SelectForUpdateOptions()
    public object SkipLocked : SelectForUpdateOptions()
    public object NoWait : SelectForUpdateOptions()
    public data class Wait(val seconds: Int) : SelectForUpdateOptions()
}

Don't hesitate to ask me if you have any other questions.

I mean it would be better if this PR can implement more for update options like nowait, wait, not just skip locked. We can modify the SelectExpression like this:

Okay, I see what you mean. The only thing I don't quite like about that solution is that "skip locked" is postgres specific - it doesn't really seem elegant to put explicit support for it in the main library, as I believe we'd have to do in this case. (Correct me if I'm wrong.) Nowait on the other hand exists in more dialects, oracle at least, but as far as I can see its not universal - SQL Server is missing it, as far as I can see. Although now that I'm trying to find it, I'm not sure SQL Server has any support for "for update" in the first place? (See: https://docs.microsoft.com/en-us/sql/t-sql/queries/select-transact-sql?view=sql-server-ver15 )

If indeed I'm correct in for update not being universal, I'm tempted to say the function should be moved from the main library into the relevant dialects, and given the appropriate parameters and behaviours for each DBMS there. And probably use the extraProperties for representation in SelectExpression to avoid polluting that class with DBMS-specific code - although as it's behind the scenes, I could see an argument for keeping it more readable by putting fairly common things there.

I'll update the PR to reflect these thoughts if you agree.

Yes, for update is not universal, it was a mistake implementing the forUpdate functions in the core library. I think we can deprecate the legacy code and reimplement them in dialect modules. The only pain is we have to copy-paste some code between dialects, but it is acceptable, this time only supporting MySQL and PostgreSQL is enough.

You might need two MySQL dialects to support this.

Both 5.7 and 8.0 have two locking modes, share and update, but the syntax is different.

5.7 share: SELECT ... LOCK IN SHARE MODE
5.7 update: SELECT ... FOR UPDATE

8.0 share: SELECT ... FOR SHARE
8.0 update: SELECT ... FOR UPDATE

You might need two MySQL dialects to support this.

Hm. Is there any prior work in Ktorm where there is support for several versions of a DBMS' dialect?

I can think of two ways to do it that don't require connection info. Either publish two different ktorm-support-mysql modules, or require that a env var/Java System Prop has been set to specify the MySQL major version.

Or it could be lazy and require that the SqlDialect gets a DataSource and then do something like:

dataSource.connection.use { connection ->
    when (val databaseMajorVersion = connection.metaData.databaseMajorVersion) {
        5 -> {
            // MySql5Dialect
        }
        8 -> {
            // MySql8Dialect
        }
        else -> throw UnsupportedMySqlVersion(databaseMajorVersion)
    }
}

edit: disregard all that. Database already gets a Connection on init and inspects the DatabaseMetaData. It could just pass that to the SqlDialect.

I took a stab at it here:
https://github.com/efenderbosch/ktorm/tree/forUpdate

Feel free to use/modify that.

MySql 5, MySql 8 and Postgres should be fully implemented. Oracle and SqlServer only have the basic "for update" options. SQLite throws DialectFeatureNotSupportedException.

edit: tweaked Postgres to follow the docs. Oracle could be done similarly.

Yes, we already have productVersion in Database. We can use it in MySqlFormatter when generating the SQL.

Updated my branch to use productVersion

Should I submit a separate PR?

@efenderbosch Yes, please create another PR.

I'm sorry about dropping the ball here, life got chaotic. My understanding is that efenderbosch's PR will now solve this ,so I should just close this PR`?

Close this PR since #247 was merged.