Why does SqlDatabaseSnapshot "remove PK indexes"

I’ve create several tables in Postgres with Indexes, Primary Keys and Foreign Keys.  When I run generateChangeLog it wasn’t creating all of the indexes that I specified in PGAdmin.  While stepping through the LiquiBase code (v 1.9.4), I found that it is removing any indexes that are associated with Primary Keys (PKs), Foreign Keys (FKs) and Unique Constraints. 

        Set indexesToRemove = new HashSet();         //remove PK indexes         for (Index index : indexes) {             for (PrimaryKey pk : primaryKeys) {                 if (index.getTable().getName().equalsIgnoreCase(pk.getTable().getName()) && index.getColumnNames().equals(pk.getColumnNames())) {                     indexesToRemove.add(index);                 }             }             for (ForeignKey fk : foreignKeys) {                 if (index.getTable().getName().equalsIgnoreCase(fk.getForeignKeyTable().getName()) && index.getColumnNames().equals(fk.getForeignKeyColumns())) {                     indexesToRemove.add(index);                 }             }             for (UniqueConstraint uc : uniqueConstraints) {                 if (index.getTable().getName().equalsIgnoreCase(uc.getTable().getName()) && index.getColumnNames().equals(uc.getColumnNames())) {                     indexesToRemove.add(index);                 }             }

        }
        indexes.removeAll(indexesToRemove);

I talked with other developers and our DBA and this is not desirable, at least for what we need.

Based on the code, I don’t see an option to turn this functionality off.  Am I using it incorrectly?  I know that I can manually at the XML needed to create the indexes, but it seems to me that I shouldn’t have to do that.  Is this an education issue on my part?

Thank you.

Also, to help me understand the code a bit better, what it the reasoning for removing those index types for the main list?

This bug was found in November last year and fix was sent as patch. In the latest version i see same bug.
In index comparison index name is not taken in consideration, but only index columns which is wrong.

Originally posted by: alexthe1st
This bug was found in November last year and fix was sent as patch. In the latest version i see same bug. In index comparison index name is not taken in consideration, but only index columns which is wrong.

Thank you alexthe1st.

Do you know what patch that is in?

My version of this code for postgresql is following

       for (Index index : indexes) {        for (PrimaryKey pk : primaryKeys) {            if (index.getTable().getName().equalsIgnoreCase(pk.getTable().getName())                    && index.getColumnNames().equals(pk.getColumnNames()) && index.getName().equalsIgnoreCase(pk.getName())) {                indexesToRemove.add(index);            }        }        for (ForeignKey fk : foreignKeys) {            if (index.getTable().getName().equalsIgnoreCase(fk.getForeignKeyTable().getName())                    && index.getColumnNames().equals(fk.getForeignKeyColumns()) && index.getName().equalsIgnoreCase(fk.getName())) {                indexesToRemove.add(index);            }        }        for (UniqueConstraint uc : uniqueConstraints) {          if (index.getTable().getName().equalsIgnoreCase(uc.getTable().getName()) && index.getColumnNames().equals(uc.getColumnNames()) && index.getName().equalsIgnoreCase(uc.getName())) {            indexesToRemove.add(index);          }        }            }

A patch is referred to above.  What JIRA issue is this?  Does this affect ALL databases?

Thanks,
  Jamie

You are thinking that the indexes on FK columns etc should be created as well, right?

The trouble is that different databases have different ways they handle indexes and PKs/FKs, etc.  For example, MySQL automatically creates indexes for FK columns and it will throw an error if you create the FK and create the index, even though both are returned through the JDBC metadata.  SqlServer, on the other hand, does not create indexes for FK columns and so you would want to create both. 

I think we are going to have to do a bit of work around better control of the diff tool, which is scheduled for 2.1.

I created an issue to track it: http://liquibase.jira.com/browse/CORE-499

For now, you’ll have to modify your generated changelog manually (if there are not too many indexes) or patch the code to remove the check.

Nathan 

I was stopped by this issue too :smiley:

But I have an idea!! ;D

What if we add new attribute to index-creation xml-element, something like:

    associated-with
or
    key
that will contain only key-word: primaryKey, foreignKey, uniqueConstraint or none. Then fill it by these keys instead of adding index to indexesToRemove set. We just mark it!

After that we can add checking of this field in index sql-generation process and then execute or skip and ignore creation of this index according to DB type.

Results:

  1. Generation of index creation will run in all cases.
  2. SQL-Creation of indexes, which marked by new attribute, will run only if database rules requires manual creation of indexes for PK, FK, UC.

This solution have some bad points. It seems, that changeLog may have changeSet with task which was not executed.

If it is an acceptable way - i’ll try to fix it by my own.

Hm… Only now i found: this kind of solution is not compatible for early liquibase versions…

then +1 question :slight_smile:
Is there some way to set requirement level of elements or attributes?
Something like that:

usage=“optional” - then element may_be/may_be_not initialized through xml description
usage=“required” - then element must be initialized through xml description

I’m ready to commit these fixations.

I added associatedWith attribute to createIndex xml-element.
On this stage it’s only xsd:string typed value. I marked it as optional-use in xsd.
For example:

or

By default, it have empty value:

    .. associatedWith="" ..

After that - absolutely all indexes have been set to ChangeSets.

And finally, i have inserted this statement in CreateIndexGenerator.generateSql method:

    if (database instanceof OracleDatabase) { List associatedWith = StringUtils.splitAndTrim(statement.getAssociatedWith(), ","); if (associatedWith.contains(Index.MARK_PRIMARY_KEY) || associatedWith.contains(Index.MARK_UNIQUE_CONSTRAINT)) { return new Sql[0]; } }

Also, I can add else statement to keep old sql-generation way for other Databases. Something like that:

    if (database instanceof OracleDatabase) { List associatedWith = StringUtils.splitAndTrim(statement.getAssociatedWith(), ","); if (associatedWith.contains(Index.MARK_PRIMARY_KEY) || associatedWith.contains(Index.MARK_UNIQUE_CONSTRAINT)) { return new Sql[0]; } } else { if (associatedWith.contains(Index.MARK_PRIMARY_KEY) || associatedWith.contains(Index.MARK_UNIQUE_CONSTRAINT) || associatedWith.contains(Index.MARK_FOREIGN_KEY)) { return new Sql[0]; } }

Is it suitable?

That is a very good idea.  Go ahead and commit it if you haven’t yet.

Sorry for the slow responses lately.  Busy couple weeks at work…

Nathan

I fixed it and commited.

Pls, monitor this commit carefully :slight_smile: I’m afraid to break something ))

Thanks, I’ll take a look.  It looks like it’s causing derby to fail, I can fix that up.  For future reference, the H2IntegrationTest, HsqlIntegrationTest and DerbyIntegrationTest classes will run using embedded databases so you can test against a few other systems without needing to install anything.

Nathan

The latest version is causing a NPE when the associatedWith attribute is undefined in combination with Oracle. I fixed it in SVN

Thanks for fixing that.

Nathan