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

Added MSSQLDatabase specific error evaluation when creating lock table #1817

Merged
merged 3 commits into from Nov 9, 2021

Conversation

stalbrecht
Copy link
Contributor

@stalbrecht stalbrecht commented Apr 30, 2021

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
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.

@molivasdat molivasdat added DBMSSQLServer ImpactLow IntegrationCDI RiskHigh Items that require more extensive testing, change an existing API, or add new features. Severity3 TypeBug labels May 7, 2021
@molivasdat molivasdat added this to To Do in Conditioning++ Jun 4, 2021
@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
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 moved this from To Do to Code Review in Conditioning++ Jun 7, 2021
@suryaaki2 suryaaki2 moved this from Code Review to Ready for Handoff (In JIRA) in Conditioning++ Jun 17, 2021
@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
Conditioning++ automation moved this from Ready for Handoff (In JIRA) to Done 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DBMSSQLServer ImpactLow IntegrationCDI RiskHigh Items that require more extensive testing, change an existing API, or add new features. Severity3 SyncTicket TypeBug
Projects
None yet
4 participants