Skip to content

Added MSSQLDatabase specific error evaluation when creating lock table #1817

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Nov 9, 2021
Merged

Added MSSQLDatabase specific error evaluation when creating lock table #1817

merged 3 commits into from
Nov 9, 2021

Conversation

stalbrecht
Copy link
Contributor

@stalbrecht stalbrecht commented Apr 30, 2021 •
edited by sync-by-unito bot
Loading

Proposed fix for #1816

Dev Handoff Notes (Internal Use)

Links

Testing

Dev Verification

Ran locally
(did not save output at the time)

Test Requirements (Internal Liquibase QA)

The bug is a race condition when multiple Liquibase updates are running at the same time.

  • The bug triggers when the DATABASECHANGELOGLOCK table is being created.
  • When two or more update operations attempt to create the DATABASECHANGELOGLOCK table at the same time, one or more update operations fails because the DATABASECHANGELOGLOCK table already exists.
  • This bug (and fix) are SQL Server specific.

The fix allows update operations to continue if the DATABASECHANGELOGLOCK table already exists.

The scripts attached to reproduce this bug are Windows-specific files; batch script (.cmd) and powershell (ps1). If you are not on Windows, please do not take this ticket; duplicating effort to rewrite scripts in bash just isn’t where we want to spend our test time. The powershell script runs 3 concurrent Liquibase operations. I have found 3 concurrent Liquibase operations is sufficient to get into the race condition. However, to increase the number of concurrent Liquibase operations, change the $list = 1..3 to $list = 1..n, where n is the new maximum concurrent operations. The powershell script calls a batch script that does the job of setting up and running Liquibase update.

When the powershell script runs, you will see three CLI prompts show up.

Logging is enabled in the batch script. The execution will output three log files: lb1555-log.txt, lb1555-log.txt1, lb1555-log.txt2. I’ll attach an example of the log produced when the race condition is encountered. You’ll notice that Liquibase exits/fails in older builds, and does not exit/fail when using the fix build.

To test this fix, you will need:

  • Windows OS
  • SQL Server 17
  • The attached changelog, properties file and scripts
    • You will need to update the paths and database connection properties for your environment.

Manual Tests

Prior to starting the test, ensure your SQL Server database does not have the DATABASECHANGELOGLOCK in the catalog you’re connecting to; do this by running liquibase drop-all.

Verify Liquibase update does not exit when DATABASECHANGELOGLOCK exists.

  • In Powershell ISE or Powershell CLI, run .\parallell-foreach.ps1
  • The update should continue when the DATABASECHANGELOGLOCK table already exists.

Automated Tests

  • None required for this ticket; it is a multi-threaded issue and not suitable for our functional test framework.

┆Issue is synchronized with this Jira Story by Unito

@molivasdat molivasdat linked an issue Apr 30, 2021 that may be closed by this pull request
liquibase.lockservice.StandardLockService does not detect that parallel running thread/application has created DATABASECHANGELOGLOCK table #1816
Closed
@molivasdat
Copy link
Contributor

Hi @stalbrecht We have a new build process. If the build succeeds, you should be able to download your fix. We will evaluate whether we need to change or update the code before merging it in.

Here's what happens next:
A member of the Liquibase team will take a look at your contribution and may suggest:

The PR will be prioritized according to our internal development and testing capacity.

We’ll let you know when it’s ready to move to the next step or if any changes are needed.

@nvoxland nvoxland changed the base branch from master to 4.3.x June 7, 2021 15:58
@nvoxland nvoxland changed the base branch from 4.3.x to master June 7, 2021 15:58
nvoxland
nvoxland approved these changes Jun 7, 2021
View reviewed changes
Copy link
Contributor

@nvoxland nvoxland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. I made a minor change to the new method name, and made it protected so subclasses can provide their own logic checks on the exception.

@nvoxland nvoxland requested a review from suryaaki2 June 7, 2021 15:59
@nvoxland nvoxland changed the base branch from master to 4.4.x July 6, 2021 18:07
@sync-by-unito sync-by-unito bot changed the title Added MSSQLDatabase specific error evaluation when creating lock tabl… Added MSSQLDatabase specific error evaluation when creating lock table Sep 21, 2021
@nvoxland nvoxland changed the base branch from 4.4.x to master October 26, 2021 17:36
@sync-by-unito
Copy link

sync-by-unito bot commented Nov 9, 2021

➤ Erzsebet Carmean commented:

Liquibase Internal QA Notes on Fix Scope
The original issue of updates being blocked by an existing DATABASECHANGELOGLOCK table is fixed, or at least did not occur over the course of ten executions of 3 to 5 concurrent Liquibase updates.

However, at 5 concurrent Liquibase updates, at least one will fail due to a violation of the PRIMARY KEY constraint, PK_DATABASECHANGELOGLOCK.

[2021-11-08 12:17:13] SEVERE [liquibase.integration] Violation of PRIMARY KEY constraint 'PK_DATABASECHANGELOGLOCK'.
Cannot insert duplicate key in object 'dbo.DATABASECHANGELOGLOCK'.
The duplicate key value is (1).
[Failed SQL: (2627) INSERT INTO DATABASECHANGELOGLOCK (ID, LOCKED) VALUES (1, 0)]Fixing the secondary duplicate key value error is out of scope for this fix. A more generic fix for concurrency issues is being worked in https://datical.atlassian.net/browse/LB-2131 ( https://datical.atlassian.net/browse/LB-2131|smart-link ) .

@suryaaki2 suryaaki2 merged commit 4261e7f into liquibase:master Nov 9, 2021
@sync-by-unito
Copy link

sync-by-unito bot commented Nov 17, 2021

➤ Erzsebet Carmean commented:

UPDATE :: Liquibase Internal QA Notes

The concurrency issues with DATABASECHANGELOGLOCK are addressed more broadly in #2198 ( https://github.com/liquibase/liquibase/pull/2198|smart-link ) . PR 2198 addresses the original issue as well as the PK constraint violation noted during testing of this PR.

@sync-by-unito sync-by-unito bot mentioned this pull request Nov 17, 2021
Closed
@nvoxland nvoxland added this to the v4.6.2 milestone Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
2 more reviewers

@nvoxland nvoxland nvoxland approved these changes

@suryaaki2 suryaaki2 suryaaki2 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v4.6.2
Development

Successfully merging this pull request may close these issues.

liquibase.lockservice.StandardLockService does not detect that parallel running thread/application has created DATABASECHANGELOGLOCK table
4 participants
@stalbrecht @molivasdat @nvoxland @suryaaki2