Skip to content

HHH-13843: make schema migration faster #3222

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

Closed
wants to merge 4 commits into from
Closed

HHH-13843: make schema migration faster #3222

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cdde935
HHH-13843: make schema migration faster
Francoisvandelft Feb 6, 2020
8782b16
HHH-13843: processed first review comment.
Francoisvandelft Feb 7, 2020
3b1ae57
HHH-13843: Code style. => sorry
Francoisvandelft Feb 7, 2020
9cc6b7b
HHH-13843: Remove unused import
Francoisvandelft Feb 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions ...c/main/java/org/hibernate/tool/schema/extract/internal/DatabaseInformationCachedImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* Hibernate, Relational Persistence for Idiomatic Java
*
* License: GNU Lesser General Public License (LGPL), version 2.1 or later.
* See the lgpl.txt file in the root directory or <http://www.gnu.org/licenses/lgpl-2.1.html>.
*/
package org.hibernate.tool.schema.extract.internal;

import java.sql.SQLException;

import org.hibernate.boot.model.relational.Namespace;
import org.hibernate.boot.model.relational.QualifiedTableName;
import org.hibernate.engine.jdbc.env.spi.JdbcEnvironment;
import org.hibernate.resource.transaction.spi.DdlTransactionIsolator;
import org.hibernate.service.ServiceRegistry;
import org.hibernate.tool.schema.extract.spi.NameSpaceTablesInformation;
import org.hibernate.tool.schema.extract.spi.TableInformation;

/**
* Will cache all database information by reading the tableinformation from the name space in one call.
* Schema migration becomes much faster when this object is used.
* NOTE: superclass already caches the sequence information, so perhaps cache can also be coded in superlcass
* @author francois
*/
public class DatabaseInformationCachedImpl extends DatabaseInformationImpl {
private NameSpaceTablesInformation defaultNameSpaceTablesInformation = null;
Copy link
Contributor

@NathanQingyangXu NathanQingyangXu Feb 6, 2020

Choose a reason for hiding this comment

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

I am pretty sure we don't need to set null above. It has been set during object creation by JVM already.

Copy link
Contributor Author

@Francoisvandelft Francoisvandelft Feb 7, 2020

Choose a reason for hiding this comment

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

fixed

private final Namespace defaultNamespace;

/**
*
* @param serviceRegistry ServiceRegistry
* @param jdbcEnvironment JdbcEnvironment
* @param ddlTransactionIsolator DdlTransactionIsolator
* @param defaultNamespace NameSpace
*/
public DatabaseInformationCachedImpl(
ServiceRegistry serviceRegistry,
JdbcEnvironment jdbcEnvironment,
DdlTransactionIsolator ddlTransactionIsolator, Namespace defaultNamespace)
throws SQLException {
super(serviceRegistry, jdbcEnvironment, ddlTransactionIsolator, defaultNamespace.getName());
this.defaultNamespace = defaultNamespace;
Copy link
Contributor

@NathanQingyangXu NathanQingyangXu Feb 6, 2020

Choose a reason for hiding this comment

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

Hibernate has specific code style, especially we use empty char aggressively. We have a ide style repo even: https://github.com/hibernate/hibernate-ide-codestyles.

You might need to refer to existing code for the style issue, especially you need to insert more space chars.

}

@Override
public TableInformation getTableInformation(QualifiedTableName qualifiedTableName) {

if (defaultNameSpaceTablesInformation == null) {
// Load table information from the whole space in one go (expected to be used in case of schemaMigration, so (almost) all
// tables are likely to be retrieved)
defaultNameSpaceTablesInformation = getTablesInformation(defaultNamespace);
}
TableInformation tableInformation = defaultNameSpaceTablesInformation.getTableInformation(qualifiedTableName.getTableName().getText());
if (tableInformation != null) {
return tableInformation;
}
return super.getTableInformation(qualifiedTableName); // result of this call can of course also be cached, does not seem to be in scope now
}

}
8 changes: 3 additions & 5 deletions hibernate-core/src/main/java/org/hibernate/tool/schema/internal/AbstractSchemaMigrator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,7 @@ public void doMigration(Metadata metadata, ExecutionOptions options, TargetDescr
final JdbcContext jdbcContext = tool.resolveJdbcContext( options.getConfigurationValues() );
final DdlTransactionIsolator ddlTransactionIsolator = tool.getDdlTransactionIsolator( jdbcContext );
try {
final DatabaseInformation databaseInformation = Helper.buildDatabaseInformation(
tool.getServiceRegistry(),
ddlTransactionIsolator,
metadata.getDatabase().getDefaultNamespace().getName()
);
final DatabaseInformation databaseInformation = getDatabaseInformation(ddlTransactionIsolator, metadata.getDatabase().getDefaultNamespace());

final GenerationTarget[] targets = tool.buildGenerationTargets(
targetDescriptor,
Expand Down Expand Up @@ -139,6 +135,8 @@ public void doMigration(Metadata metadata, ExecutionOptions options, TargetDescr
}
}

protected abstract DatabaseInformation getDatabaseInformation(DdlTransactionIsolator ddlTransactionIsolator, Namespace namespace);
Copy link
Contributor

@NathanQingyangXu NathanQingyangXu Feb 6, 2020

Choose a reason for hiding this comment

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

Hibernate strives to maintain backward compatibility so new method is supposed to be introduced via JDK8's default interface method mechanism (our client might be compiling based on the public methods we have provided and this new abstract method might break all their existing code).

Copy link
Member

@dreab8 dreab8 Feb 7, 2020

Choose a reason for hiding this comment

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

it is an internal so it is not a problem

Copy link
Contributor Author

@Francoisvandelft Francoisvandelft Feb 7, 2020

Choose a reason for hiding this comment

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

I will update the pull request, and make this method none-abstract, which will implement the "current" behavior. " final DatabaseInformation databaseInformation = Helper.buildDatabaseInformation(
tool.getServiceRegistry(),
ddlTransactionIsolator,
metadata.getDatabase().getDefaultNamespace().getName()
);"

and I will overwrite this method in the "GroupedSchemaMigratorImpl"

As a result the implementation of "IndividuallySchemaMigratorImpl" and "CheckForExistingForeignKeyTest" are no longer affected by this pull request.

Copy link
Contributor

@NathanQingyangXu NathanQingyangXu Feb 7, 2020

Choose a reason for hiding this comment

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

it is an internal so it is not a problem

Thanks. I forgot it is in 'internal' package.

Copy link
Contributor

@NathanQingyangXu NathanQingyangXu Feb 6, 2020

Choose a reason for hiding this comment

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

Rather than introducing a non-compatible abstract method, is it possible to add caching mechanism in Helper.buildDatabaseInformation() internally? Seems more straightforward and safe.


protected abstract NameSpaceTablesInformation performTablesMigration(
Metadata metadata,
DatabaseInformation existingDatabase,
Expand Down
18 changes: 18 additions & 0 deletions ...nate-core/src/main/java/org/hibernate/tool/schema/internal/GroupedSchemaMigratorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,18 @@
*/
package org.hibernate.tool.schema.internal;

import java.sql.SQLException;
import java.util.Set;

import org.hibernate.boot.Metadata;
import org.hibernate.boot.model.naming.Identifier;
import org.hibernate.boot.model.relational.Namespace;
import org.hibernate.dialect.Dialect;
import org.hibernate.engine.jdbc.env.spi.JdbcEnvironment;
import org.hibernate.engine.jdbc.internal.Formatter;
import org.hibernate.mapping.Table;
import org.hibernate.resource.transaction.spi.DdlTransactionIsolator;
import org.hibernate.tool.schema.extract.internal.DatabaseInformationCachedImpl;
import org.hibernate.tool.schema.extract.spi.DatabaseInformation;
import org.hibernate.tool.schema.extract.spi.NameSpaceTablesInformation;
import org.hibernate.tool.schema.extract.spi.TableInformation;
Expand All @@ -35,6 +39,20 @@ public GroupedSchemaMigratorImpl(
super( tool, schemaFilter );
}

protected DatabaseInformation getDatabaseInformation(DdlTransactionIsolator ddlTransactionIsolator, Namespace namespace) {
final JdbcEnvironment jdbcEnvironment = tool.getServiceRegistry().getService( JdbcEnvironment.class );
try {
return new DatabaseInformationCachedImpl(
tool.getServiceRegistry(),
jdbcEnvironment,
ddlTransactionIsolator,
namespace);
}
catch (SQLException e) {
throw jdbcEnvironment.getSqlExceptionHelper().convert( e, "Unable to build DatabaseInformationCached" );
}
}

@Override
protected NameSpaceTablesInformation performTablesMigration(
Metadata metadata,
Expand Down
5 changes: 5 additions & 0 deletions ...core/src/main/java/org/hibernate/tool/schema/internal/IndividuallySchemaMigratorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.hibernate.dialect.Dialect;
import org.hibernate.engine.jdbc.internal.Formatter;
import org.hibernate.mapping.Table;
import org.hibernate.resource.transaction.spi.DdlTransactionIsolator;
import org.hibernate.tool.schema.extract.spi.DatabaseInformation;
import org.hibernate.tool.schema.extract.spi.NameSpaceTablesInformation;
import org.hibernate.tool.schema.extract.spi.TableInformation;
Expand All @@ -35,6 +36,10 @@ public IndividuallySchemaMigratorImpl(
super( tool, schemaFilter );
}

protected DatabaseInformation getDatabaseInformation(DdlTransactionIsolator ddlTransactionIsolator, Namespace namespace) {
return Helper.buildDatabaseInformation(tool.getServiceRegistry(), ddlTransactionIsolator, namespace.getName());
}

@Override
protected NameSpaceTablesInformation performTablesMigration(
Metadata metadata,
Expand Down
8 changes: 8 additions & 0 deletions ...src/test/java/org/hibernate/test/tool/schema/internal/CheckForExistingForeignKeyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.hibernate.mapping.Column;
import org.hibernate.mapping.ForeignKey;
import org.hibernate.mapping.Table;
import org.hibernate.resource.transaction.spi.DdlTransactionIsolator;
import org.hibernate.tool.schema.extract.internal.ColumnInformationImpl;
import org.hibernate.tool.schema.extract.internal.ForeignKeyInformationImpl;
import org.hibernate.tool.schema.extract.internal.TableInformationImpl;
Expand Down Expand Up @@ -49,6 +50,13 @@ public SchemaMigrator() {
super( null, null );
}

@Override
protected DatabaseInformation getDatabaseInformation(
DdlTransactionIsolator ddlTransactionIsolator, Namespace namespace)
{
return null;
}

/**
* Needed implementation. Not used in test.
*/
Expand Down