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.

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

Set up SEARCH_PATH only once when running an update command #5444

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Set up SEARCH_PATH only once when running an update command #5444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
39069a4
- Create a DatabaseUtils method to set search path permanently for Po…
MalloD12 Jan 5, 2024
9d14274
Merge branch 'master' into fix-issue-5316
MalloD12 Jan 5, 2024
436a631
searchPath not null check added.
MalloD12 Jan 5, 2024
5d294cc
Merge branch 'master' into fix-issue-5316
MalloD12 Jan 8, 2024
7b91bb3
Log level update for initializePostgresSearchPathPermanently.
MalloD12 Jan 8, 2024
3973f64
Integration test added to validate SEARCH_PATH is set only once.
MalloD12 Jan 9, 2024
25cdeec
- Add a method for getting finalSearchPath for postgres.
MalloD12 Jan 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions ...ration-tests/src/test/groovy/liquibase/command/UpdateSqlCommandStepIntegrationTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package liquibase.command

import liquibase.Scope
import liquibase.command.core.UpdateSqlCommandStep
import liquibase.command.core.helpers.DbUrlConnectionArgumentsCommandStep
import liquibase.database.core.DatabaseUtils
import liquibase.executor.Executor
import liquibase.executor.ExecutorService
import liquibase.extension.testing.testsystem.DatabaseTestSystem
import liquibase.extension.testing.testsystem.TestSystemFactory
import liquibase.extension.testing.testsystem.spock.LiquibaseIntegrationTest
import liquibase.statement.core.RawSqlStatement
import spock.lang.Shared
import spock.lang.Specification

@LiquibaseIntegrationTest
class UpdateSqlCommandStepIntegrationTest extends Specification{

@Shared
private DatabaseTestSystem postgres = Scope.currentScope.getSingleton(TestSystemFactory).getTestSystem("postgresql") as DatabaseTestSystem

def "validate UpdateSql only generates SQL statement to set SEARCH_PATH once"() {
when:
def postgresDB = postgres.getDatabaseFromFactory()
final Executor executor = Scope.getCurrentScope().getSingleton(ExecutorService.class).getExecutor("jdbc", postgresDB)
def catalogName = postgres.getConnection().getCatalog()
def schemaName = postgres.getConnection().getSchema()
def searchPath = DatabaseUtils.getFinalPostgresSearchPath(executor, catalogName, schemaName, postgresDB)

def outputStream = new ByteArrayOutputStream()
def updateSqlCommand = new CommandScope(UpdateSqlCommandStep.COMMAND_NAME)
updateSqlCommand.addArgumentValue(DbUrlConnectionArgumentsCommandStep.DATABASE_ARG, postgresDB)
updateSqlCommand.addArgumentValue(UpdateSqlCommandStep.CHANGELOG_FILE_ARG, "liquibase/update-tests.yml")
updateSqlCommand.setOutput(outputStream)
updateSqlCommand.execute()
def generatedSql = outputStream.toString()

then:
countSetSearchPathOccurrences(generatedSql, String.format("SET SEARCH_PATH TO %s, %s;", catalogName, searchPath)) == 0
countSetSearchPathOccurrences(generatedSql, String.format("ALTER DATABASE %s SET SEARCH_PATH TO %s;", catalogName, searchPath)) == 1
Copy link
Collaborator

@filipelautert filipelautert Jan 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@MalloD12 did you try to execute it with an user that is not database owner? Cause I tried to create an user, give him access to a database as below and then the alter failed:

--as postgres user:
create user tst with password 'tst';
create database tst;
grant connect, create on database tst to tst;

-- reconnect as tst, and then try it:
ALTER DATABASE tst SET SEARCH_PATH TO public;

[42501] ERROR: must be owner of database tst

I need to grant all privileges on database tst to tst so it works.

I can think of some options:

  1. try the alter table; if it fails fall back to the set search repetition ;
  2. do not rollback connections for postgresql (is it really required) then we can have only one set search_path
  3. Create (another) flag so users can decide what to do.

I'm suggesting this because requiring all privileges would be a breaking change and could create problems with companies devops pipelines.

CC @StevenMassaro

}

private int countSetSearchPathOccurrences(String updateSqlOutput, String searchPathSetup) {
int count = 0;
int index = updateSqlOutput.indexOf(searchPathSetup)

while (index != -1) {
count++;
index = updateSqlOutput.indexOf(searchPathSetup, index + 1)
}
return count
}
}
4 changes: 4 additions & 0 deletions liquibase-standard/src/main/java/liquibase/command/core/AbstractUpdateCommandStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import liquibase.command.CommandScope;
import liquibase.command.core.helpers.DatabaseChangelogCommandStep;
import liquibase.database.Database;
import liquibase.database.core.PostgresDatabase;
import liquibase.exception.DatabaseException;
import liquibase.exception.LiquibaseException;
import liquibase.exception.LockException;
Expand Down Expand Up @@ -67,6 +68,9 @@ public void run(CommandResultsBuilder resultsBuilder) throws Exception {
resultsBuilder.addResult("updateReport", updateReportParameters);
CommandScope commandScope = resultsBuilder.getCommandScope();
Database database = (Database) commandScope.getDependency(Database.class);
if(database instanceof PostgresDatabase) {
((PostgresDatabase) database).setSearchPathPermanently();
}
updateReportParameters.getDatabaseInfo().setDatabaseType(database.getDatabaseProductName());
updateReportParameters.getDatabaseInfo().setVersion(database.getDatabaseProductVersion());
updateReportParameters.setJdbcUrl(database.getConnection().getURL());
Expand Down
73 changes: 48 additions & 25 deletions liquibase-standard/src/main/java/liquibase/database/core/DatabaseUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package liquibase.database.core;

import liquibase.GlobalConfiguration;
import liquibase.Scope;
import liquibase.configuration.GlobalConfiguration;
import liquibase.database.Database;
import liquibase.database.OfflineConnection;
import liquibase.exception.DatabaseException;
Expand Down Expand Up @@ -37,11 +37,44 @@ public static void initializeDatabase(String defaultCatalogName, String defaultS
executor.execute(
new RawSqlStatement("ALTER SESSION SET CURRENT_SCHEMA=" +
database.escapeObjectName(schema, Schema.class)));
} else if (database instanceof PostgresDatabase && defaultSchemaName != null) {
String searchPath = executor.queryForObject(new RawSqlStatement("SHOW SEARCH_PATH"), String.class);
} else if (database instanceof PostgresDatabase) {
String finalSearchPath = getFinalPostgresSearchPath(executor, defaultCatalogName, defaultSchemaName, database);
executor.execute(new RawSqlStatement("SET SEARCH_PATH TO " + finalSearchPath));
} else if (database instanceof AbstractDb2Database) {
String schema = defaultCatalogName;
if (schema == null) {
schema = defaultSchemaName;
}
executor.execute(new RawSqlStatement("SET CURRENT SCHEMA "
+ schema));
} else if (database instanceof MySQLDatabase) {
String schema = defaultCatalogName;
if (schema == null) {
schema = defaultSchemaName;
}
executor.execute(new RawSqlStatement("USE " + schema));
} else if (database instanceof MSSQLDatabase) {
defaultCatalogName = StringUtil.trimToNull(defaultCatalogName);
if (defaultCatalogName != null) {
executor.execute(new RawSqlStatement(String.format("USE %s", defaultCatalogName)));
}
}
}
}

public static String getFinalPostgresSearchPath(Executor executor, String defaultCatalogName, String defaultSchemaName, Database database) {
String finalSearchPath = "";

if (database instanceof PostgresDatabase && defaultSchemaName != null) {
String searchPath = null;
try {
searchPath = executor.queryForObject(new RawSqlStatement("SHOW SEARCH_PATH"), String.class);
} catch (Throwable e) {
Scope.getCurrentScope().getLog(DatabaseUtils.class).warning("Cannot get search_path", e);
}

if (searchPath != null) {
if (!searchPath.equals(defaultCatalogName) && !searchPath.equals(defaultSchemaName) && !searchPath.equals("\"" + defaultSchemaName + "\"") && !searchPath.startsWith(defaultSchemaName + ",") && !searchPath.startsWith("\"" + defaultSchemaName + "\",")) {
String finalSearchPath;
if (GlobalConfiguration.PRESERVE_SCHEMA_CASE.getCurrentValue()) {
finalSearchPath = ((PostgresDatabase) database).quoteObject(defaultSchemaName, Schema.class);
} else {
Expand All @@ -57,30 +90,20 @@ public static void initializeDatabase(String defaultCatalogName, String defaultS
return ((PostgresDatabase) database).quoteObject(obj, Schema.class);
});
}

executor.execute(new RawSqlStatement("SET SEARCH_PATH TO " + finalSearchPath));
}

} else if (database instanceof AbstractDb2Database) {
String schema = defaultCatalogName;
if (schema == null) {
schema = defaultSchemaName;
}
executor.execute(new RawSqlStatement("SET CURRENT SCHEMA "
+ schema));
} else if (database instanceof MySQLDatabase) {
String schema = defaultCatalogName;
if (schema == null) {
schema = defaultSchemaName;
}
executor.execute(new RawSqlStatement("USE " + schema));
} else if (database instanceof MSSQLDatabase) {
defaultCatalogName = StringUtil.trimToNull(defaultCatalogName);
if (defaultCatalogName != null) {
executor.execute(new RawSqlStatement(String.format("USE %s", defaultCatalogName)));
}
}
}
return finalSearchPath;
}

public static void initializePostgresSearchPathPermanently(String defaultCatalogName, String defaultSchemaName, Database database) {
final Executor executor = Scope.getCurrentScope().getSingleton(ExecutorService.class).getExecutor("jdbc", database);
String finalSearchPath = getFinalPostgresSearchPath(executor, defaultCatalogName, defaultSchemaName, database);
try {
executor.execute(new RawSqlStatement(String.format("ALTER DATABASE %s SET SEARCH_PATH TO %s", database.getLiquibaseCatalogName(), finalSearchPath)));
} catch (Throwable e) {
Scope.getCurrentScope().getLog(DatabaseUtils.class).severe("Cannot set search_path at database level", e);
}
}
}

9 changes: 5 additions & 4 deletions liquibase-standard/src/main/java/liquibase/database/core/PostgresDatabase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package liquibase.database.core;

import liquibase.CatalogAndSchema;
import liquibase.GlobalConfiguration;
import liquibase.Scope;
import liquibase.changelog.column.LiquibaseColumn;
import liquibase.configuration.GlobalConfiguration;
import liquibase.database.AbstractJdbcDatabase;
import liquibase.database.DatabaseConnection;
import liquibase.database.ObjectQuotingStrategy;
Expand Down Expand Up @@ -399,9 +399,6 @@ public CatalogAndSchema.CatalogAndSchemaCase getSchemaAndCatalogCase() {
@Override
public void rollback() throws DatabaseException {
super.rollback();

//Rollback in postgresql resets the search path. Need to put it back to the defaults
DatabaseUtils.initializeDatabase(getDefaultCatalogName(), getDefaultSchemaName(), this);
}

@Override
Expand All @@ -416,4 +413,8 @@ public void setDefaultCatalogName(String defaultCatalogName) {
public boolean supportsCreateIfNotExists(Class<? extends DatabaseObject> type) {
return type.isAssignableFrom(Table.class);
}

public void setSearchPathPermanently() {
DatabaseUtils.initializePostgresSearchPathPermanently(defaultCatalogName, defaultSchemaName, this);
}
}
Loading