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

Use single connection for both schema history table management and applying migrations #2

Closed
wants to merge 1 commit into from
Closed

Use single connection for both schema history table management and applying migrations #2

wants to merge 1 commit into from

Conversation

ashetkar
Copy link
Collaborator

@ashetkar ashetkar commented Aug 9, 2024 •
edited
Loading

What

  • Fix for GH Issue 23327 (Flyway migrations with ALTER TABLE ... RENAME COLUMN may fail with YBDB 2.20+)
    • Currently, the plugin uses a separate connection to manage schema history table and migrations are applied in another thread. This was causing catalog version mismatch issues when one thread altered catalog version while another was querying a pg_catalog.pg_class table inside a transaction.
    • The fix is to use single connection for both schema history table management and applying migrations
  • Also, made tableEntries map a static variable in YugabyteDBExecutionTemplate
    • This map keeps track of which entries are added to the lock table across different migrations from within the same app. Earlier, being an instance variable it was getting reinitialised for each migration and not serving its purpose.

Testing

  • Ran modified tests from flyway-tests repository. PR available here.

@ashetkar ashetkar requested review from kneeraj and Sfurti-yb August 9, 2024 14:05
@ashetkar
Copy link
Collaborator Author

ashetkar commented Aug 9, 2024

This PR is for review only. It'll be raised with upstream repository's main.

kneeraj
kneeraj approved these changes Aug 12, 2024
View reviewed changes
Copy link
Collaborator

@kneeraj kneeraj left a comment

Choose a reason for hiding this comment

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

lgtm.

One minor clarification seeked.

.../java/org/flywaydb/community/database/postgresql/yugabytedb/YugabyteDBExecutionTemplate.java

@CustomLog
public class YugabyteDBExecutionTemplate {

private final JdbcTemplate jdbcTemplate;
private final String tableName;
private final HashMap<String, Boolean> tableEntries = new HashMap<>();

private static final Map<String, Boolean> tableEntries = new ConcurrentHashMap<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: why changed the type to ConscurrentHashMap?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just in case, different threads in an application doing migrations try to update the map at the same time.

@ashetkar
Copy link
Collaborator Author

PR with upstream raised here: flyway#53
Closing this PR

@ashetkar ashetkar closed this Aug 12, 2024
@ashetkar ashetkar deleted the gh_23327 branch October 25, 2024 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kneeraj kneeraj kneeraj approved these changes

@Sfurti-yb Sfurti-yb Awaiting requested review from Sfurti-yb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ashetkar @kneeraj