Add support for Oracle as backend database #89

posulliv · 2023-11-08T21:08:02Z

No description provided.

posulliv · 2023-11-08T21:09:39Z

gateway-ha/src/main/java/io/trino/gateway/ha/router/HaQueryHistoryManager.java

-      return QueryHistory.upcast(QueryHistory.findBySQL(String.join(" ",
-          sql,
-          "order by created desc",
-          "limit 2000")));


I removed this hard-coded SQL since LIMIT is not supported in Oracle.

posulliv · 2023-11-08T21:10:28Z

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/dao/ResourceGroups.java

@@ -77,7 +77,6 @@ public static List<ResourceGroupsDetail> upcast(List<ResourceGroups> resourceGro
   * @param resourceGroupDetail
   */
  public static void create(ResourceGroups model, ResourceGroupsDetail resourceGroupDetail) {
-    model.set(resourceGroupId, resourceGroupDetail.getResourceGroupId());


I made this change to ensure auto-increment resource group ID column is used.

willmostly

Just a few minor comments, thanks for the submission!

willmostly · 2023-11-08T21:36:44Z

gateway-ha/src/main/java/io/trino/gateway/ha/router/HaQueryHistoryManager.java

      if (user.isPresent()) {
-        sql += " where user_name = '" + user.get() + "'";
+        return QueryHistory.upcast(QueryHistory.where("user_name = '" + user.get() + "'").limit(2000).orderBy("created desc"));


use QueryHistory.userName instead of "user_name" and QueryHistory.created instead of "created"

It appears all the attributes of the activejdbc models are private. Should we add getters for them? Not sure what the typical activejdbc pattern is for this.

We should make these public IMO

willmostly · 2023-11-08T21:42:49Z

gateway-ha/src/main/resources/gateway-ha-persistence-oracle.sql

+CREATE INDEX query_history_created_idx ON query_history(created);
+
+CREATE TABLE resource_groups (
+    resource_group_id NUMBER GENERATED ALWAYS as IDENTITY(START with 1 INCREMENT by 1),


I don't know Oracle very well, but would a LONG be a better choice than a NUMBER? Iiuc a NUMBER with unspecified scale supports real numbers as well as integers. Not that it probably matters much

a LONG in oracle is not actually a numeric data type confusingly enough :) https://docs.oracle.com/en/database/oracle/oracle-database/19/sqlrf/Data-Types.html#GUID-F6309DF8-162F-48A4-9454-FEE59EC6644F

I would vote to use NUMBER as the data type for the identity column here.

After reviewing, I'd agree

willmostly · 2023-11-08T21:44:42Z

gateway-ha/src/main/resources/gateway-ha-persistence.sql

@@ -18,7 +18,7 @@ CREATE INDEX query_history_created_idx ON query_history(created);

 CREATE TABLE IF NOT EXISTS resource_groups (
    resource_group_id BIGINT NOT NULL AUTO_INCREMENT,
-    name VARCHAR(250) NOT NULL UNIQUE,
+    name VARCHAR(250) NOT NULL,


Why are we changing the MySQL sql for this PR?

I updated it to match the schema in trino for consistency - https://github.com/trinodb/trino/blob/master/plugin/trino-resource-group-managers/src/main/resources/db/migration/mysql/V2__add_resource_groups.sql

Happy to revert if unique names need to be enforced for gateway.

One use case for non-unique names I have had in the past with Trino is storing the same resource group definitions for different environments in the same database. This is possible since resource groups are determined by the environment column of the resource_groups table. So you could use a single database and have the same resource group definitions for multiple environments.

I'm not sure. Trino clusters require an environment name set, while the gateway could front backends with different environment names. So environment could be less meaningful in the gateway context. OTOH, this is defined by the user, and they can choose not to complicate things.

I guess being able to import a set of policies from Trino to the gateway probably wins out.

vishalya · 2023-11-10T19:54:48Z

Also, a general comment here - we are supporting almost 3 dbs now, will it be a good idea to use migrations now than actual DDL like Flyway.

mosabua · 2023-11-10T20:15:05Z

Also, a general comment here - we are supporting almost 3 dbs now, will it be a good idea to use migrations now than actual DDL like Flyway.

Totally agree .. that could also help with installation/ bootstrap .. and we need to figure out testing for the different backends.

posulliv · 2023-11-15T16:15:52Z

Also, a general comment here - we are supporting almost 3 dbs now, will it be a good idea to use migrations now than actual DDL like Flyway.

We are using flyway in trino for resource group migrations so could likely reuse some of what we did there. Are we good with doing this in a follow up PR though?

willmostly · 2023-11-15T20:50:45Z

Are we good with doing this in a follow up PR though?
I agree that we should split these up. Either first Oracle, then Flyway, or the other way round.

mosabua · 2024-01-05T17:39:52Z

Fyi @ebyhr ... any thought of adding an abstraction layer like Flyway or so? Also given you are ripping over from activejdbc to jdbi it would be good to have a coherent plan in place

And @posulliv please rebase this PR so we can review again and look towards merging.

posulliv · 2024-01-11T14:18:32Z

@willmostly @mosabua OK I rebased against latest master to resolve conflicts.

willmostly

This looks ready to go. @posulliv and I have discussed offline and will begin work to introduce Flyway for schema management once this PR is merged

pom.xml

mosabua · 2024-02-02T23:29:41Z

At minimum a rebase has to be done. I can review more then ..

mosabua · 2024-02-05T22:06:44Z

docs/resource-groups-api.md


 ```$xslt
 curl -X POST http://localhost:8080/trino/selector/create \
 -d '{
+        "resourceGroupId": 1, \


that seems like a bad idea .. how would you know what number to use? An id should be created automatically. Also .. this change should be independent and separate

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/dao/QueryHistory.java

gateway-ha/pom.xml

posulliv · 2024-02-06T22:03:32Z

Rebased but tests failing now. Will try to figure it out when I get a chance this week.

posulliv · 2024-02-07T19:51:26Z

@ebyhr the problem I'm facing now is that the SQL in QueryHistoryDao is not compatible with Oracle (limit is not supported in Oracle). Didn't have this issue with activejdbc when calling QueryHistory.limit().

Any suggestions on the preferred approach to deal with this in JDBI?

ebyhr · 2024-03-05T04:09:27Z

@posulliv We can call different dao methods based on the connection-url prefix jdbc:oracle.

cla-bot bot added the cla-signed label Nov 8, 2023

posulliv commented Nov 8, 2023

View reviewed changes

willmostly reviewed Nov 8, 2023

View reviewed changes

posulliv force-pushed the add-oracle-db-support branch from 219b650 to 6469a13 Compare November 15, 2023 16:29

willmostly approved these changes Nov 15, 2023

View reviewed changes

posulliv force-pushed the add-oracle-db-support branch 2 times, most recently from 0ce3613 to 8595d29 Compare January 11, 2024 14:17

willmostly approved these changes Feb 2, 2024

View reviewed changes

ebyhr reviewed Feb 2, 2024

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

mosabua reviewed Feb 5, 2024

View reviewed changes

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/dao/QueryHistory.java Outdated Show resolved Hide resolved

mosabua reviewed Feb 5, 2024

View reviewed changes

gateway-ha/pom.xml Outdated Show resolved Hide resolved

posulliv force-pushed the add-oracle-db-support branch from 8595d29 to 1b600dd Compare February 6, 2024 16:02

Add support for Oracle as backend database

8119bd0

posulliv force-pushed the add-oracle-db-support branch from 1b600dd to 8119bd0 Compare February 6, 2024 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Oracle as backend database #89

Add support for Oracle as backend database #89

posulliv commented Nov 8, 2023

posulliv Nov 8, 2023

posulliv Nov 8, 2023

willmostly left a comment

willmostly Nov 8, 2023

posulliv Nov 9, 2023

willmostly Nov 14, 2023

willmostly Nov 8, 2023

posulliv Nov 9, 2023

willmostly Nov 14, 2023

willmostly Nov 8, 2023

posulliv Nov 8, 2023

posulliv Nov 9, 2023

willmostly Nov 14, 2023

vishalya commented Nov 10, 2023

mosabua commented Nov 10, 2023 •

edited

posulliv commented Nov 15, 2023

willmostly commented Nov 15, 2023

mosabua commented Jan 5, 2024 •

edited

posulliv commented Jan 11, 2024

willmostly left a comment

mosabua commented Feb 2, 2024

mosabua Feb 5, 2024

posulliv commented Feb 6, 2024

posulliv commented Feb 7, 2024

ebyhr commented Mar 5, 2024

Add support for Oracle as backend database #89

Are you sure you want to change the base?

Add support for Oracle as backend database #89

Conversation

posulliv commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willmostly 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

vishalya commented Nov 10, 2023

mosabua commented Nov 10, 2023 • edited

posulliv commented Nov 15, 2023

willmostly commented Nov 15, 2023

mosabua commented Jan 5, 2024 • edited

posulliv commented Jan 11, 2024

willmostly left a comment

Choose a reason for hiding this comment

mosabua commented Feb 2, 2024

Choose a reason for hiding this comment

posulliv commented Feb 6, 2024

posulliv commented Feb 7, 2024

ebyhr commented Mar 5, 2024

mosabua commented Nov 10, 2023 •

edited

mosabua commented Jan 5, 2024 •

edited