Add support for Oracle as backend database by posulliv · Pull Request #89 · trinodb/trino-gateway

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

No description provided.

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

-      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

   * @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

-      String sql = "select * from query_history";
      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

+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

 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

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

 ```$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

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.

mosabua · 2024-09-05T16:50:16Z

Chatted with @willmostly .. I think this can be closed and restarted when it becomes relevant. Feel free to reopen after rebasing and adjusting

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

Comment thread pom.xml Outdated

mosabua reviewed Feb 5, 2024

View reviewed changes

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

mosabua reviewed Feb 5, 2024

View reviewed changes

Comment thread gateway-ha/pom.xml Outdated

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

mosabua closed this Sep 5, 2024

Conversation

posulliv commented Nov 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willmostly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vishalya commented Nov 10, 2023

Uh oh!

mosabua commented Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

posulliv commented Nov 15, 2023

Uh oh!

willmostly commented Nov 15, 2023

Uh oh!

mosabua commented Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

posulliv commented Jan 11, 2024

Uh oh!

willmostly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mosabua commented Feb 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

posulliv commented Feb 6, 2024

Uh oh!

posulliv commented Feb 7, 2024

Uh oh!

ebyhr commented Mar 5, 2024

Uh oh!

mosabua commented Sep 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

mosabua commented Nov 10, 2023 •

edited

Loading

mosabua commented Jan 5, 2024 •

edited

Loading