Skip to content

MDEV-36397 Record change_user command in MTR output #4012

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 1 commit into
base: main
Choose a base branch
from

Conversation

FarihaIS
Copy link
Contributor

@FarihaIS FarihaIS commented Apr 24, 2025

Description

MTR .result files currently do not contain output to indicate if a change_user command has been executed in the corresponding .test files.

Record change_user command in the following format in MTR output only if enable_connect_log and enable_query_log is set to true:
change_user <user>,<password>,<db>;

Release Notes

N/A

How can this PR be tested?

Execute the main suite in mysql-test-run. This commit modifies tests in that suite.

For example, a change_user root; line in the .test file will generate a new change_user root,,; line in the .result file if no password or db is set.

If a parameter (user, db, or password) is not specified, the current value for that parameter will be used in the output.

Basing the PR against the correct MariaDB version

  • This is a new feature and the PR is based against the latest MariaDB development branch.

Copyright

All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@svoj svoj added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Apr 24, 2025
@FarihaIS FarihaIS force-pushed the mdev-36397 branch 2 times, most recently from 56f4c21 to 790bb72 Compare April 24, 2025 21:23
Copy link
Member

@vuvova vuvova left a comment

Choose a reason for hiding this comment

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

Good, thanks! Result files became much easier to read!

See few comments inline.

@@ -4655,6 +4655,24 @@ void do_change_user(struct st_command *command)
dynstr_set(&ds_db, mysql->db);
}

/* Connection logging if enabled */
if (!disable_result_log && !disable_connect_log && !disable_query_log)
Copy link
Member

Choose a reason for hiding this comment

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

this is getting too complex, let's just have if (!disable_query_log) here

Copy link
Contributor Author

@FarihaIS FarihaIS Apr 28, 2025

Choose a reason for hiding this comment

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

Good point, I've changed it as per your suggestion.

I did notice, however, that excluding !disable_result_log from the check causes main.plugin_vars to fail either locally or in the PR's CI/CD run.

Locally, I'm seeing a result length mismatch like so:

connection con2;
CALL p_show_vars(100);
connection default;
+change_user root,,test;
...
+change_user root,,test;
connection con1;

But after running the test with --record and pushing my changes, in the CI/CD run, I'm seeing a result length mismatch like so:

connection con2;
CALL p_show_vars(100);
connection default;
-change_user root,,test;
...
-change_user root,,test;
+change_user root,,;
...
+change_user root,,;
connection con1;

What do you suggest I do?

Copy link
Member

Choose a reason for hiding this comment

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

Note, that what fails is debug-emb test step. On the amd64-debian-12-debug-embedded builder. It runs the mtr with the --embedded switch, you can see it in the log. That is,

  • run cmake -DWITH_EMBEDDED_SERVER=1
  • make (or ninja or whatever you're using)
  • run ./mtr --emb main.plugin_vars
  • figure out why it's different in embedded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do, thanks for the tip

MTR .result files currently do not contain output to indicate if a
change_user command has been executed in the corresponding .test files.

Record change_user command in MTR output in the following format in MTR
output only if enable_connect_log and enable_query_log is set to true:
change_user <user>,<password>,<db>;

All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the
BSD-new license. I am contributing on behalf of my employer Amazon Web
Services, Inc.
@vuvova
Copy link
Member

vuvova commented Apr 28, 2025

Don't forget to re-request review when you're ready.

@FarihaIS FarihaIS requested a review from vuvova April 28, 2025 17:22
@grooverdan
Copy link
Member

it looks like your main.plugin_vars test is still having a different result under embedded.

@FarihaIS
Copy link
Contributor Author

it looks like your main.plugin_vars test is still having a different result under embedded.

I haven't pushed any changes to rectify the failure yet, I'm still working on the fix - thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements.
Development

Successfully merging this pull request may close these issues.

5 participants