Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Oracle db support #379

Merged
merged 24 commits into from
Dec 7, 2023
Merged

Oracle db support #379

merged 24 commits into from
Dec 7, 2023

Conversation

ngbanguyen
Copy link
Contributor

No description provided.

@ngbanguyen ngbanguyen closed this Oct 17, 2023
@ngbanguyen
Copy link
Contributor Author

ngbanguyen commented Oct 17, 2023 •
edited
Loading

Waiting for approvals before reopening.

@ngbanguyen ngbanguyen reopened this Oct 18, 2023
@ngbanguyen
Copy link
Contributor Author

Added Oracle profile as an optional feature. Building and running Oracle profile is similar to other benchmarks.

./mvnw clean package -P oracle

Tested with Oracle Database Express Edition 18.4.0, 21.3.0 and Oracle DB Free Release 23.2.0.0.

Some changes that need explanations:

  • General changes to avoid creating empty string when running Oracle profile, as Oracle DB treats empty strings as null.
  • oracle.sql.TIMESTAMP is not an instance of java.sql.Timestamp. oracle.sql.TIMESTAMP (and the conversion method) needs to be reflectively handled as it is OJDBC-specific (not available in JDBC).
  • Handles CLOB conversion for LoadConfig procedure in auctionmark and seats. OJDBC CLOB is an instance of java.sql.Clob so this can be handled in-code. However, CLOB needs to be read/converted to String while the connection that retrieved it is still open.
  • Added an option to run script after loading. tpch and chbenchmark sample configs contain this option.
    • There were performance issues while running tpch and chbenchmark. Running dbms_stats.gather_schema_stats would solve the performance issue. The sample Oracle config for chbenchmark and tpch was appended with the option to make sure the optimization was run after loading (especially after running --create=true --load=true --execute=true).
      This issue is Oracle profile specific, however it makes more sense to leave this option for other profiles.
  • Add and/or amend Oracle DDL and dialects for all benchmarks. I tried to make the changed dialect and/or DDL as close
    to other profiles as possible, mostly referencing MySQL DDL. There are comments for some changes in DDL, such as resourcestresser MD5 function in Oracle.
  • Official Oracle DB container (container-registry.oracle.com/database/free) doesn't allow creating a user at startup using variables. I chose to use Oracle image from gvenzl/oci-oracle-xe as this image provides a short option for creating a non-DBA user at start up using options.

GitHub Actions with Oracle DB is also added. Latest run

  • Using Oracle Database Express Edition 21.3.0 XE.
  • The error rates are higher on some benchmarks due to procedures raising ORA-08177 not serializable.

A remaining issue:

  • There are some errors while running auctionmark on NewPurchase procedure, but I believe the SQL for other procedures are correct. Would appreciate some inputs.

@ngbanguyen
Copy link
Contributor Author

@fgauthie @kavorobyov Please review.

Should I also include GraalVM Native Image configuration file in this PR as well?

apavlo
apavlo requested changes Oct 19, 2023
View reviewed changes
Copy link
Member

@apavlo apavlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sending this! Please address the comments in the review.

Most importantly please remove the Oracle copyright. We cannot take this.

.github/workflows/maven.yml Outdated Show resolved Hide resolved
config/oracle/sample_auctionmark_config.xml Outdated Show resolved Hide resolved
config/oracle/sample_chbenchmark_config.xml Outdated Show resolved Hide resolved
src/main/java/com/oltpbenchmark/DBWorkload.java Outdated Show resolved Hide resolved
src/main/java/com/oltpbenchmark/api/BenchmarkModule.java Outdated Show resolved Hide resolved
src/main/java/com/oltpbenchmark/api/Procedure.java Outdated Show resolved Hide resolved
src/main/java/com/oltpbenchmark/benchmarks/auctionmark/procedures/LoadConfig.java Outdated
Comment on lines 88 to 93
// CLOB needs to be done while connection is alive
if (dbType == DatabaseType.ORACLE) {
for (Object[] configProfileInstance: configProfile) {
configProfileInstance[3] = SQLUtil.clobToString(configProfileInstance[3]);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why this is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oracle DB DDL contains some CLOB fields (for LoadConfig procedures). These CLOB needs to be converted to String while the connection is alive.

This CLOB conversion for Oracle needs to be done here, otherwise the conversion will be attempted by SQLUtil.getString(Object) after the connection closes, which will result in java.sql.SQLRecoverableException: Closed Connection.

src/main/java/com/oltpbenchmark/util/SQLUtil.java Outdated
Comment on lines 141 to 155
private static final Class<?> ORACLE_TIMESTAMP;
private static final Method TIMESTAMP_VALUE_METHOD;
static {
Method timestampValueMethod;
Class<?> oracleTimestamp;
try {
oracleTimestamp = Class.forName("oracle.sql.TIMESTAMP");
timestampValueMethod = oracleTimestamp.getDeclaredMethod("timestampValue");
} catch (ClassNotFoundException | NoSuchMethodException e) {
oracleTimestamp = null;
timestampValueMethod = null;
}
TIMESTAMP_VALUE_METHOD = timestampValueMethod;
ORACLE_TIMESTAMP = oracleTimestamp;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this code doing? Why is it necessary?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, a few more comments would be nice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oracle support needed TIMESTAMP fields which corresponds to OJDBC-specific type oracle.sql.TIMESTAMP (not available in JDBC).

I need to convert OJDBC timestamp to JDBC java.sql.Timestamp (with timestampValue method) without having to import the type directly, hence the use of reflection with Class.forName(String) and getDeclaredMethod(String). For Oracle profile the types will be initialized and used in getTimestamp(Object) and for other profiles the variables will be null and the conversion branch wouldn't be reachable.

bpkroth
bpkroth reviewed Nov 6, 2023
View reviewed changes
.github/workflows/maven.yml Outdated Show resolved Hide resolved
bpkroth
bpkroth reviewed Nov 6, 2023
View reviewed changes
docker/oracle/down.sh Outdated Show resolved Hide resolved
bpkroth
bpkroth reviewed Nov 6, 2023
View reviewed changes
docker/oracle/reset.sql Outdated Show resolved Hide resolved
bpkroth
bpkroth reviewed Nov 6, 2023
View reviewed changes
docker/oracle/up.sh Outdated Show resolved Hide resolved
@bpkroth
Copy link
Collaborator

bpkroth commented Dec 6, 2023

Left some cleanup nits and one remaining comment about the docker image selection, but overall this is looking pretty good to me. Thanks for your continued work on it!

bpkroth
bpkroth approved these changes Dec 7, 2023
View reviewed changes
Copy link
Collaborator

@bpkroth bpkroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @apavlo, any further comments?

@apavlo apavlo self-requested a review December 7, 2023 18:16
apavlo
apavlo approved these changes Dec 7, 2023
View reviewed changes
Copy link
Member

@apavlo apavlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@apavlo apavlo merged commit 6ba8da3 into cmu-db:main Dec 7, 2023
121 checks passed
@bpkroth bpkroth mentioned this pull request Dec 8, 2023
Merged
bpkroth added a commit that referenced this pull request Dec 8, 2023
@bpkroth
Add oracle to the set of produced benchbase docker images (#415)
65322e9 
Small tweak missed from #379
This was referenced Dec 14, 2023
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@bpkroth bpkroth mentioned this pull request Jun 17, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bpkroth bpkroth bpkroth approved these changes

@apavlo apavlo apavlo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ngbanguyen @apavlo @bpkroth