Add Oracle support in the trigger service #9286

cocreature · 2021-03-30T14:16:06Z

This PR migrates the ddl & queries and adds tests for this. It does
not yet expose this to users. I’ll handle that in a separate PR.

changelog_begin
changelog_end

Pull Request Checklist

Read and understand the contribution guidelines
Include appropriate tests
Set a descriptive title and thorough description
Add a reference to the issue this PR will solve, if appropriate
Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

cocreature · 2021-03-30T14:16:58Z

libs-scala/oracle-testing/src/main/scala/testing/oracle/OracleAround.scala

@@ -29,7 +31,7 @@ trait OracleAround {
    // See https://docs.oracle.com/cd/B19306_01/server.102/b14200/sql_elements008.htm#i27570
    // for name restrictions.
    val u = "u" + Random.alphanumeric.take(29).mkString("")
-    createNewUser(u)
+    createNewUser(u.toUpperCase)


oracle is case insensitive so this seems like a less confusing option.

cocreature · 2021-03-30T14:17:50Z

triggers/service/src/main/resources/com/daml/lf/engine/trigger/db/migration/oracle/V1__Init.sql

+-- Copyright (c) 2020 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved.
+-- SPDX-License-Identifier: Apache-2.0
+
+-- Running trigger table.


We could in theory use only one migration here since nobody has any historical data on oracle. However, keeping the two in sync seems easier to understand.

cocreature · 2021-03-30T14:18:34Z

triggers/service/src/main/resources/com/daml/lf/engine/trigger/db/migration/oracle/V1__Init.sql

+  trigger_party nvarchar2(255) not null,
+  -- the identifier for the trigger in its dalf,
+  --  of the form "packageId:moduleName:triggerName"
+  full_trigger_name nvarchar2(1000) not null


1000 is somewhat arbitrary. There is no length limit on module name and trigger name. Oracle really does not like unlimited length (there are options but they all suck). The JSON API and canton also use somewhat arbitrary limits here.

cocreature · 2021-03-30T14:19:41Z

triggers/service/src/main/scala/com/digitalasset/daml/lf/engine/trigger/dao/DbTriggerDao.scala

+  ): ConnectionIO[Unit] =
+  {
+    val insert: Fragment = sql"""
+      insert /*+  ignore_row_on_dupkey_index ( dalfs ( package_id ) ) */


yes magic comments are how you do on conflict do nothing in oracle. This is fine.gif

cocreature · 2021-03-30T14:22:52Z

triggers/service/src/test/resources/logback.xml

@@ -11,7 +11,7 @@
    <logger name="io.grpc.netty" level="WARN">
        <appender-ref ref="stderr-appender"/>
    </logger>
-    <logger name="com.daml.lf.engine.trigger" level="DEBUG">
+    <logger name="com.daml.lf.engine.trigger" level="INFO">


happy to revert this but I found it super hard to find the right places with debug logs turned on.

Seems fine to change

This PR migrates the ddl & queries and adds tests for this. It does not yet expose this to users. I’ll handle that in a separate PR. changelog_begin changelog_end

cocreature · 2021-03-30T14:55:45Z

triggers/service/src/main/scala/com/digitalasset/daml/lf/engine/trigger/ServiceConfig.scala

@@ -56,6 +57,7 @@ object JdbcConfig {
      user <- requiredField(x)("user")
      password <- requiredField(x)("password")
    } yield JdbcConfig(
+      driver = "org.postgresql.Driver", // TODO make this configurable


This is what prevents this from being user-facing at the moment.

cocreature · 2021-03-30T14:56:34Z

triggers/service/src/main/scala/com/digitalasset/daml/lf/engine/trigger/dao/DbTriggerDao.scala

-      *> dropTriggerTable.update.run
-      *> dropDalfTable.update.run).void
-  }
+  private def dropTables: ConnectionIO[Unit] = flywayMigrations.clean()


flyway names things slightly differently depending on the driver and there really seems no point in reinventing what Flyway.clean does.

S11001001 · 2021-03-31T00:30:02Z

triggers/service/src/main/scala/com/digitalasset/daml/lf/engine/trigger/dao/DbTriggerDao.scala

  def apply(c: JdbcConfig, poolSize: PoolSize = Production)(implicit
      ec: ExecutionContext
  ): DbTriggerDao = {
    implicit val cs: ContextShift[IO] = IO.contextShift(ec)
    val (ds, conn) = Connection.connect(c, poolSize)
-    new DbTriggerDao(ds, conn)
+    val driver = supportedJdbcDrivers.get(c.driver) match {


Can use supportedJdbcDrivers.getOrElse here.

c3cbb7c thanks

S11001001 · 2021-03-31T00:30:47Z

triggers/service/src/test/resources/logback.xml

@@ -11,7 +11,7 @@
    <logger name="io.grpc.netty" level="WARN">
        <appender-ref ref="stderr-appender"/>
    </logger>
-    <logger name="com.daml.lf.engine.trigger" level="DEBUG">
+    <logger name="com.daml.lf.engine.trigger" level="INFO">


Seems fine to change

S11001001 · 2021-03-31T00:35:10Z

...s/service/src/test/scala/com/digitalasset/daml/lf/engine/trigger/TriggerServiceFixture.scala

+  private lazy val jdbcConfig_ =
+    JdbcConfig("oracle.jdbc.OracleDriver", oracleJdbcUrl, oracleUser, oraclePwd)
+  // TODO For whatever reason we need a larger pool here, otherwise
+  // the connection deadlocks. I have no idea why :(


My guess would be that culprit=flyway, maybe more connections when running on Oracle.

Yeah it’s definitely flyway. I just don’t really get why it would use more connections against Oracle than against PostgreSQL for the same number of migrations but 🤷 doesn’t seem that bad to change it.

changelog_begin changelog_end

@cocreature

- suggested by @cocreature in #9286 f7b2f14; thanks

@cocreature

* comparison queries * name the contract primary key constraint * use ignore_row_on_dupkey_index instead of merge - suggested by @cocreature in #9286 f7b2f14; thanks * retrySqlStates for oracle * enable all non-websocket tests * name the template_id primary key constraint * clean up concatFragment calls * add Websocket tests for oracle * move iouCreateCommand to be usable by oracle integration tests * work around Scala 2.12 NPE in Source * multiquery support for Oracle * matchedQueries, therefore query-stream support for Oracle * enable websocket tests * test '& bar' and 5kb strings - 5kb string fails on Oracle with ORA-01704: string literal too long * refine the long data cases; gets too long at 4000 bytes as expected - however, the predicate fails for unknown reason before then; possibly a missed escape character case * handle long data with a fallback - now the predicate fails in all cases instead of a SQL error, which is...better * only interpolate true, false, null into JSON predicate conditions - the problem was with JSON-formatted data; it must be SQL-typed instead * adapt equal's large-data solution for comparison as well - only works for numbers and strings, but that's all we need to compare * move Implicits to Queries * remove stray spaces in output * test Oracle query expressions alongside Postgresql * test that bools aren't compared like numbers and strings * test @> conjunctions and special {}-query handling * no changelog CHANGELOG_BEGIN CHANGELOG_END * note on PASSING ... AS X - suggested by @cocreature; thanks * remove printlns; these functions don't really need scaffolding anymore - suggested by @stefanobaghino-da; thanks

cocreature commented Mar 30, 2021

View reviewed changes

cocreature force-pushed the trigger-oracle branch 5 times, most recently from 6768a11 to 7712d78 Compare March 30, 2021 15:35

Add Oracle support in the trigger service

cc8f51d

This PR migrates the ddl & queries and adds tests for this. It does not yet expose this to users. I’ll handle that in a separate PR. changelog_begin changelog_end

cocreature force-pushed the trigger-oracle branch from 7712d78 to cc8f51d Compare March 30, 2021 15:39

cocreature commented Mar 30, 2021

View reviewed changes

cocreature requested review from aherrmann-da and S11001001 and removed request for aherrmann-da March 30, 2021 15:52

cocreature marked this pull request as ready for review March 30, 2021 15:52

cocreature requested review from aherrmann-da, garyverhaegen-da and hurryabit as code owners March 30, 2021 15:52

cocreature requested a review from a user March 30, 2021 15:52

cocreature requested a review from stefanobaghino-da as a code owner March 30, 2021 15:52

S11001001 reviewed Mar 31, 2021

View reviewed changes

use getOrElse

c3cbb7c

changelog_begin changelog_end

S11001001 approved these changes Mar 31, 2021

View reviewed changes

cocreature merged commit f7b2f14 into main Mar 31, 2021

cocreature deleted the trigger-oracle branch March 31, 2021 16:39

S11001001 added this to the Trigger Service milestone Apr 5, 2021

S11001001 added a commit that referenced this pull request Apr 5, 2021

use ignore_row_on_dupkey_index instead of merge

e6a53be

- suggested by @cocreature in #9286 f7b2f14; thanks

S11001001 mentioned this pull request Nov 1, 2022

Query store support for interface query/fetch #15304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Oracle support in the trigger service #9286

Add Oracle support in the trigger service #9286

cocreature commented Mar 30, 2021

cocreature Mar 30, 2021

cocreature Mar 30, 2021

cocreature Mar 30, 2021

cocreature Mar 30, 2021

cocreature Mar 30, 2021

S11001001 Mar 31, 2021

cocreature Mar 30, 2021

cocreature Mar 30, 2021

S11001001 Mar 31, 2021

cocreature Mar 31, 2021

S11001001 Mar 31, 2021

S11001001 Mar 31, 2021

cocreature Mar 31, 2021

Add Oracle support in the trigger service #9286

Add Oracle support in the trigger service #9286

Conversation

cocreature commented Mar 30, 2021

Pull Request Checklist

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