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-21166 Add Mroonga initialized check to Mroonga UDFs #3329

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

Conversation

kou
Copy link
Contributor

@kou kou commented Jun 12, 2024

  • The Jira issue number for this PR is: MDEV-21166

Description

If Mroonga UDFs such as last_insert_id() is used without INSTALL PLUGIN mroonga, MariaDB is crashed. See MDEV-21166 for unexpected behavior details.

Release Notes

I think that we don't need to add this to the release notes because this is unexpected usage. Mroonga UDFs are available only with INSTALL PLUGIN mroonga. This changes just add validations for unexpected usage.

How can this PR be tested?

  1. Execute CREATE FUNCTION last_insert_grn_id RETURNS INTEGER SONAME 'ha_mroonga.so' without INSTALL PLUGIN mroonga
  2. Execute SELECT last_insert_grn_id();
  3. It returns an error (ERROR 1123 (HY000): Can't initialize function 'last_insert_grn_id'; last_insert_grn_id(): Mroonga isn't initialized) without crash

We can't add .test for this because we enable Mroonga for all tests under storage/mroonga/mysql-test/mroonga/storage/:

--loose-plugin-load-add=$HA_MROONGA_SO --loose-plugin-mroonga=ON

Basing the PR against the correct MariaDB version

  • This is a new feature and the PR is based against the latest MariaDB development branch.
  • 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

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

Mroonga UDFs can't be used without loading Mroonga.
@kou
Copy link
Contributor Author

kou commented Jun 12, 2024

#3307 (review)

As for testing, whilst it is true there is a suite wide option for "storage" and "wrapper", there is no reason why another directory / suite could not be created with this as the first test in it.

Is this check important for MariaDB? If so, the approach will be reasonable.
(From my pointer of view, this is unsupported usage. ( https://jira.mariadb.org/browse/MDEV-21166?focusedCommentId=280394&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-280394 ) So this check isn't important for me.)

@@ -296,6 +297,7 @@ static PSI_mutex_info mrn_mutexes[] =
#endif

/* global variables */
bool mrn_initialized = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Continuing the comment in 3307:

Is this safe for multi-threading or CPU optimization? Is there a need to add mrn_mutexes similar to other global variables?

Yes. It's safe for multi-threading. This is changed only by st_mysql_plugin::init and st_mysql_plugin::deinit and they aren't called with multiple threads at once.

I'm not sure for CPU optimization. What situation do you think about?

I mean you probably need to ensure the memory barrier using mutex or atomic operations to maintain ordering constraints and avoid inconsistent memory views between threads due to CPU and compiler optimizations?

Copy link
Member

Choose a reason for hiding this comment

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

There's a LOCK_plugin around the initialization and de-initialization of all plugins and UDFs so there isn't actually a race condition here.

I'm still preferring the plugin_is_ready enhancement to the server below to facilitate consistent checking of relations between plugins.

Copy link
Member

@grooverdan grooverdan left a comment

Choose a reason for hiding this comment

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

@vuvova what you do you think about exposing the plugin_is_ready function to plugins in include/mysql/services*.h?

Sounds like a more consistent approach than global variables.

@vuvova
Copy link
Member

vuvova commented Jun 16, 2024

I think mrn_initialized is fine here, a plugin has easier ways of checking whether it's loaded than searching a global plugin hash.

But generally at some point in the future — yes, it's needed for inter-plugin dependencies.Spider already uses it to check for Aria.

Copy link
Contributor

@LinuxJedi LinuxJedi left a comment

Choose a reason for hiding this comment

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

Approved based on my review of the previous PR and the comments in this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants