Skip to content

Renaming performance indexes on upgrade #342

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
2fcc701
First changes for PRF rename
BrunoSMonteiro74 May 2, 2025
7dbf733
Add coverage for new code to GraphBasedUpgradeSchemaChangeVisitor, In…
BrunoSMonteiro74 May 2, 2025
664a70d
Add more coverage for new code to GraphBasedUpgradeSchemaChangeVisito…
BrunoSMonteiro74 May 2, 2025
ac3c65f
Solve reliability issue due to Sonar misunderstanding code.
BrunoSMonteiro74 May 2, 2025
74aa746
Remove commented code
BrunoSMonteiro74 May 5, 2025
c1ffd1a
AdditionalMetadata - remove ignoredIndexes()
BrunoSMonteiro74 May 5, 2025
94fe49d
Changes that apply to more than one file:
BrunoSMonteiro74 May 5, 2025
0c4f2b0
Solve Sonar issues.
BrunoSMonteiro74 May 5, 2025
1b425cb
Add try with resources for SchemaResource to GraphBasedUpgradeBuilder…
BrunoSMonteiro74 May 9, 2025
fdfd5a4
Fix Upgrade try with resources for SchemaResource. openSchemaResource…
BrunoSMonteiro74 May 9, 2025
d1f434c
Fix dead store.
BrunoSMonteiro74 May 9, 2025
285d40f
Remove ignoredIndexes() from Table and dependents.
BrunoSMonteiro74 May 16, 2025
52bf1e5
Add more coverage to PostgreSQLMetaDataProvider and DatabaseMetaDataP…
BrunoSMonteiro74 May 16, 2025
29f9fc8
Add more coverage to Upgrade and AdditionalMetadata.
BrunoSMonteiro74 May 16, 2025
45a1d96
Add comment and add two constants on TestPostgreSqlMetaDataProvider
BrunoSMonteiro74 May 17, 2025
b54403c
Fix test and add more assertions on TestPostgreSqlMetaDataProvider
BrunoSMonteiro74 May 17, 2025
9677554
Add slightly more cover.
BrunoSMonteiro74 May 19, 2025
2f1d774
Inline extracted method.
BrunoSMonteiro74 May 23, 2025
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
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ public abstract class DatabaseMetaDataProvider implements Schema {

private final Supplier<Map<String, String>> databaseInformation = Suppliers.memoize(this::loadDatabaseInformation);


/**
* @param connection The database connection from which meta data should be provided.
* @param schemaName The name of the schema in which the data is stored. This might be null.
Expand Down Expand Up @@ -677,7 +676,7 @@ protected Table loadTable(AName tableName) {

final Map<AName, Integer> primaryKey = loadTablePrimaryKey(realTableName);
final Supplier<List<Column>> columns = Suppliers.memoize(() -> loadTableColumns(realTableName, primaryKey));
final Supplier<List<Index>> indexes = Suppliers.memoize(() -> loadTableIndexes(realTableName));
final Supplier<List<Index>> indexes = Suppliers.memoize(() -> loadTableIndexes(realTableName, false));

return new Table() {
@Override
Expand Down Expand Up @@ -789,10 +788,12 @@ protected static List<Column> createColumnsFrom(Collection<ColumnBuilder> origin
* Loads the indexes for the given table name, except for the primary key index.
*
* @param tableName Name of the table.
* @param returnIgnored if true return ignored indexes, when false return all the non ignored.
* @return List of table indexes.
*/
protected List<Index> loadTableIndexes(RealName tableName) {
protected List<Index> loadTableIndexes(RealName tableName, boolean returnIgnored) {
final Map<RealName, ImmutableList.Builder<RealName>> indexColumns = new HashMap<>();
final Map<RealName, ImmutableList.Builder<RealName>> ignoredIndexColumns = new HashMap<>();
final Map<RealName, Boolean> indexUniqueness = new HashMap<>();

if (log.isTraceEnabled()) log.trace("Reading table indexes for " + tableName);
Expand All @@ -811,10 +812,6 @@ protected List<Index> loadTableIndexes(RealName tableName) {
if (isPrimaryKeyIndex(indexName)) {
continue;
}
if (DatabaseMetaDataProviderUtils.shouldIgnoreIndex(indexName.getDbName())) {
log.info("Ignoring index: ["+indexName.getDbName()+"]");
continue;
}

String dbColumnName = indexResultSet.getString(INDEX_COLUMN_NAME);
String realColumnName = allColumns.get().get(tableName).get(named(dbColumnName)).getName();
Expand All @@ -827,8 +824,13 @@ protected List<Index> loadTableIndexes(RealName tableName) {

indexUniqueness.put(indexName, unique);

indexColumns.computeIfAbsent(indexName, k -> ImmutableList.builder())
if (DatabaseMetaDataProviderUtils.shouldIgnoreIndex(indexName.getDbName())) {
ignoredIndexColumns.computeIfAbsent(indexName, k -> ImmutableList.builder())
.add(columnName);
} else {
indexColumns.computeIfAbsent(indexName, k -> ImmutableList.builder())
.add(columnName);
}
}
catch (SQLException e) {
throw new RuntimeSqlException("Error reading metadata for index ["+indexName+"] on table ["+tableName+"]", e);
Expand All @@ -838,9 +840,15 @@ protected List<Index> loadTableIndexes(RealName tableName) {
long end = System.currentTimeMillis();
if (log.isTraceEnabled()) log.trace(String.format("Read table indexes for %s in %dms; %d indexes; %d unique", tableName, end-start, indexColumns.size(), indexUniqueness.size()));

return indexColumns.entrySet().stream()
if (returnIgnored) {
return ignoredIndexColumns.entrySet().stream()
.map(e -> createIndexFrom(e.getKey(), indexUniqueness.get(e.getKey()), e.getValue().build()))
.collect(Collectors.toList());
} else {
return indexColumns.entrySet().stream()
.map(e -> createIndexFrom(e.getKey(), indexUniqueness.get(e.getKey()), e.getValue().build()))
.collect(Collectors.toList());
}
}
}
catch (SQLException e) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.alfasoftware.morf.metadata;

import java.util.List;
import java.util.Map;

import org.apache.commons.lang3.NotImplementedException;
Expand All @@ -16,4 +17,9 @@ public interface AdditionalMetadata extends Schema {
default Map<String, String> primaryKeyIndexNames() {
throw new NotImplementedException("Not implemented yet.");
}

default Map<String, List<Index>> ignoredIndexes() {
return Map.of();
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,8 @@ private TableBuilderImpl(String name) {
}


private TableBuilderImpl(String name, Iterable<? extends Column> columns, Iterable<? extends Index> indexes, boolean isTemporary) {
private TableBuilderImpl(String name, Iterable<? extends Column> columns, Iterable<? extends Index> indexes,
boolean isTemporary) {
super(name, columns, indexes, isTemporary);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import java.util.ArrayList;
import java.util.List;


import com.google.common.collect.Iterables;


Expand Down Expand Up @@ -79,7 +78,8 @@ private TableBean(String tableName, boolean isTemporary) {
* @param indexes indexes for the table;
* @param isTemporary Whether the table is a temporary table.
*/
TableBean(String tableName, Iterable<? extends Column> columns, Iterable<? extends Index> indexes, boolean isTemporary) {
TableBean(String tableName, Iterable<? extends Column> columns, Iterable<? extends Index> indexes,
boolean isTemporary) {
this(tableName, isTemporary);

Iterables.addAll(this.columns, columns);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package org.alfasoftware.morf.upgrade;

import java.util.Collection;
import java.util.List;

import org.alfasoftware.morf.jdbc.SqlDialect;
import org.alfasoftware.morf.metadata.Index;
import org.alfasoftware.morf.metadata.Schema;

/**
* Common code between SchemaChangeVisitor implementors
*/
public abstract class AbstractSchemaChangeVisitor implements SchemaChangeVisitor {

protected Schema sourceSchema;
protected SqlDialect sqlDialect;
protected final UpgradeConfigAndContext upgradeConfigAndContext;

protected abstract void writeStatements(Collection<String> statements);


public AbstractSchemaChangeVisitor(Schema sourceSchema, UpgradeConfigAndContext upgradeConfigAndContext, SqlDialect sqlDialect) {
this.sourceSchema = sourceSchema;
this.upgradeConfigAndContext = upgradeConfigAndContext;
this.sqlDialect = sqlDialect;
}


@Override
public void visit(AddIndex addIndex) {
sourceSchema = addIndex.apply(sourceSchema);
Index foundIndex = null;
List<Index> ignoredIndexes = upgradeConfigAndContext.getIgnoredIndexesForTable(addIndex.getTableName());
if (!ignoredIndexes.isEmpty()) {
for (Index index : ignoredIndexes) {
if (index.columnNames().equals(addIndex.getNewIndex().columnNames())) {
foundIndex = index;
break;
}
}
}

if (foundIndex != null) {
writeStatements(sqlDialect.renameIndexStatements(sourceSchema.getTable(addIndex.getTableName()), foundIndex.getName(), addIndex.getNewIndex().getName()));
} else {
writeStatements(sqlDialect.addIndexStatements(sourceSchema.getTable(addIndex.getTableName()), addIndex.getNewIndex()));
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ public class GraphBasedUpgradeBuilder {
private final Schema targetSchema;

private final ConnectionResources connectionResources;
private final UpgradeConfigAndContext upgradeConfigAndContext;
private final Set<String> exclusiveExecutionSteps;
private final SchemaChangeSequence schemaChangeSequence;
private final ViewChanges viewChanges;
Expand All @@ -57,8 +58,7 @@ public class GraphBasedUpgradeBuilder {
* @param sourceSchema source schema
* @param targetSchema target schema
* @param connectionResources connection resources to be used
* @param exclusiveExecutionSteps names of the upgrade step classes which should
* be executed in an exclusive way
* @param upgradeConfigAndContext the upgrade config
* @param schemaChangeSequence to be used to build a
* {@link GraphBasedUpgrade}
* @param viewChanges view changes which need to be made to match
Expand All @@ -71,7 +71,7 @@ public class GraphBasedUpgradeBuilder {
Schema sourceSchema,
Schema targetSchema,
ConnectionResources connectionResources,
Set<String> exclusiveExecutionSteps,
UpgradeConfigAndContext upgradeConfigAndContext,
SchemaChangeSequence schemaChangeSequence,
ViewChanges viewChanges) {
this.visitorFactory = visitorFactory;
Expand All @@ -80,7 +80,8 @@ public class GraphBasedUpgradeBuilder {
this.sourceSchema = sourceSchema;
this.targetSchema = targetSchema;
this.connectionResources = connectionResources;
this.exclusiveExecutionSteps = exclusiveExecutionSteps;
this.upgradeConfigAndContext = upgradeConfigAndContext;
this.exclusiveExecutionSteps = upgradeConfigAndContext.getExclusiveExecutionSteps();
this.schemaChangeSequence = schemaChangeSequence;
this.viewChanges = viewChanges;
}
Expand All @@ -97,19 +98,22 @@ public GraphBasedUpgrade prepareGraphBasedUpgrade(List<String> initialisationSql

GraphBasedUpgradeNode root = prepareGraph(nodes);

List<String> preUpgStatements;
List<String> postUpgStatements;
Table idTable = SqlDialect.IdTable.withPrefix(connectionResources.sqlDialect(), "temp_id_", false);

GraphBasedUpgradeSchemaChangeVisitor visitor = visitorFactory.create(
sourceSchema,
upgradeConfigAndContext,
connectionResources.sqlDialect(),
idTable,
nodes.stream().collect(Collectors.toMap(GraphBasedUpgradeNode::getName, Function.identity())));

GraphBasedUpgradeScriptGenerator scriptGenerator = scriptGeneratorFactory.create(sourceSchema, targetSchema, connectionResources, idTable, viewChanges, initialisationSql);

List<String> preUpgStatements = scriptGenerator.generatePreUpgradeStatements();
preUpgStatements = scriptGenerator.generatePreUpgradeStatements();
schemaChangeSequence.applyTo(visitor);
List<String> postUpgStatements = scriptGenerator.generatePostUpgradeStatements();
postUpgStatements = scriptGenerator.generatePostUpgradeStatements();

if (LOG.isDebugEnabled()) {
logGraph(root);
Expand Down Expand Up @@ -436,8 +440,7 @@ public GraphBasedUpgradeBuilderFactory(
* @param sourceSchema source schema
* @param targetSchema target schema
* @param connectionResources connection resources to be used
* @param exclusiveExecutionSteps names of the upgrade step classes which should
* be executed in an exclusive way
* @param upgradeConfigAndContext upgrade config and context
* @param schemaChangeSequence to be used to build a
* {@link GraphBasedUpgrade}
* @param viewChanges view changes which need to be made to match
Expand All @@ -448,7 +451,7 @@ GraphBasedUpgradeBuilder create(
Schema sourceSchema,
Schema targetSchema,
ConnectionResources connectionResources,
Set<String> exclusiveExecutionSteps,
UpgradeConfigAndContext upgradeConfigAndContext,
SchemaChangeSequence schemaChangeSequence,
ViewChanges viewChanges) {
return new GraphBasedUpgradeBuilder(
Expand All @@ -458,7 +461,7 @@ GraphBasedUpgradeBuilder create(
sourceSchema,
targetSchema,
connectionResources,
exclusiveExecutionSteps,
upgradeConfigAndContext,
schemaChangeSequence,
viewChanges);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,8 @@
*
* @author Copyright (c) Alfa Financial Software Limited. 2022
*/
class GraphBasedUpgradeSchemaChangeVisitor implements SchemaChangeVisitor {
class GraphBasedUpgradeSchemaChangeVisitor extends AbstractSchemaChangeVisitor implements SchemaChangeVisitor {

private Schema sourceSchema;
private final SqlDialect sqlDialect;
private final Table idTable;
private final TableNameResolver tracker;
private final Map<String, GraphBasedUpgradeNode> upgradeNodes;
Expand All @@ -30,12 +28,14 @@ class GraphBasedUpgradeSchemaChangeVisitor implements SchemaChangeVisitor {
* Default constructor.
*
* @param sourceSchema schema prior to upgrade step.
* @param upgradeConfigAndContext upgrade config
* @param sqlDialect dialect to generate statements for the target database.
* @param idTable table for id generation.
* @param upgradeNodes all the {@link GraphBasedUpgradeNode} instances in the
* upgrade for which the visitor will generate statements
*/
GraphBasedUpgradeSchemaChangeVisitor(Schema sourceSchema, SqlDialect sqlDialect, Table idTable, Map<String, GraphBasedUpgradeNode> upgradeNodes) {
GraphBasedUpgradeSchemaChangeVisitor(Schema sourceSchema, UpgradeConfigAndContext upgradeConfigAndContext, SqlDialect sqlDialect, Table idTable, Map<String, GraphBasedUpgradeNode> upgradeNodes) {
super(sourceSchema, upgradeConfigAndContext, sqlDialect);
this.sourceSchema = sourceSchema;
this.sqlDialect = sqlDialect;
this.idTable = idTable;
Expand All @@ -47,7 +47,8 @@ class GraphBasedUpgradeSchemaChangeVisitor implements SchemaChangeVisitor {
/**
* Write statements to the current node
*/
private void writeStatements(Collection<String> statements) {
@Override
protected void writeStatements(Collection<String> statements) {
currentNode.addAllUpgradeStatements(statements);
}

Expand All @@ -74,13 +75,6 @@ public void visit(RemoveTable removeTable) {
}


@Override
public void visit(AddIndex addIndex) {
sourceSchema = addIndex.apply(sourceSchema);
writeStatements(sqlDialect.addIndexStatements(sourceSchema.getTable(addIndex.getTableName()), addIndex.getNewIndex()));
}


@Override
public void visit(AddColumn addColumn) {
sourceSchema = addColumn.apply(sourceSchema);
Expand Down Expand Up @@ -242,15 +236,16 @@ static class GraphBasedUpgradeSchemaChangeVisitorFactory {
* Creates {@link GraphBasedUpgradeSchemaChangeVisitor} instance.
*
* @param sourceSchema schema prior to upgrade step
* @param upgradeConfigAndContext upgrade config
* @param sqlDialect dialect to generate statements for the target database
* @param idTable table for id generation
* @param upgradeNodes all the {@link GraphBasedUpgradeNode} instances in the upgrade for
* which the visitor will generate statements
* @return new {@link GraphBasedUpgradeSchemaChangeVisitor} instance
*/
GraphBasedUpgradeSchemaChangeVisitor create(Schema sourceSchema, SqlDialect sqlDialect, Table idTable,
Map<String, GraphBasedUpgradeNode> upgradeNodes) {
return new GraphBasedUpgradeSchemaChangeVisitor(sourceSchema, sqlDialect, idTable, upgradeNodes);
GraphBasedUpgradeSchemaChangeVisitor create(Schema sourceSchema, UpgradeConfigAndContext upgradeConfigAndContext, SqlDialect sqlDialect, Table idTable,
Map<String, GraphBasedUpgradeNode> upgradeNodes) {
return new GraphBasedUpgradeSchemaChangeVisitor(sourceSchema, upgradeConfigAndContext, sqlDialect, idTable, upgradeNodes);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,9 @@
*
* @author Copyright (c) Alfa Financial Software 2010
*/
public class InlineTableUpgrader implements SchemaChangeVisitor {
public class InlineTableUpgrader extends AbstractSchemaChangeVisitor implements SchemaChangeVisitor {

private Schema currentSchema;
private final SqlDialect sqlDialect;
private final SqlStatementWriter sqlStatementWriter;
private final Table idTable;
private final TableNameResolver tracker;
Expand All @@ -42,11 +41,13 @@ public class InlineTableUpgrader implements SchemaChangeVisitor {
* Default constructor.
*
* @param startSchema schema prior to upgrade step.
* @param upgradeConfigAndContext upgrade config
* @param sqlDialect Dialect to generate statements for the target database.
* @param sqlStatementWriter recipient for all upgrade SQL statements.
* @param idTable table for id generation.
*/
public InlineTableUpgrader(Schema startSchema, SqlDialect sqlDialect, SqlStatementWriter sqlStatementWriter, Table idTable) {
public InlineTableUpgrader(Schema startSchema, UpgradeConfigAndContext upgradeConfigAndContext, SqlDialect sqlDialect, SqlStatementWriter sqlStatementWriter, Table idTable) {
super(startSchema, upgradeConfigAndContext, sqlDialect);
this.currentSchema = startSchema;
this.sqlDialect = sqlDialect;
this.sqlStatementWriter = sqlStatementWriter;
Expand Down Expand Up @@ -93,16 +94,6 @@ public void visit(RemoveTable removeTable) {
}


/**
* @see org.alfasoftware.morf.upgrade.SchemaChangeVisitor#visit(org.alfasoftware.morf.upgrade.AddIndex)
*/
@Override
public void visit(AddIndex addIndex) {
currentSchema = addIndex.apply(currentSchema);
writeStatements(sqlDialect.addIndexStatements(currentSchema.getTable(addIndex.getTableName()), addIndex.getNewIndex()));
}


/**
* @see org.alfasoftware.morf.upgrade.SchemaChangeVisitor#visit(org.alfasoftware.morf.upgrade.AddColumn)
*/
Expand Down Expand Up @@ -226,7 +217,8 @@ private void visitStatement(Statement statement) {
/**
* Write out SQL
*/
private void writeStatements(Collection<String> statements) {
@Override
protected void writeStatements(Collection<String> statements) {
sqlStatementWriter.writeSql(statements);
}

Expand Down
Loading