Skip to content

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

Open
an3l wants to merge 1 commit into
MariaDB:10.5from
an3l:bb-10.5-anel-MDEV-26231-mysql_upgrade_remove_failed_plugins
Open

MDEV-26231:mysql_upgrade attempts to remove plugins which it failed to install#2840
an3l wants to merge 1 commit into
MariaDB:10.5from
an3l:bb-10.5-anel-MDEV-26231-mysql_upgrade_remove_failed_plugins

Conversation

@an3l

@an3l an3l commented Nov 14, 2023

Copy link
Copy Markdown
Contributor

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

@cvicentiu cvicentiu left a comment

Copy link
Copy Markdown
Contributor

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: <cvicentiu@mariadb.org>
@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

an3l commented Nov 30, 2023

Copy link
Copy Markdown
Contributor Author

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: <cvicentiu@mariadb.org>
@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

Development

Successfully merging this pull request may close these issues.

2 participants