feat: SQL Server / mssql dialect support #494

sebastian-alfers · 2023-12-18T15:07:06Z

Started off the postgres dialect impl, so some things should be familiar / identical.

What I am currently not sure about is the usage of timezones and timestamps.

Some perf test is failing sometimes locally..

Docs will be in a separate pr.

jdbc uses a different docker image which is a bit old. Not using that for now, unless we need the benefit is provides.

patriknw

a few high level comments before getting into more details...

patriknw · 2023-12-19T06:55:48Z

core/src/main/scala/akka/persistence/r2dbc/internal/sqlserver/SqlServerJournalDao.scala

+    INSERT INTO $journalTable
+    (slice, entity_type, persistence_id, seq_nr, writer, adapter_manifest, event_ser_id, event_ser_manifest, event_payload, tags, meta_ser_id, meta_ser_manifest, meta_payload, db_timestamp)
+    OUTPUT inserted.db_timestamp
+    VALUES (@slice, @entityType, @persistenceId, @seqNr, @writer, @adapterManifest, @eventSerId, @eventSerManifest, @eventPayload, @tags, @metaSerId, @metaSerManifest, @metaSerPayload, @dbTimestamp)"""


We should use positional indexes for binding the values, same as we use for the other dialects. Slightly more efficient, and by using the same approach we can reuse much more code between the dialects.

Value binding in this specific driver does not allow positional index binding based on ?s. The only alternative to what is currently used is positional binding zero-based - so numbers instead of ?s.

Unless I misunderstand, this means we can not reuse code related to queries definition / binding.

If have no problem with moving to zero-based binding for this PR if you still suggest this is the preferred approach. I just think named binding is much more readable.

For now I kept binding using named param as this seems to be recommended by the plugin: https://github.com/r2dbc/r2dbc-mssql/blob/main/src/main/java/io/r2dbc/mssql/MssqlStatement.java#L29

Alright, so you mean that the SqlServer itself uses named parameters in the actual requests sent to the server? That is different to Postgres, which uses positional parameters, and the translation from name to index happens in the client. Maybe the opposite translation from index to name isn't too bad for SqlServer and we could still use the same approach? The advantage of using the same approach would be that much more code would be the same (no need for special bind methods for all sql).

Yes, I agree. If the query is similar enough to re-use the binding based on positional index and then using some customization (e.g. for timestamps or tags) that would work out nice.

I will keep this in mind for later, once we have a direction regarding the other discussion below.

patriknw · 2023-12-19T06:59:15Z

core/src/main/scala/akka/persistence/r2dbc/internal/sqlserver/SqlServerJournalDao.scala

+private[r2dbc] class SqlServerJournalDao(settings: R2dbcSettings, connectionFactory: ConnectionFactory)(implicit
+    ec: ExecutionContext,
+    system: ActorSystem[_])
+    extends JournalDao {


Can't we extend the Postgres Dao and only override the bare minimum, in the same way we do for h2 and yugabyte? If there are some things that are different and need more specific override we can add such methods to be overridden in the Postgres Dao. It's internal api so we are free to refactor as needed when adding new dialect.

Yes, I can look into that.

Can't we extend the Postgres Dao and only override the bare minimum

@patriknw I took your comment serious and added a big refactoring to demonstrate how we could separate the actual sql query definition+binding (which is dialect specific) from the logic+error handling (which is not dialect specific and which should be reused as proposed). This is currently only done for SqlServerDurableStateDao and SqlServerJournalDao which I recommend to look at first as they show the real benefit of this change.

My last commit to this pr is rather big and is only a proposal at this point, but it shows how this could look like. If you agree with this direction, I would probably take a step back and create a separate pr from the current main and add this kind of decoupling for postgres and then rebase this pr on that.

I see how that looks more structured by having clear definitions of all sql and bind methods together. On the other hand I'm worried that this becomes over-designed and just adds more ceremony and makes it more difficult to understand via the additional indirection. It's like we have two DAO layers?

What is the benefit compared to placing the sql and bind methods in the Dao itself? I can understand that it might feel wrong to use PostgresDao as the base, so we could extract that (all common parts) to a BaseDao.

Extracting common parts to a BaseDao would make sense to me if we agree that the overhead (of separating the sql and bind methods from the actual logic and error handling) is worth it.

Maybe adding a new dialect is not worth the complexity / overhead it seems to introduce? I definitely do not want to open up a can of worms without getting enough value back here, so please share you thoughts.

What is the benefit compared to placing the sql and bind methods in the Dao itself?

I actually also prefer the current approach, where the dao contains the sql and binding. Hover, if we keep that approach, it would mean I would have to duplicate lots of logic. Example is your recent contribution. Without the refactoring I would have to basically copy what you have added / changed in that pr for postgres and re-implement it in the sqlserver dialect.

After looking and thinking about it for a bit I lean towards preferring (reasonable) duplication with extending the PostgresDAO (and possibly making more things there possible to override) over the extra clever abstraction for now. Once/if we add two-three more dialects we can reconsider.

and possibly making more things there possible to override

This is key to the pragmatic approach that I'm advocating. We evolve it and extract pieces and methods to override as we need, but not before we need it.

The way I would like to see it is that we extend PostgresDao and override only the things that are different for SqlServer. Still with sql in the Dao, and bindings in the Dao, where the bindings could be separate methods to be overridden when it's needed. That approach was quite successful when adding the H2 dialect, and I don't see that SqlServer would be so much different.

Then moving most of PostgresDao to BaseDao is pretty much just a naming cleanup that we can make separately afterwards.

Sounds good. I may start a new PR based on the learnings from this one.

Thanks, don't hesitate to share a small early draft to make sure we are aligned.

patriknw · 2023-12-19T07:00:42Z

core/src/test/scala/akka/persistence/r2dbc/internal/SqlServerDialectSpec.scala

+      val conf = ConfigFactory.parseString("""{
+          |  tag-separator = "|"
+          |}
+          |""".stripMargin)


such margin char isn't needed

patriknw · 2023-12-19T07:02:05Z

core/src/test/scala/akka/persistence/r2dbc/journal/PersistTimestampSpec.scala

@@ -80,7 +80,11 @@ class PersistTimestampSpec
              Row(
                pid = row.get("persistence_id", classOf[String]),
                seqNr = row.get[java.lang.Long]("seq_nr", classOf[java.lang.Long]),
-                dbTimestamp = row.get("db_timestamp", classOf[Instant]),
+                dbTimestamp = if (settings.dialectName == "sqlserver") {
+                  row.get("db_timestamp", classOf[LocalDateTime]).atZone(ZoneId.systemDefault()).toInstant


I haven't read up on sqlserver timestamps, but the principle should be that we always use UTC

Yes this is what I have done. I just missed migrating it in this test, good catch!

patriknw · 2023-12-19T07:03:56Z

ddl-scripts/create_tables_sqlserver.sql

+    tags NVARCHAR(255),
+
+    meta_ser_id INTEGER,
+    meta_ser_manifest NVARCHAR(MAX),


255 should be enough here

patriknw · 2023-12-19T07:06:02Z

project/Dependencies.scala

@@ -26,6 +26,10 @@ object Dependencies {
    val r2dbcPool = "io.r2dbc" % "r2dbc-pool" % "1.0.1.RELEASE" // ApacheV2
    val r2dbcPostgres = "org.postgresql" % "r2dbc-postgresql" % "1.0.3.RELEASE" // ApacheV2

+    // we have to stick to this version for now: https://github.com/r2dbc/r2dbc-mssql/issues/276
+    // bumping to 1.0.1.RELEASE or later currently requires pool config initial-size=1 and max-size=1
+    val r2dbcSqlServer = "io.r2dbc" % "r2dbc-mssql" % "1.0.0.RELEASE" // ApacheV2


should be % Provided

Where does it come from if we mark it as Provided?

It would be defined in the build of the user. We don't want to include dependencies to all drivers. At some point we might make the postgres driver provided as well.

patriknw · 2023-12-19T07:11:43Z

core/src/main/scala/akka/persistence/r2dbc/internal/sqlserver/SqlServerDialectHelper.scala

+  require(tagSeparator.length == 1, s"Tag separator '$tagSeparator' must be a single character.")
+
+  def tagsToDb(tags: Set[String]): String = {
+    if (tags.exists(_.contains(tagSeparator))) {


slightly more efficient to use Char as tagSeparator, since we only allow single character

sebastian-alfers · 2023-12-20T21:24:28Z

@patriknw I took a short spike in extending from PostgresJournalDao (not done, maybe I can re-use more so this is WIP) and mainly migrated one query to the alternative way of binding params. Do you think this the way to go? Then I will start migrating all other queries to that approach.

sebastian-alfers · 2023-12-21T07:56:02Z

Tests are failing for the changes in main which I will have a look at.

patriknw · 2023-12-21T09:34:16Z

It's in the right direction, but I think you have to start from the other end. Start with an empty SqlServerJournalDao that extends PostgresJournalDao. Then add overrides only on the things that are different. Look at H2JournalDao.

Also note that I made some dao changes in #485 so you will need to rebase to get the latest, for example writeEvents had some changes.

…references for one query

…dling to reuse the latter one while adding new dialects

Separate sql query definition+binding from the actual locic

sebastian-alfers · 2024-01-10T10:15:41Z

Closing and re-opening to re-trigger CI.

patriknw reviewed Dec 19, 2023

View reviewed changes

sebastian-alfers added 3 commits December 23, 2023 14:16

feat: SQL Server / mssql dialect support

88546ed

fix PersistTimestampSpec for h2

ae6d9fe

extend from PostgresJournalDao and use positional index (zero-based) …

7b15676

…references for one query

sebastian-alfers force-pushed the mssql-support branch from 15be8aa to 7b15676 Compare December 23, 2023 13:16

sebastian-alfers and others added 5 commits January 9, 2024 11:49

Separate sql query definition+binding from the actual locic+error han…

c194e4f

…dling to reuse the latter one while adding new dialects

Merge pull request #1 from sebastian-alfers/refactor-mssql-support-3

8a76a98

Separate sql query definition+binding from the actual locic

fmt

db50fe0

fix now for instant

87e6001

mima

b6908d7

sebastian-alfers closed this Jan 10, 2024

sebastian-alfers reopened this Jan 10, 2024

fix timestamp retrival

85ae32e

sebastian-alfers closed this Jan 22, 2024

sebastian-alfers mentioned this pull request Jan 22, 2024

feat: sqlserver / mssql support #505

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: SQL Server / mssql dialect support #494

feat: SQL Server / mssql dialect support #494

sebastian-alfers commented Dec 18, 2023 •

edited

Loading

patriknw left a comment

patriknw Dec 19, 2023

sebastian-alfers Dec 19, 2023

sebastian-alfers Jan 9, 2024

patriknw Jan 10, 2024

sebastian-alfers Jan 10, 2024

patriknw Dec 19, 2023

sebastian-alfers Dec 19, 2023

sebastian-alfers Jan 9, 2024

patriknw Jan 10, 2024

sebastian-alfers Jan 10, 2024

johanandren Jan 10, 2024

patriknw Jan 10, 2024

sebastian-alfers Jan 10, 2024

patriknw Jan 22, 2024

patriknw Dec 19, 2023

patriknw Dec 19, 2023

sebastian-alfers Dec 19, 2023

patriknw Dec 19, 2023

patriknw Dec 19, 2023

sebastian-alfers Dec 19, 2023

patriknw Dec 19, 2023

patriknw Dec 19, 2023

sebastian-alfers commented Dec 20, 2023

sebastian-alfers commented Dec 21, 2023

patriknw commented Dec 21, 2023

sebastian-alfers commented Jan 10, 2024

feat: SQL Server / mssql dialect support #494

feat: SQL Server / mssql dialect support #494

Conversation

sebastian-alfers commented Dec 18, 2023 • edited Loading

patriknw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastian-alfers commented Dec 20, 2023

sebastian-alfers commented Dec 21, 2023

patriknw commented Dec 21, 2023

sebastian-alfers commented Jan 10, 2024

sebastian-alfers commented Dec 18, 2023 •

edited

Loading