Skip to content

PT-1776: Adding in slave-user and slave-password for dsn table based …#425

Open
mbenshoof wants to merge 7 commits into
percona:3.xfrom
mbenshoof:mbenshoof-slave-dsn
Open

PT-1776: Adding in slave-user and slave-password for dsn table based …#425
mbenshoof wants to merge 7 commits into
percona:3.xfrom
mbenshoof:mbenshoof-slave-dsn

Conversation

@mbenshoof

Copy link
Copy Markdown

…recursion

@it-percona

it-percona commented Oct 25, 2019

Copy link
Copy Markdown
Contributor

CLA assistant check
All committers have signed the CLA.

@svetasmirnova svetasmirnova left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you for your contribution.

You modified the auto-generated code. It will be removed once we run the update-modules utility. Please put your changes into the MasterSlave package.

@svetasmirnova

Copy link
Copy Markdown
Collaborator

Another issue with this proposal is that it modifies DSN table entries without checking if they have user and password defined. This could cause issues when users have, say, 10 replicas with the same user/password pairs and two with different ones.

We also need a test case to accept this PR.

@svetasmirnova

Copy link
Copy Markdown
Collaborator

There is also similar PR at #381

… making them not overwrite what is specified in DSN table.
@mbenshoof

Copy link
Copy Markdown
Author

@svetasmirnova - Modified the logic to check for user/pass specified in DSN string. Only sets the CLI passed options if they are not set in the DSN table.

For the test case, I've been using dbdeployer for a quick primary/replica environment and setting different variations of DSN string in the table (with both user/pass, with only one, with neither) to validate it doesn't overwrite, but sets if missing

@svetasmirnova

Copy link
Copy Markdown
Collaborator

@mbenshoof

Thank you for the changes; I will review them in the new year.

For the test case, I've been using dbdeployer for a quick primary/replica environment and setting different variations of the DSN string in the table (with both user/pass, with only one, with neither) to validate it doesn't overwrite, but sets if missing

We need automatic test cases to run them at each release and ensure your changes are not overwritten by another contribution. We use a solution similar to MySQL Sandbox and dbdeployer for automatic test cases. Please check directory t and CONTRIBUTING.md.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to make DSN-table–based recursion honor --slave-user and --slave-password when constructing slave connections, aligning DSN-table recursion with other recursion methods in Percona Toolkit’s replication tooling.

Changes:

  • Adds logic in lib/MasterSlave.pm to apply --slave-user/--slave-password when DSNs are sourced from a DSN table.
  • Touches the embedded MasterSlave copies inside pt-table-sync and pt-table-checksum (currently only whitespace), but they still need the functional update to actually deliver the feature in those tools.
  • No tests added/updated for the new DSN-table credential override behavior.

Reviewed changes

Copilot reviewed 1 out of 3 changed files in this pull request and generated 5 comments.

File Description
lib/MasterSlave.pm Adds DSN-table credential injection; currently risks password exposure and fragile DSN-string construction; should instead override creds on parsed DSN hashrefs and preserve “copy from previous DSN” semantics.
bin/pt-table-sync Embedded MasterSlave copy still does not apply --slave-user/--slave-password for DSN-table recursion (only whitespace change in this PR).
bin/pt-table-checksum Same as pt-table-sync: embedded MasterSlave copy still lacks the feature (only whitespace change in this PR).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/MasterSlave.pm
Comment thread lib/MasterSlave.pm Outdated
Comment thread lib/MasterSlave.pm Outdated
Comment thread lib/MasterSlave.pm Outdated
Comment thread lib/MasterSlave.pm Outdated
@svetasmirnova svetasmirnova changed the base branch from 3.0 to 3.x June 12, 2026 12:41
svetasmirnova and others added 5 commits June 12, 2026 16:07
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- Merged version 3.x
- Run update-modules and fixed some tests
- Re-added the fix
- Created test case for lib/MasterSlave.pm
- Updated modules
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants