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.

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

Changeset generates wrong CREATE TABLE query for auto increment primary key column #2362

Closed
0x7d7b opened this issue Jan 13, 2022 · 16 comments · Fixed by #2372
Closed

Changeset generates wrong CREATE TABLE query for auto increment primary key column #2362

0x7d7b opened this issue Jan 13, 2022 · 16 comments · Fixed by #2372

Comments

@0x7d7b
Copy link

0x7d7b commented Jan 13, 2022

Environment

Liquibase Version: 4.7.0
Database Vendor & Version: H2 2.0.206

Description

When using a liquibase changeset containing primary auto increment keys a wrong create table statement is being generated.

Steps To Reproduce

Have a changeset containing e.g.:

<createTable tableName="FOO">
        <column name="ID" type="bigint" autoIncrement="true">
            <constraints nullable="false" primaryKey="true"/>
        </column>
        ...
</createTable>

Actual Behavior

That will produce an error like:

Syntax error in SQL statement "CREATE TABLE MYDB.FOO(ID BIGINT AUTO_INCREMENT[*] NOT NULL, ...

Expected/Desired Behavior

I would expect it to generate a proper query like:

CREATE TABLE MYDB.FOO(ID BIGINT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY, ...
@rburgst
Copy link
Contributor

rburgst commented Jan 14, 2022

have a similar problem, it looks as if the newer keywords in liquibase 2.x are not applied:

     Reason: liquibase.exception.DatabaseException: Syntax error in SQL statement "CREATE TABLE PUBLIC.KEYVALUE (CORRELATIONID UUID NOT NULL, KEY[*] NVARCHAR(50) NOT NULL, DATA NVARCHAR(1024), TYPE NVARCHAR(50) NOT NULL)"; expected "identifier"; SQL statement:

I am not entirely sure why this is happening, since the PR https://github.com/liquibase/liquibase/pull/2285/files#diff-585aea777bfaf5867d624fd65d291f5f3aa3ae3662293eea3a5cb4a94a4b2dabR86 looked as if it would add those new keywords.

From the release notes it looked as if 4.7.0 would include the support for newer h2 version (and therefore also 4281f9c)

@rburgst
Copy link
Contributor

rburgst commented Jan 14, 2022

@nvoxland can you please take a look?

@rburgst
Copy link
Contributor

rburgst commented Jan 14, 2022

ok, actually I am seeing a different problem, when using variables then the reserved keyword quoting does not seem to work

<?xml version="1.0" encoding="UTF-8"?>
<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
                   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                   xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.1.xsd">

    <property name="key" value="[KEY]" dbms="mssql" />
    <property name="key" value="KEY" dbms="h2" />
    <property name="key" value="KEY" dbms="postgresql" />

    <changeSet id="id3" author="author">
        <update tableName="KEYVALUE">
            <column name="TYPE" value="val1" />
            <where>${key}=:value</where>
            <whereParams>
                <param name="value" value="val2" />
            </whereParams>
        </update>
    </changeSet>
</databaseChangeLog>

@rburgst
Copy link
Contributor

rburgst commented Jan 14, 2022

I can also reproduce the problem with autoincrement:

"CREATE TABLE PUBLIC.TABLENAME (ID BIGINT AUTO_INCREMENT[*] NOT NULL, ...)"; 

@nvoxland
Copy link
Contributor

We hadn't updated the "what is the auto-increment clause" logic for h2 because it wasn't failing for me in the tests we have. The old AUTO_INCREMENT clause was working fine so left it at that vs. putting in a version check.

Even using the

        <createTable tableName="FOO">
            <column name="ID" type="bigint" autoIncrement="true">
                <constraints nullable="false" primaryKey="true"/>
            </column>
        </createTable>

example it's working for me.

My h2 url is just jdbc:h2:mem:liquibase. Are there different parameters that may control how well it supports the old AUTO_INCREMENT field?

Changing the logic to use the new GENERATED BY DEFAULT AS IDENTITY works fine against my setup too, though, so I'll open a PR to address that.

@nvoxland
Copy link
Contributor

nvoxland commented Jan 14, 2022

@rburgst and @0x7d7b could you check if the artifacts from https://github.com/liquibase/liquibase/pull/2372/checks resolves the problem for you?

@nvoxland
Copy link
Contributor

ok, actually I am seeing a different problem, when using variables then the reserved keyword quoting does not seem to work

@rburgst, Liquibase doesn't try to add quoting to the the clause you have. As a partial raw SQL statement, we don't have the ability to understand what is going on in there so we just pass it as-is to to the database. You'll have to handle the quoting there yourself.

@nvoxland nvoxland moved this from To Do to Code Review in Conditioning++ Jan 14, 2022
@rburgst
Copy link
Contributor

rburgst commented Jan 14, 2022

@nvoxland I can understand that. So please ignore that part.

As to the autoincrement. It fails for me with the following JDBC URL

jdbc:h2:mem:liquibase;MODE=PostgreSQL;DB_CLOSE_DELAY=-1;DB_CLOSE_ON_EXIT=FALSE;CASE_INSENSITIVE_IDENTIFIERS=TRUE

@0x7d7b
Copy link
Author

0x7d7b commented Jan 17, 2022

@nvoxland I tested your pull request in my environment. The reported problem is now solved by your fix.
But that fix also uncovers an other problem: The INT UNSIGNED data type does not work any longer with the new version in case you have a changeset defined like:

<column defaultValueNumeric="0" name="FOO" type="INT UNSIGNED">
    <constraints primaryKey="true"/>
</column>

It reports:

Syntax error in SQL statement "CREATE TABLE MYDB.BAR(FOO INT UNSIGNED[*] DEFAULT 0 NOT NULL, ...

@0x7d7b
Copy link
Author

0x7d7b commented Jan 17, 2022

@nvoxland BTW in case this is still relevant my test suites JDBC URL looks like this:

jdbc:h2:mem:foo;IGNORECASE=TRUE;MODE=HSQLDB;DB_CLOSE_DELAY=-1;DB_CLOSE_ON_EXIT=TRUE;AUTO_RECONNECT=TRUE;INIT=CREATE SCHEMA IF NOT EXISTS mydb\;SET SCHEMA mydb\;

@suryaaki2 suryaaki2 moved this from Code Review to Ready for Handoff (In JIRA) in Conditioning++ Jan 21, 2022
@0x7d7b
Copy link
Author

0x7d7b commented Feb 8, 2022

Any news on this topic?

@XDelphiGrl
Copy link
Contributor

XDelphiGrl commented Feb 10, 2022

@nvoxland, I discovered two test cases that demonstrate the syntax issues. Both reproduce if you have a JDBC connection that specifies MODE=PostgreSQL. I did a quick test with the fix build; it appears one of the two test cases is addressed.
CC @yodzhubeiskyi


Steps to Reproduce

  • Start an h2 database

    • liquibase init start-h2 (v4.7.0 and up)
    • start-h2 (from the liquibase/examples directory; versions < 4.7.0)
  • Update the h2 JDBC URL in your liquibase.properties file

    • liquibase.command.url=jdbc:h2:tcp://localhost:9090/mem:dev;MODE=PostgreSQL
  • On the h2 database, manually create the table "KEYVALUE"

CREATE TABLE PUBLIC.KEYVALUE (CORRELATIONID UUID NOT NULL, "KEY[*]" NVARCHAR(50) NOT NULL, DATA NVARCHAR(1024), TYPE NVARCHAR(50) NOT NULL);

Actual Results

  • Case1: Fails
Syntax error in SQL statement "UPDATE PUBLIC.KEYVALUE SET TYPE = 'val1' WHERE [*]KEY='val2'"; expected "INTERSECTS, NOT, EXISTS, UNIQUE, INTERSECTS"; SQL statement:
UPDATE PUBLIC.KEYVALUE SET TYPE = 'val1' WHERE KEY='val2' [42001-202] [Failed SQL: (42001) UPDATE PUBLIC.KEYVALUE SET TYPE = 'val1' WHERE KEY='val2']
  • Case2: Fails
Syntax error in SQL statement "CREATE TABLE PUBLIC.PR2372 (""KEY"" BIGINT [*]AUTO_INCREMENT NOT NULL, CONSTRAINT PK_PR2372 PRIMARY KEY (""KEY""))"; expected "ARRAY, INVISIBLE, VISIBLE, NOT, NULL, AS, DEFAULT, GENERATED, ON, NOT, NULL, DEFAULT, NULL_TO_DEFAULT, SEQUENCE, SELECTIVITY, COMMENT, CONSTRAINT, COMMENT, PRIMARY, UNIQUE, NOT, NULL, CHECK, REFERENCES, ,, )"; SQL statement:
CREATE TABLE PUBLIC.PR2372 ("KEY" BIGINT AUTO_INCREMENT NOT NULL, CONSTRAINT PK_PR2372 PRIMARY KEY ("KEY")) [42001-210]

Expected Results

  • Case1 updates the table KEYVALUE successfully.
  • Case2 creates the table PR2372 successfully.
    • Case2 appears to be fixed by the change in the PR.

Test Environment
Liquibase 4.7.0
h2 v.2.0.202

@XDelphiGrl XDelphiGrl assigned nvoxland and unassigned XDelphiGrl Feb 10, 2022
@nvoxland
Copy link
Contributor

The INT UNSIGNED data type does not work any longer
@0x7d7b This appears to be a change in h2 itself. We've always passed the type you give along to the database, and INT UNSIGNED has never really been a valid type for h2. But they've gotten more pedantic in their tracking. You'll have to update your changelog to work with the correct datatype. Since taht changese the liquibase checksum, you'll have to use <validChecksum> to let liquibase know you are OK with the change.

@nvoxland
Copy link
Contributor

I think all the questions in this issue have been addressed.

The overall problem this issue is handling is the fact that on h2 2.x we need to use GENERATED BY DEFAULT and on 1.x we use AUTO_INCREMENT. #2372 fixes that.

Besides that issue, there has been discussion on other unconnected problems which if they were problems should have independent issues for better tracking:

  1. Column that are reserved words in a createTable like key are not being quoted
    a. They are already being correctly quoted in createTable. It's just where where they are not being
  2. Columns in a "where" clause aren't being quoted
    a. This is expected, we don't try to parse the where clause because we can't safely do it. It's up to the user to correctly quote the syntax, even when that changes from one version of their database to the other.
  3. Syntax errors on INT UNSIGNED
    a. This is expected because that is no longer a valid datatype type in newer versions of h2 like it used to be. It's an h2 question, not a liquibase issue.

If anyone is seeing any of those 3 issues still, or any other h2 2.x compatibility issues, please open them as separate issues.

@0x7d7b
Copy link
Author

0x7d7b commented Feb 16, 2022

@nvoxland I changed those INT UNSIGNED types accordingly.

But still: even when I try #2372 the original bug can still be reproduced. I think it has to do with my JDBC URL. There I use the MODE=HSQLDB parameter.

@nvoxland BTW in case this is still relevant my test suites JDBC URL looks like this:

jdbc:h2:mem:foo;IGNORECASE=TRUE;MODE=HSQLDB;DB_CLOSE_DELAY=-1;DB_CLOSE_ON_EXIT=TRUE;AUTO_RECONNECT=TRUE;INIT=CREATE SCHEMA IF NOT EXISTS mydb\;SET SCHEMA mydb\;

@XDelphiGrl XDelphiGrl self-assigned this Feb 16, 2022
@XDelphiGrl
Copy link
Contributor

Following up here, @nvoxland and @0x7d7b. I retested with the following changeset:

	<changeSet author="liquibase-user" id="2::useMODEInJDBCURL" labels="case2">
		<createTable tableName="PR2372">
			<column name="KEY" type="bigint" autoIncrement="true">
			</column>
		</createTable>
	</changeSet>
Results of Testing
MODE H2 Version AutoInc SQL Correct?
POSTGRESQL 1.4.200 "KEY" BIGINT AUTO_INCREMENT Yes
HSQLDB 1.4.200 "KEY" BIGINT AUTO_INCREMENT Yes
(none specified) 1.4.200 "KEY" BIGINT AUTO_INCREMENT Yes
POSTGRESQL 2.1.210 "KEY" BIGINT GENERATED BY DEFAULT AS IDENTITY Yes
HSQLDB 2.1.210 "KEY" BIGINT GENERATED BY DEFAULT AS IDENTITY) Yes
(none specified) 2.1.210 "KEY" BIGINT GENERATED BY DEFAULT AS IDENTITY) Yes

Using @0x7d7b's JDBC URL generated the correct SQL for me on h2 v2.1.210.

JDBC:
url:jdbc:h2:tcp://localhost:9090/mem:dev;IGNORECASE=TRUE;MODE=HSQLDB;DB_CLOSE_DELAY=-1;DB_CLOSE_ON_EXIT=TRUE;AUTO_RECONNECT=TRUE;INIT=CREATE SCHEMA IF NOT EXISTS mydb\\;SET SCHEMA mydb\\;

GENERATED SQL
-- Changeset pr2362-changelog.xml::2::useMODEInJDBCURL::liquibase-user
CREATE TABLE MYDB.PR2372 ("KEY" BIGINT GENERATED BY DEFAULT AS IDENTITY);

Test Environment
Liquibase Core: h2-autoincrement/1214/c29a51, Pro: master/405/dee743
H2 v1.4.200
H2 v2.1.210
Windows 10

Conditioning++ automation moved this from Ready for Handoff (In JIRA) to Done Feb 17, 2022
@nvoxland nvoxland added this to the v4.8.0 milestone Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

6 participants