Improve schema update for H2 and PostgreSQL and document various dev approaches

[rbaumgar]

application.properties

quarkus.datasource.url=jdbc:h2:tcp://localhost/~/test
quarkus.datasource.driver=org.h2.Driver
quarkus.hibernate-orm.database.generation=update

Developer.java

package org.acme.quickstart;
import javax.persistence.Entity;
import io.quarkus.hibernate.orm.panache.PanacheEntity;
@Entity
public class Developer extends PanacheEntity{
    public String name;

    //public String favoriteFramework = "Quarkus";

}

When I uncomment favoriteFramework, I get

14:40:29,749 WARN  [org.hib.too.sch.int.ExceptionHandlerLoggedImpl] GenerationTarget encountered exception accepting command : Error executing DDL "create table Developer (id bigint not null, favoriteFramework varchar(255), name varchar(255), primary key (id))" via JDBC Statement: org.hibernate.tool.schema.spi.CommandAcceptanceException: Error executing DDL "create table Developer (id bigint not null, favoriteFramework varchar(255), name varchar(255), primary key (id))" via JDBC Statement
...
Caused by: org.h2.jdbc.JdbcSQLException: Table "DEVELOPER" already exists; SQL statement:
create table Developer (id bigint not null, favoriteFramework varchar(255), name varchar(255), primary key (id)) [42101-197]

Actions todo #1886 (comment)

• H2 to PostgreSQL -> lowercase
• Document drop-and-create vs update

[FroMage]

@gsmet could this be related to Panache? Or to update vs drop-create?

[Sanne]

hi @rbaumgar ! could you please verify if you still have this problem with version 0.13 ?

If you still have it, could you give us some more details? I couldn't reproduce it on version 0.13.

thanks

[FroMage]

This seems to be due to the naming strategy and databases using case-insensitive table names, which the update mode can't find back. We will try to find a solution to make this work OOTB.

[rbaumgar]

Still in 0.13
mvn compile quarkus:dev

[INFO] Scanning for projects...
[INFO] 
[INFO] ----------------------< org.acme:getting-started >----------------------
[INFO] Building getting-started 1.0-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- maven-resources-plugin:2.6:resources (default-resources) @ getting-started ---
[INFO] Using 'UTF-8' encoding to copy filtered resources.
[INFO] Copying 2 resources
[INFO] 
[INFO] --- maven-compiler-plugin:3.1:compile (default-compile) @ getting-started ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 2 source files to /home/rbaumgar/demo/quarkus/demo/target/classes
[INFO] 
[INFO] --- quarkus-maven-plugin:0.13.0:dev (default-cli) @ getting-started ---
Listening for transport dt_socket at address: 5005
2019-04-08 18:48:36,189 INFO  [io.qua.dep.QuarkusAugmentor] (main) Beginning quarkus augmentation
2019-04-08 18:48:36,741 WARN  [io.qua.dep.ind.ApplicationArchiveBuildStep] (build-12) Re-indexing /home/rbaumgar/.m2/repository/io/quarkus/quarkus-hibernate-orm-panache/0.13.0/quarkus-hibernate-orm-panache-0.13.0.jar - at least Jandex 2.1 must be used to index an application dependency
2019-04-08 18:48:37,546 INFO  [io.qua.dep.QuarkusAugmentor] (main) Quarkus augmentation completed in 1357ms
2019-04-08 18:48:39,041 WARN  [org.hib.too.sch.int.ExceptionHandlerLoggedImpl] (main) GenerationTarget encountered exception accepting command : Error executing DDL "create table Developer (id bigint not null, name varchar(255), primary key (id))" via JDBC Statement: org.hibernate.tool.schema.spi.CommandAcceptanceException: Error executing DDL "create table Developer (id bigint not null, name varchar(255), primary key (id))" via JDBC Statement
	at org.hibernate.tool.schema.internal.exec.GenerationTargetToDatabase.accept(GenerationTargetToDatabase.java:67)
...
Caused by: org.h2.jdbc.JdbcSQLException: Table "DEVELOPER" already exists; SQL statement:
create table Developer (id bigint not null, name varchar(255), primary key (id)) [42101-197]
	at org.h2.message.DbException.getJdbcSQLException(DbException.java:357)
...

[yanaga]

Works on MariaDB. Probably related to case-sensitive/insensitive identifiers on the database. In this case, H2 - as pointed on the issue.

[Sanne]

Right the problem is the way H2 and PostgreSQL handle capitalization.

I don't think we can fix this in Quarkus; using "update" requires some care:

• be precise with naming, for example:
  • explicitly set a lowercase name for the table using the @Table
  • choose a custom naming strategy which does the lowercasing for you
  • reconfigure the database to deal with this differently...
  • you could set org.hibernate.cfg.AvailableSettings.GLOBALLY_QUOTED_IDENTIFIERS
• don't expect "update" to work consistently with non-empty databases; for this reason it's probably not ideal to be used in combination with Quarkus's "dev mode".

For quarkus:dev I'd recommend to always use "drop-and-create", this will also force you to keep the import.sql file aligned with your model changes - this might look like an inconvenience but IMO it's cool as it brings to your attention any unexpected incompatible schema change.

Regarding org.hibernate.cfg.AvailableSettings.GLOBALLY_QUOTED_IDENTIFIERS : I'm wondering if we should enable this by default but it's a high risk change. Significantly different than what people expect from Hibernate ORM, and it then requires to use weird quoting all over consistently, should you want to access the database with other tools that's inconvenient.

[Sanne]

P.S. I think we'll have to close this issue but I'm happy to keep it open for a bit longer, so to have a conversation about what should be done - maybe no changes at all, but we could consider at least logging a warning when people combine "update" within quarkus:dev.

[burrsutter]

I wish to use "update" for live code demos, therefore this is no import.sql (takes too long to key that in). If I understand the issue, Java likes classes that begin with a capital letter (e.g. Customer.java) but postgres and H2 want that to be a lower-case "c". Can we not just force table names to lower case? Also, if the table/column name is a reserved word, can we get an better error message?

[emmanuelbernard]

Here is the compromise we will lean towards

1. write in the guide that drop-and-create is great for tests and import.sql driven people
2. if you work on "prod" data then you have the following choice
   a. none which does what it says
   b. update which is really mostly-update and we document the types of cases where it would fail and what to do to workaround conceptually
3. to make update work for the one issue we have see, we:
   a. do the hack I mention in the update logic to check for the lowercase version of an object in case the cased version is not found
   b. force a different default naming strategy which goes lower case (we changed it so there will be cases where it's not what the user will want)

3.b. would work to a certain extend, PostgreSQL and MySQL are probably going to be fine. Older database with more baggage like DB2 or Oracle will possibly not work and the user would have to set a different naming strategy or use @Table / @Column

[emmanuelbernard]

@burrsutter I do demo with import.sql, not sure why you see this as incompatible.

[burrsutter]

import.sql is not incompatible but the file is just not present as it takes too long to type from scratch, not too mention it is yet another syntax to be learned

[FroMage]

FTR I tried to change the implicit naming strategy to both legacy-jpa and legacy-hbm and this did not solve the problem.

[FroMage]

So, in NameSpaceTablesInformation I have this code:

	public TableInformation getTableInformation(Table table) {
		return tables.get( identifierHelper.toMetaDataObjectName( table.getQualifiedTableName().getTableName() ) );
	}

identifierHelper.toMetaDataObjectName( table.getQualifiedTableName().getTableName() ) returns DirectEntity while tables contains directentity.

identifierHelper is a NormalizingIdentifierHelperImpl with unquotedCaseStrategy set to MIXED which is either wrong and should be LOWER, or it's tables that has been filled wrong.

[FroMage]

It looks like we call dialect.buildIdentifierHelper(..., null) so we never detect the casing strategy from the DB and we default to MIXED. But that must be wrong, surely, since PG will lower-case everything not quoted for us.

[FroMage]

@emmanuelbernard or @Sanne could you pass this over to the right person in Hibernate that could tell us what to do if we wanted to support update for postgres and h2?

[FroMage]

I can't find in the code in JdbcEnvironmentImpl where it would actually use the naming strategy.

[FroMage]

OK, so here's a workaround:

public class QuarkusPostgreSQL95Dialect extends PostgreSQL95Dialect {

    @Override
    public IdentifierHelper buildIdentifierHelper(IdentifierHelperBuilder builder, DatabaseMetaData dbMetaData)
            throws SQLException {
        builder.setUnquotedCaseStrategy(IdentifierCaseStrategy.LOWER);

        return super.buildIdentifierHelper(builder, dbMetaData);
    }
}

And in HibernateOrmProcessor:

    private Optional<String> guessDialect(Optional<String> driver) {
        // For now select the latest dialect from the driver
        // later, we can keep doing that but also avoid DCE
        // of all the dialects we want in so that people can override them
        String resolvedDriver = driver.orElse("NODRIVER");
        if (resolvedDriver.contains("postgresql")) {
            return Optional.of(QuarkusPostgreSQL95Dialect.class.getName());
        }
...

This gets me unstuck. I wonder why the postgres dialect does not do that by default? I've never seen a postgres DB configured with unquoted mixed case.

[emmanuelbernard]

I spoke with @Sanne and we will change Quarkus ORM extension behavior to adapt the naming strategy to the dialect. For Postgres, we would go lower case by default. With the expectation that users can change the naming strategy.

[emmanuelbernard]

This we think will be a better approach that hacking Hibernate or subclassing dialects.

[emmanuelbernard]

H2 and Postgres go for lower case

[FroMage]

I have tried setting the naming strategy to no effect. I also did not find in the code where the schema-update code uses the strategy, which may be related to the lack of effect.

[gsmet]

@emmanuelbernard but that does mean that it will still not work by default with Hibernate ORM?

What I'm wondering is if the change suggested by @FroMage should go in ORM proper. It seems like the behavior we would want.

[emmanuelbernard]

@FroMage I'll investigate that

[emmanuelbernard]

@gsmet with ORM 6 coming I think I'm fine with that compromise. Can't cure world hunger in one commit.

[FroMage]

@gsmet with ORM 6 coming I think I'm fine with that compromise. Can't cure world hunger in one commit.

If we do the change here and people love it, it makes it easier to justify porting it upstream later, for a lower risk to non-Quarkus ORM users.

[emmanuelbernard]

Ok so update for everyone. It turns out that the Dialect has a way to tell whether a DB is upper case or lower case for its identifiers and that setup is separate for quoted identifiers and non quoted ones.

H2 seems to be uppercase for non quoted, and mixed for quoted
Postgresql seems to be lowercase for non quoted, and mixed for quoted

So changing the dialect tree probably would be a better approach than changing the physical naming strategy
CC @Sanne rings any bell?

[emmanuelbernard]

@Sanne @gsmet @FroMage I looked at the code and I now agree that the dialect IdentifierHelperBuilder is most likely the right way to go. I've created a pull request with Quarkus specific dialects.
I also documented how a developer would chose between quarkus.hibernate-orm.database.generation none, update, drop-and-create. Let me know if that needs improvements.

I think the dialect changes should go in ORM upstream but with Steve and Andrea's load I did not want it to be a roadbloacker. It would be good to get their perspective.
For PostgreSQL, we should patch not the 95 dialect but the original top level PostgreSQL81Dialect
I even wonder if the default value (baring reading from the database metadata) should be UPPER as Thomas Mueller from H2 seems to imply that it's the ANSI SQL-92 default and NormalizingIdentifierHelperImpl seem to have that assumption. The NormalizingIdentifierHelperImpl is overridden though because IdentifierHelperBuilder defaults to MIXED.

[emmanuelbernard]

CC @gsmet
@Sanne told me the reason why he thinks the Identifier is not a proper solution at least for upstream.
According to him, one can change the behavior of the DB wrt lowercase / uppercase interpretation. I had not seen a flag in PostgreSQL but H2 has one. So the dialects hard codes the default value. Since we can't parameterize the dialects that's too strong of a choice to push upstream in the Pg and H2 dialects proper.

For now it's fine to put it like that in Quarkus. And we can decide to go for the physical naming strategy approach later. Or the strategy I proposed which was to ignore casing for DB comparisons. The latter we can add a flag for the few that do DB casing for some reason.
Anyways, not ready to push ORM upstream.

I'd love to see the Pg DB config options around that though as I think it might go against SQL-92 to enforce casing on unquoted identifiers.

[Sanne]

+1 for such a solution in quarkus. Power users can switch dialects.