r2dbc / r2dbc-mssql

R2DBC Driver for Microsoft SQL Server using TDS (Tabular Data Stream) Protocol
Apache License 2.0
178 stars 32 forks source link

Ensure that`PreferredCursorExecution` is the only configuration flag to control cursor preference #267

Closed Dabble63 closed 1 year ago

Dabble63 commented 1 year ago

It should be possible to specify that cursors not be used. I receive the message "Could not complete cursor operation because the table schema changed after the cursor was declared.". The query completes successfully when run without using cursors. The schema on the database did in fact change. Whether cursors are used or not is determined by the following code in SimpleMssqlStatement.java

static boolean prefersCursors(String sql) {
        if (sql.isEmpty()) {
            return false;
        }

        String lc = sql.trim().toLowerCase(Locale.ENGLISH);
        if (lc.contains("for xml") || lc.contains("for json")) {
            return false;
        }

        char c = sql.charAt(0);
        return (c == 's' || c == 'S') && lc.startsWith("select");
    }
}

It is not clear whether the author deliberately checks the sql string starting with 's' or 'S' rather than the trimmed string. Anyway both checks do not take into account that the SQL may start with a comment. As a consequence, it is possible to force cursors to not be used by starting the statement with a space. This feels like exploiting another bug so if this is fixed this workaround will no longer work, It also means that you cannot use automatically generated statements such as those produced by JPA.

mp911de commented 1 year ago

MssqlConnectionConfiguration allows configuring a predicate to force cursor usage. Even if preferCursoredExecution is false, it is a preference and not a directive. We favor cursor usage because cursors work smoother with backpressure.

Currently, you can force direct execution by specifying the fetch size to zero.

Dabble63 commented 1 year ago

Unfortunately the fetchSize is not applied in all situations. When MssqlConnection.createStatement is run it creates the SimpleMssqlStatement object which initializes the fetchSize to the default value of -1. If the statement starts with the word select (see code in previous post) then preferCursoredExecution will be set to true. The getEffectiveFetchSize routine will then return FETCH_SIZE which has a value of 128.

@Override public MssqlStatement createStatement(String sql) {

    Assert.requireNonNull(sql, "SQL must not be null");
    logger.debug(this.context.getMessage("Creating statement for SQL: [{}]"), sql);

    if (ParametrizedMssqlStatement.supports(sql)) {
        return new ParametrizedMssqlStatement(this.client, this.connectionOptions, sql);
    }

    return new SimpleMssqlStatement(this.client, this.connectionOptions, sql);
}

MssqlStatementSupport: int getEffectiveFetchSize() {

    if (this.preferCursoredExecution) {
        return this.fetchSize == FETCH_UNCONFIGURED ? FETCH_SIZE : this.fetchSize;
    }

    return this.fetchSize == FETCH_UNCONFIGURED ? 0 : this.fetchSize;
}
mp911de commented 1 year ago

Not quite sure we're on the same page. You can set the fetch size yourself via connection.createStatement(…).fetchSize(0). In that case, Statement.execute() is using direct execution instead of cursored execution.

Dabble63 commented 1 year ago

Unfortunately I am running a Spring Boot JPA application with a connection pool. fetchSize is part of the configuration however it is not always applied. I don't see how I can interfere without rewriting a good portion of the code.

From the call stack of the creation of SimpleMssqlStatement I see: new SimpleMssqlStatement(it, this.connectionOptions, this.METADATA_QUERY).execute()

In short, execute is called directly after creation of the statement. Probably the options should include the fetchSize so that it can be applied during the constructor of SimpleMssqlStatement. The fetchSize optimally would be set using \@QueryHint in JPA. My solution has been to ensure that no statement, for which I do not want cursors used, starts with s or S. This does mean that I cannot use the JPA generated selects however in my case that is not terrible.

mp911de commented 1 year ago

We will move the predicate evaluating SELECT as text into the default cursor preference predicate so you can entirely control cursor preference without having other predicates that would override the preference.