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

CORE-2321 Liquibase tag command tags too much, CORE-842 #392

Closed
wants to merge 5 commits into from
Closed

CORE-2321 Liquibase tag command tags too much, CORE-842 #392

wants to merge 5 commits into from

Conversation

mches
Copy link
Contributor

@mches mches commented Mar 30, 2015

CORE-2321 Liquibase tag command tags too much

I've tested this with Microsoft SQL Server 2008 R2 using DATEEXECUTED with data type date (no hours, minutes, seconds, or fractional seconds).

@mches
Copy link
Contributor Author

mches commented Mar 31, 2015

This is working fine for me as implemented as far as rolling back to a tag is concerned. However, I noticed differences in the number of records updated, depending on the accuracy of the data type chosen for DATEEXECUTED. Now I'm thinking, is there any reason not to use MAX(ORDEREXECUTED) in preference to MAX(DATEEXECUTED). That would seem to guarantee exactly 1 record is updated rather than a varying number.

@mches
Copy link
Contributor Author

mches commented Mar 31, 2015

On a further review, it seems possible that the INSERT statement generated by MarkChangeSetRanGenerator obsoletes the UPDATE statement generated by TagDatabaseGenerator. Thoughts?

@mches
Copy link
Contributor Author

mches commented Apr 1, 2015

I found that the MarkChangeSetRanGenerator could be improved to always tag successfully ran changes, eliminating the need for TagDatabaseChange to generate any SQL statements. I also found that the TagDatabaseGenerator was used outside the scope of change log execution, and under that scenario it was necessary to run the complex update statement to identify and update the last change set executed. Therefore I thought it would be prudent to implement both ideas. The pull request has been updated accordingly. Looking forward to feedback.

responsible for tagging when executing a change log, while still allowing
the database to be tagged outside of that scenario using the necessary
update statement with subqueries and aggregate expressions. This can improve
performance of executing change logs by eliminating the need to execute unnecessary update statements with subqueries
and aggregate expressions. Improve the update statement to use the
more-unique ORDEREXECUTED column rather than potentially less unique
DATEEXECUTED to ensure that only the minimum number of records get updated.
@mches
Copy link
Contributor Author

mches commented Apr 3, 2015

I believe this also resolves the following issue
CORE-842 Tag database not taking orderexecuted into account

@mches mches changed the title CORE-2321 Liquibase tag command tags too much CORE-2321 Liquibase tag command tags too much, CORE-842 Apr 3, 2015
@nvoxland
Copy link
Contributor

nvoxland commented Apr 7, 2015

I merged the pull request into master since it's more than I'd like to do in a patch release in 3.3.x.

Your pull request had a lot of good improvements, but the logic in TagDatabaseGenerator wasn't quite correct because you cannot rely on orderexecuted to always be increasing. There are times (especially when running updateSql) where we cannot know where to start orderexectued so it just restarts at zero. Orderexecuted is guaranteed to be increasing within a single update run, but isn't necessarily increasing across update runs.

I merged your request into master along with an additional fix to change the TagDatabaseGenerator logic for oracle, mysql, mssql, postgres, informix and db2 to take orderexecuted and dateexecuted into account. As a fallback it just uses dateexectuted until other SQL for other databases can be created and tested.

@nvoxland nvoxland closed this Apr 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants