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

H2Dialect shouldn't use MySQL-style syntax and other potential problems #2306

Closed
katzyn opened this issue Dec 30, 2021 · 6 comments
Closed

Comments

@katzyn
Copy link

katzyn commented Dec 30, 2021

  1. All versions of H2 support GENERATED BY DEFAULT AS IDENTITY, GENERATED BY DEFAULT AS IDENTITY (START WITH 100), etc.

GENERATED ALWAYS AS IDENTITY is accepted since H2 1.1.100 (2008-10-04), but its behavior was the same as BY DEFAULT until H2 1.4.200, it is different only since H2 2.0.202.

There is a problem with H2Database.getAutoIncrementClause() method, it returns MySQL compatibility syntax instead of standard one:

This legacy syntax is rejected since H2 2.0.202 in compatibility modes where it shouldn't be used. In H2 2.0.204 it is available only in REGULAR, LEGACY, MARIADB, and MYSQL compatibility modes and is not available in STRICT and all other modes.

Some people use these modes and Liquibase is unable to create identity columns, a syntax error is thrown:
https://stackoverflow.com/questions/70453996/h2-version-2-0-202-auto-increment-not-working

  1. Next value of a sequence should be fetched with standard NEXT VALUE FOR sequenceName expression, it is supported by all versions of H2:
    https://h2database.com/html/grammar.html#sequence_value_expression
    Liquibase seems to use buggy and deprecated NEXTVAL() function:
    https://h2database.com/html/functions.html#nextval

    super.sequenceNextValueFunction = "NEXTVAL('%s')";

    This legacy function may not work in some compatibility modes or work in unexpected way, there are some deviations between compatibility modes.

  2. Current value of a sequence should be fetched with non-standard CURRENT VALUE FOR sequnceName in H2 1.4.200 and later versions:
    https://h2database.com/html/grammar.html#sequence_value_expression
    Legacy CURRVAL() function should only be used in H2 1.4.199 and older versions, this function is also deprecated in H2 2.0:
    https://h2database.com/html/functions.html#currval

    super.sequenceCurrentValueFunction = "CURRVAL('%s')";

    This legacy function also may not work in some compatibility modes.

There are some other suspicious places.

  1. getViewDefinition() tries to find SELECT in it, but SELECT … in not the only one kind of queries, there are others, such as table value constructor (VALUES …) and explicit table (TABLE tableName). View definition may also start with WITH

  2. getConcatSql() uses CONCAT function, why this is needed? Default implementation in superclass produces standard concatenation operator || supported by all versions of H2.

  3. List of current datetime value functions contains various deprecated functions, but doesn't contain standard functions for TIMESTAMP [ WITHOUT TIME ZONE] and TIME [WITHOUT TIME ZONE] values:
    https://h2database.com/html/functions.html#localtimestamp
    https://h2database.com/html/functions.html#localtime

@kataggart
Copy link
Contributor

Thanks @katzyn for all the detail you have provided here. We have been investigating some issues related to the changes from H2 1.x to H2 2.x, so your contribution here is particularly useful.

By the way, if you haven't yet checked out https://forum.liquibase.org/, I recommend - it's becoming a pretty solid resource and I am sure others in the community would benefit from your expertise over there.

@jdlee726
Copy link

I think the followings also should be fixed.

@Override
protected String getAutoIncrementStartWithClause() {
return "%d";
}

@Override
    protected String getAutoIncrementStartWithClause() {
//	return "%d"; <-- error
	return "START WITH %d";
    }

@DanielFran
Copy link

@nvoxland @kataggart Can you confirm if #3026 have fixed all issues reported here or partially?

@kataggart
Copy link
Contributor

@DanielFran will do our best to take a look. Thanks.

@nvoxland
Copy link
Contributor

nvoxland commented Aug 1, 2022

Yes, I think that #3026 should address all the issues listed. I'm going to close this as fixed by that PR.

The ones not addressed are:

  1. The timestamp with/without timezone support, but that support is problematic across all databases and will be addressed separately
  2. The concat logic is still using the concat function, but as far as I can tell that is also supported by all versions? It handles nulls a bit better so I don't see a reason to change that.

If anyone does find anything still not working, open it as a new issue.

@nvoxland nvoxland closed this as completed Aug 1, 2022
Conditioning++ automation moved this from To Do to Done Aug 1, 2022
@nvoxland
Copy link
Contributor

nvoxland commented Aug 1, 2022

Fixed by #3026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

6 participants