[536] [913] [910] SchemaMigrator/SchemaValidator support for PostgreSQL and CockroachDB #903

gbadner · 2021-07-24T00:20:13Z

Requires ORM PR:
hibernate/hibernate-orm#4106

DavideD · 2021-07-26T14:14:06Z

I will convert this to a draft, it contains changes to the gradle.properties file that we don't want on the main branch. And also because we have to wait for ORM to include the PR that we need.

DavideD · 2021-07-26T14:19:42Z

...rnate-reactive-core/src/test/java/org/hibernate/reactive/SchemaUpdatePostgreSqlTestBase.java

+												.thenAccept( result ->
+																	 context.assertEquals(
+																			 result,
+																			 "CREATE UNIQUE INDEX aanother_pkey ON public.aanother USING btree (id)"


Not critical, but when using context.equals(expected, actual), the firs value is the expected one and the second the actual one.

I normally do it that way, I don't know why I didn't in this test. I'll change it.

@DavideD, I pushed a commit that corrects the order.

DavideD · 2021-07-26T14:57:02Z

hibernate-reactive-core/src/main/java/org/hibernate/reactive/pool/ReactiveConnection.java

@@ -50,6 +50,7 @@
 	CompletionStage<Result> select(String sql);
 	CompletionStage<Result> select(String sql, Object[] paramValues);
 	CompletionStage<ResultSet> selectJdbc(String sql, Object[] paramValues);
+	CompletionStage<ResultSet> selectJdbcOutsideTransaction(String sql, Object[] paramValues);


Could you remind us why you need the additional method?

@DavideD , since SchemaMigrator executes select queries on system tables and executes DDL statements (which update system tables), I assumed that these queries should be executed outside of a transaction (i.e., with autocommit=true). IIUC, this would help avoid contention with other work the DB is doing on schemas used by various applications.

If you look at the corresponding ORM code in GenerationTargetToDatabase, you can see that the statement is created using a connection obtained from a DdlTransactionIsolator.

Javadoc for DdlTransactionIsolator says:

* Provides access to a Connection that is isolated from * any "current transaction" with the designed purpose of * performing DDL commands

If you look at the DdlTransactionIsolator implementations, you will see that autocommit is set to true on the Connection that is obtained:
DdlTransactionIsolatorJtaImpl
DdlTransactionIsolatorNonJtaImpl
DdlTransactionIsolatorProvidedConnectionImpl relies on JdbcConnectionAccessProvidedConnectionImpl which sets autocommit to true

I didn't see any other way to execute a query outside of a transaction that returned a CompletionStage<ResultSet>, so I created a new method.

Is there some other method that should be used instead?

I don't think we had this requirement before. I don't see any problem adding a new method.

But I mainly wanted a bit of an explanation written somewhere so that others know as well (maybe it wasn't obvious only to me though ;-)

At some point, we will need to add some javadoc to that class. Anyway, not something you need to worry about at the moment.

Yes, I know this stuff is lacking in documentation. I will be adding that.

gavinking · 2021-07-27T10:14:17Z

I have not tested it but the code looks great! Thanks @gbadner

(The only very minor comment I have is I would prefer that SQL keywords were in lowercase like the rest of the SQL we generate.)

gbadner · 2021-07-27T20:02:03Z

@DavideD, thanks for doing making it a draft. I thought I defined it as a draft when I created it, but maybe I missed that.

gbadner · 2021-07-27T23:46:14Z

I have not tested it but the code looks great! Thanks @gbadner

Thanks @gavinking!

(The only very minor comment I have is I would prefer that SQL keywords were in lowercase like the rest of the SQL we generate.)

That's fine. I can change that.

gbadner · 2021-07-27T23:59:59Z

I'm currently using the Hibernate ORM codestyle. Which of the codestyles do you use for Hibernate Reactive?

gbadner · 2021-07-28T00:16:55Z

I finally got around to taking a good look at what else needs to be done for SchemaValidator, and, AFAICT, it's already working with the code provided here!?!

gbadner · 2021-07-28T03:04:04Z

@gavinking, I added a new commit that changes SQL keywords to lower-case.

gavinking · 2021-07-28T08:40:24Z

Thanks, @gbadner

DavideD · 2021-07-28T12:27:16Z

@DavideD, thanks for doing making it a draft. I thought I defined it as a draft when I created it, but maybe I missed that.

no problem

I'm currently using the Hibernate ORM codestyle. Which of the codestyles do you use for Hibernate Reactive?

Yes, that's what we should use. Thanks.

Although, our build doesn't check the code style (except of a couple of basic rules) so you might find that we are a bit inconsistent sometimes.

gbadner · 2021-08-04T05:13:41Z

@DavideD, I just pushed a commit that adds Javadoc and a couple of small tweaks.

I forgot that I had already pushed a commit for encapsulating methods into ExtractionTool and ended up squashing the two commits, so I had to force-push.

Sanne is still looking at the ORM PR. I think this PR is ready to review, although we will obviously have to wait for the ORM PR to be finalized.

Unless I hear of any required changes to this PR, I will start work on adding support for MySQL.

hibernate-reactive-core/src/main/java/org/hibernate/reactive/adaptor/impl/ResultSetAdaptor.java

gavinking · 2021-08-04T08:51:18Z

This all looks very nice, @gbadner!

gbadner · 2021-08-05T04:50:01Z

Thanks @gavinking, I'm pretty happy with it.

...main/java/org/hibernate/reactive/provider/service/ReactiveImprovedExtractionContextImpl.java

...hibernate/reactive/provider/service/AbstractReactiveInformationSchemaBasedExtractorImpl.java

gbadner · 2021-08-05T20:21:28Z

I just added a commit to deal with a non-standard information_schema.columns column name used by PostgreSQL.

gbadner · 2021-08-30T20:14:13Z

Merged commits from #940.

gavinking · 2021-08-31T08:56:58Z

Merged commits from #940.

Great.

gavinking · 2021-08-31T09:06:08Z

hibernate-reactive-core/src/test/java/org/hibernate/reactive/BaseReactiveTest.java

@@ -124,17 +126,17 @@ protected Configuration constructConfiguration() {
 			doneTablespace = true;
 		}
 		//Use JAVA_TOOL_OPTIONS='-Dhibernate.show_sql=true'
-		configuration.setProperty( Settings.SHOW_SQL, System.getProperty(Settings.SHOW_SQL, "false") );
+		configuration.setProperty( Settings.SHOW_SQL, System.getProperty(Settings.SHOW_SQL, "true") );


@gbadner could you roll back this change please?

P.S. A more convenient way to enable SQL logging during development is to set:

logger.hibernate.level = debug

in log4j.properties.

@gavinking, this is done.

gavinking · 2021-08-31T09:11:26Z

I'm seeing failures in tests that restrict ORM versions [5.5,5.6).

Is this the only thing blocking us from merging this?

Sanne · 2021-08-31T09:17:17Z

We should no longer test with Hibernate ORM 5.5 since this requires those changes which are exclusively in 5.6

Sanne · 2021-08-31T09:18:34Z

P.S. I've already upgraded Quarkus to use ORM 5.6.0.Beta1 so I'm not concerned about dropping compatibility with 5.5

gavinking · 2021-08-31T09:19:07Z

We should no longer test with Hibernate ORM 5.5 since this requires those changes which are exclusively in 5.6

Yes, I agree.

gavinking · 2021-08-31T09:39:41Z

So am I understanding correctly that as of this change, we now have two redundant runs of the test suite for every CI build?

gavinking · 2021-08-31T09:43:15Z

Anyway, OK, right, @gbadner, this seemed to fix it:

gavinking@7d8e19b

gbadner · 2021-08-31T16:39:25Z

I pushed a commit that disables SQL logging and includes @gavinking 's commit to fix DI. Crossing my fingers that everything passes...

gbadner · 2021-08-31T16:46:01Z

Hurray, all passed!

gavinking · 2021-08-31T18:00:28Z

WDYT shall we merge it?

gbadner · 2021-08-31T18:04:25Z

@gavinking, yes, please do!

gavinking · 2021-08-31T18:57:20Z

It's still a "Draft" :)

gbadner · 2021-08-31T19:05:22Z

Oops, it's "Open" now.

gavinking · 2021-08-31T19:06:48Z

There's a lot of commits here, should we squash?

gbadner · 2021-08-31T19:09:08Z

Sure, I'll squash and push to my branch...

[536] Correct order of arguments to TestContext#assertEquals [536] : Change SQL keywords to lowercase [536] Encapsulate extraction methods into ReactiveExtractionTool [536] : Added Javodoc, made minor changes to sych up with ORM, changed more SQL keywords that were upper-case to lower-case [536] : Use org.hibernate.reactive.pool.impl.Parameters#process for dialect-specific parameter placeholders [536] : Add a method for non-standard information_schema.columns column name used by PostgreSQL [536] Change `ResultSetAdapter#getLong(String)` to be convert String to Long [536] Change `ResultSetAdapter#getLong(String)` to throw the original exception if the value is neither a long nor a String that can be parsed as a long. [913] SchemaMigrator/SchemaValidator support for CockroachDB [536] [913] Comment out settings in gradle.properties [910] SchemaMigrator/SchemaValidator support for MySQL [910] Fix off-by-one bug in ResultSetAdaptor#getXXX(int columnIndex) methods [910] SchemaMigrator/SchemaValidator support for MariaDB [910] Improve Javadoc; simplify MySqlReactiveInformationExtractorImpl methods [536] Disable SQL logging attempt to fix CI build

gavinking · 2021-08-31T19:32:15Z

Excellent! Fantastic to finally have this done!

Thanks @gbadner

Sanne · 2021-09-01T08:27:33Z

Nice!

gbadner requested review from DavideD and gavinking July 24, 2021 00:20

DavideD added the waiting We are waiting for another PR or issue to be solved before merging this one label Jul 26, 2021

DavideD marked this pull request as draft July 26, 2021 14:14

DavideD reviewed Jul 26, 2021

View reviewed changes

gbadner mentioned this pull request Jul 26, 2021

schema update or validation #536

Closed

gbadner mentioned this pull request Jul 27, 2021

HHH-14744 : Refactor contextual information for SchemaManagementTool to be more easily extended by Hibernate Reactive hibernate/hibernate-orm#4106

Merged

gbadner changed the title ~~[536] SchemaMigrator support for PostgreSQL~~ [536] SchemaMigrator/SchemaValidator support for PostgreSQL Jul 28, 2021

gbadner force-pushed the 536-schema-update branch from b85b05f to 7552fa8 Compare August 4, 2021 05:07

gavinking reviewed Aug 4, 2021

View reviewed changes

hibernate-reactive-core/src/main/java/org/hibernate/reactive/adaptor/impl/ResultSetAdaptor.java Show resolved Hide resolved

gbadner commented Aug 5, 2021

View reviewed changes

...main/java/org/hibernate/reactive/provider/service/ReactiveImprovedExtractionContextImpl.java Show resolved Hide resolved

gavinking mentioned this pull request Aug 5, 2021

expose ColumnDescriptor.getTypeName() eclipse-vertx/vertx-sql-client#1020

Closed

gbadner commented Aug 5, 2021

View reviewed changes

...hibernate/reactive/provider/service/AbstractReactiveInformationSchemaBasedExtractorImpl.java Outdated Show resolved Hide resolved

gbadner changed the title ~~[536] [913] SchemaMigrator/SchemaValidator support for PostgreSQL and CockroachDB~~ [536] [913] [910] SchemaMigrator/SchemaValidator support for PostgreSQL and CockroachDB Aug 30, 2021

gavinking requested changes Aug 31, 2021

View reviewed changes

gbadner marked this pull request as ready for review August 31, 2021 19:04

gbadner requested review from gavinking and DavideD August 31, 2021 19:04

gbadner force-pushed the 536-schema-update branch from ee87282 to 0223cb4 Compare August 31, 2021 19:11

gbadner removed the waiting We are waiting for another PR or issue to be solved before merging this one label Aug 31, 2021

gavinking approved these changes Aug 31, 2021

View reviewed changes

gavinking merged commit b5e88aa into hibernate:main Aug 31, 2021

gbadner deleted the 536-schema-update branch August 31, 2021 19:41

DavideD mentioned this pull request Sep 1, 2021

Track latest available ORM 5.6.x #949

Merged

[536] [913] [910] SchemaMigrator/SchemaValidator support for PostgreSQL and CockroachDB #903

[536] [913] [910] SchemaMigrator/SchemaValidator support for PostgreSQL and CockroachDB #903

Conversation

gbadner commented Jul 24, 2021 • edited Loading

DavideD commented Jul 26, 2021 • edited Loading

DavideD Jul 26, 2021

Choose a reason for hiding this comment

gbadner Jul 27, 2021

Choose a reason for hiding this comment

gbadner Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

DavideD Jul 26, 2021

Choose a reason for hiding this comment

gbadner Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

DavideD Jul 28, 2021

Choose a reason for hiding this comment

gbadner Jul 28, 2021

Choose a reason for hiding this comment

gavinking commented Jul 27, 2021

gbadner commented Jul 27, 2021

gbadner commented Jul 27, 2021

gbadner commented Jul 27, 2021

gbadner commented Jul 28, 2021

gbadner commented Jul 28, 2021

gavinking commented Jul 28, 2021

DavideD commented Jul 28, 2021

gbadner commented Aug 4, 2021 • edited Loading

gavinking commented Aug 4, 2021

gbadner commented Aug 5, 2021

gbadner commented Aug 5, 2021

gbadner commented Aug 30, 2021

gavinking commented Aug 31, 2021

gavinking Aug 31, 2021

Choose a reason for hiding this comment

gbadner Aug 31, 2021

Choose a reason for hiding this comment

gavinking commented Aug 31, 2021

Sanne commented Aug 31, 2021

Sanne commented Aug 31, 2021

gavinking commented Aug 31, 2021

gavinking commented Aug 31, 2021

gavinking commented Aug 31, 2021

gbadner commented Aug 31, 2021

gbadner commented Aug 31, 2021

gavinking commented Aug 31, 2021

gbadner commented Aug 31, 2021

gavinking commented Aug 31, 2021

gbadner commented Aug 31, 2021

gavinking commented Aug 31, 2021

gbadner commented Aug 31, 2021

gavinking commented Aug 31, 2021

Sanne commented Sep 1, 2021

gbadner commented Jul 24, 2021 •

edited

Loading

DavideD commented Jul 26, 2021 •

edited

Loading

gbadner Jul 27, 2021 •

edited

Loading

gbadner Jul 27, 2021 •

edited

Loading

gbadner commented Aug 4, 2021 •

edited

Loading