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.

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

MDEV-26231:mysql_upgrade attempts to remove plugins which it failed to install #2840

Open
wants to merge 1 commit into
base: 10.5
Choose a base branch
from

Conversation

an3l
Copy link
Collaborator

@an3l an3l commented Nov 14, 2023

When allocating array for installed MYSQL_JSON plugin, size is set.
When uninstalling the plugin, check string instead of size.

Basing the PR against the correct MariaDB version

  • This is a new feature and the PR is based against the latest MariaDB development branch.
  • [ x] This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • [ x] I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • [x ] For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

@an3l an3l added the MariaDB Foundation Pull requests created by MariaDB Foundation label Nov 14, 2023
@an3l an3l added this to the 10.5 milestone Nov 14, 2023
@an3l an3l requested a review from cvicentiu November 14, 2023 12:59
Copy link
Member

@cvicentiu cvicentiu left a comment

Choose a reason for hiding this comment

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

Hi @an3l!

I don't understand what this patch changes.

ds_plugin_data_types only has its length member != 0 if installing a plugin succeded.

First initialization of the dynamic string is here:

int main(int argc, char **argv)
{
[...]

  if (init_dynamic_string(&ds_args, "", 512, 256) ||
      init_dynamic_string(&conn_args, "", 512, 256) ||
      init_dynamic_string(&ds_plugin_data_types, "", 512, 256))
    die("Out of memory");

This sets the length of the string to 0, but it pre-allocates. Fine.

Then the only place that changes the ds_plugin_data_types string is:

        if(!run_query("INSTALL SONAME 'type_mysql_json'", NULL, TRUE))
        {
          dynstr_append(&ds_plugin_data_types, "'type_mysql_json'");
          dynstr_append(&ds_plugin_data_types, "\n");
          break;
        }

So my question then is:

Does run_query return success even if the executed query threw an error? In that case, the bug should be fixed there, because the length checking of ds_plugin_data_types seems fine to me.

an3l added a commit to an3l/server that referenced this pull request Nov 30, 2023
…o install

- Protect plugin uninstall in case plugin didn't installed at all, what
may happen if query returned and error.

- Closes PR MariaDB#2840
- Reviewer: <[email protected]>
@an3l an3l force-pushed the bb-10.5-anel-MDEV-26231-mysql_upgrade_remove_failed_plugins branch from 2702228 to dc105f2 Compare November 30, 2023 14:16
@an3l
Copy link
Collaborator Author

an3l commented Nov 30, 2023

HI @cvicentiu , yes I have updated now to catch error for query plugin and properly skip uninstall phase if there is no plugin installed at all.
Results:

  • Simulate wrong query:
if(!run_query("INSTALL SONAME 'type_mysql_json1'", &ds_result, TRUE))

Check clients

# No plugin installed
MariaDB [test]> show create table t;
ERROR 4161 (HY000): Unknown data type: 'MYSQL_JSON'

# mariadb_upgrade
$ sudo ./client/mysql_upgrade -uroot -S /tmp/mysql.sock --force
Phase 2/7: Installing used storage engines... Skipped
installing plugin for MYSQL_JSON data type
Failed to install the plugin 'type_mysql_json' for MYSQL_JSON data type
FATAL ERROR: Upgrade failed

…o install

- Protect plugin uninstall in case plugin didn't installed at all, what
may happen if query returned and error.

- Closes PR MariaDB#2840
- Reviewer: <[email protected]>
@an3l an3l force-pushed the bb-10.5-anel-MDEV-26231-mysql_upgrade_remove_failed_plugins branch from dc105f2 to 7d63e08 Compare March 7, 2024 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MariaDB Foundation Pull requests created by MariaDB Foundation
2 participants