AbstractUpdateCommandStep.upToDateFastCheck global cache does not detect DB recreation

[MosheElisha]

Search first

• I searched and no similar issues were found

Description

Hi,

We have a relatively unique case where we have a custom multi tenant datasource and we run liquibase when the tenant logs in.
We create and configure SpringLiquibase and invoke afterPropertiesSet.

Our QA jobs are recreating the DB between runs and the issue is that Liquibase is not applied again.

From what I understand, this is because Liquibase uses an internal cache.
There is a singleton CommandFactory which holds an instance of all CommandSteps (including UpdateCommandStep)
https://github.com/liquibase/liquibase/blob/master/liquibase-standard/src/main/java/liquibase/command/CommandFactory.java#L244

The AbstractUpdateCommandStep holds an upToDateFastCheck map that remembers if Liquibase was already applied (on that JDBC URL with these changesets, etc.):
https://github.com/liquibase/liquibase/blob/master/liquibase-standard/src/main/java/liquibase/command/core/AbstractUpdateCommandStep.java#L89

This cache exists as long as JVM is up.

Liquibase detects that the dbchangelog tables do not exist and recreate them. I believe this is the place to also invalidate all the upToDateFastCheck cache entries that are relevant to that URL.

Steps To Reproduce

Attached a sample project.

1. Run failsOnSecondRun test once so it will create dbchangelog with checksums - it should pass.
2. Run failsOnSecondRun test again and it will fail.

upToDateFastCheck.zip

Expected/Desired Behavior

If Liquibase understands that databasechangelog tables are missing and creates them, it should also invalidate all the upToDateFastCheck cache entries that are relevant to that URL.

Liquibase Version

4.24.0 and 4.26.0

Database Vendor & Version

PostgreSQL

Liquibase Integration

Custom spring boot

Liquibase Extensions

No response

OS and/or Infrastructure Type/Provider

Ubuntu

Additional Context

No response

Are you willing to submit a PR?

• I'm willing to submit a PR (Thank you!)

[MosheElisha]

A workaround we implemented until a fix is available:

static {
    List<CommandStep> pipeline = CommandFactory.getInstance().getCommandDefinition(UpdateCommandStep.COMMAND_NAME).getPipeline();
    for (CommandStep commandStep : pipeline) {
        if (commandStep instanceof AbstractUpdateCommandStep abstractUpdateCommandStep) {
            abstractUpdateCommandStep.setFastCheckEnabled(false);
        }
    }
}

[MalloD12]

Hi @MosheElisha,

Thank you for raising this issue for us. This is very appreciated by the entire team. Please do us the favor of trying with the latest version.

@tati-qalified: if you have some availability, could you please make sure whether this issue is still happening or not with latest version?

Thanks,
Daniel.

[MosheElisha]

Hi @MalloD12 ,

Thanks. Tested and this is still reproducing on 4.27.0

[dsteegen]

We are facing the same issue with Liquibase 4.26.0. In our usecase, we recreate the database schema between end-to-end tests. Since the server is not restarted, the in memory HashMap still has the reference of the executed changelogs. It would be nice if we could disable the upToDateFastCheck entirely with a global parameter or at least have a public api that allows us to reset the Map.
In the meantime, I will check if the solution/workaround provided by @MosheElisha suffices for our case.

[dsteegen]

I can confirm that the provided work around fixes the problem!

[MalloD12]

Hey guys,

I added the below code changes and test you provided here is always passing for me now. The way I followed was by adding a new UpdateCommandStep argument to control whether we want to disable fast-check or not. So I added to the UpdateCommandStep class the below argument:

FAST_CHECK_DISABLED = builder.argument("fastCheckEnabled", Boolean.class)
                .defaultValue(Boolean.FALSE)
                .description("")
                .hidden()
                .build();

and then updated the run() method as below:

public void run(CommandResultsBuilder resultsBuilder) throws Exception {
        setDBLock(false);
        if(resultsBuilder.getCommandScope().getArgumentValue(FAST_CHECK_DISABLED)){
            setFastCheckEnabled(false);
        }
        super.run(resultsBuilder);
    }

I can push these code changes so that you can pull them and test them. Then if you are ok with them, we can add some tests and move forward with our review process to get this code merged soon.

Thanks,
Daniel.

[MosheElisha]

Thanks, @MalloD12 !

Your proposed solutions sounds good to me and having this flag is probably good anyway.

For this use case, I believe a better approach is to clear the cache automatically when Liquibase understands that databasechangelog tables are missing and creates them.
Such a solution is better IMO because

1. users won't need to learn about this flag after they hit an issue and debug for a few hours
2. users that have this use case will benefit from the cache and it will be cleared only in the rare case that the tables were deleted.

[MalloD12]

Hi @MosheElisha,

Would you like to pull the changes added in #5917 and apply your proposal there?

Thanks,
Daniel.

[MosheElisha]

Thanks, @MalloD12 . Feel free to continue with your PR.
I will try to implement the approach I suggested in parallel.

[MalloD12]

We are fine waiting for your proposed solution. @MosheElisha If you think your proposed solution would fix more efficiently let's just ignore #5917 for now. It was trying to provide some guidance to fix this issue.

Thanks,
Daniel.

[MosheElisha]

Thanks, @MalloD12 .

I created a fork and committed a change - MosheElisha@2562046

I have verified it fixes the issue. Please see if it looks OK.
I was thinking to add a test to UpdateCommandStepIntegrationTest but wasn't able to understand how to run these tests.

I would would appreciate some guidance regarding the tests or if you prefer, you can implement the fix yourself.

[MalloD12]

Hi @MosheElisha,

Your code changes look good to me. @filipelautert one other member of the team has also made a fast-check refactoring which moved it to a new singleton service. This is the PR I'm talking about. Could you please have a look at it, and if you are ok with it I can the test you provided us here, or I can guide you so you can add it yourself. I'm fine with either option.

Thanks,
Daniel.

[MosheElisha]

Thanks, @MalloD12 .

The fast-check refactoring PR looks good.
If you can, please go ahead and add the test.

Thanks a lot!

[MalloD12]

Sure, I'll be working on it.

Thanks,
Daniel.

[MalloD12]

Hi @MosheElisha,

I tried to add the test you provided us as an integration test, but I couldn't make it work (it was always passing, I tested it using the code from your PR (same as using fast-check refactoring PR ), but it was the same as using master branch. I need to check with QA team whether that's the right approach for testing this cache feature or not. Also, when working on this I realised that we have this logic:

if (!upToDateFastCheck.containsKey(cacheKey)) {
            ChangeLogHistoryService changeLogService = Scope.getCurrentScope().getSingleton(ChangeLogHistoryServiceFactory.class).getChangeLogService(database);
            try {
                if (changeLogService.isDatabaseChecksumsCompatible() && listUnrunChangeSets(changesetFilters, database, databaseChangeLog, contexts, labelExpression).isEmpty()) {
                    Scope.getCurrentScope().getLog(getClass()).fine("Fast check found no un-run changesets");
                    upToDateFastCheck.put(cacheKey, true);
                } else {
                    upToDateFastCheck.put(cacheKey, false);
                }
            } catch (DatabaseException e) {
                Scope.getCurrentScope().getLog(getClass()).info("Error querying Liquibase tables, disabling fast check for this execution. Reason: " + e.getMessage());
                upToDateFastCheck.put(cacheKey, false);
            } finally {
                // Discard the cached fetched un-run changeset list, as if another peer is running the changesets
                // in parallel, we may get a different answer after taking out the write lock
                changeLogService.reset();
            }
        }
        return upToDateFastCheck.get(cacheKey);

But If I'm wrong we are only inserting into the map:

upToDateFastCheck.put(cacheKey, false);

and I think after an update command is executed successfully we should call the fastCheck service to set the value of the given cache entry as true. Similarly, when running a rollback/dropAll command we should set this flag as false. I'm already discussing with the team whether we need additional refactoring, but in the meantime, once we resolve how to test the implemented changes I think we will be good to move forward with fast-check refactoring PR .

If you have any questions or any additional feedback please share it.

Thanks,
Daniel.

[MosheElisha]

Thanks for the update, @MalloD12 .
I agree it is anyway good to proceed with fast-check refactoring PR