sql/handler: referenced_by_foreign_key() returns bool #3530

MaxKellermann · 2024-09-20T06:56:14Z

The Jira issue number for this PR is: MDEV-______

Description

The method was declared to return an unsigned integer, but it is really a boolean (and used as such by all callers).

Release Notes

No runtime change.

How can this PR be tested?

No runtime change.

Basing the PR against the correct MariaDB version

This is a new feature or a refactoring, and the PR is based against the main 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.

MaxKellermann · 2024-09-20T08:07:43Z

I don't understand the buildbot/amd64-debian-11-msan failure and I assume it's not related to this change.

dr-m

I see that this code was added a long time ago in e2646f0 as part of an ACID breaking hack that was removed in 1bd681c. The creator of InnoDB preferred C to C++, and in C at that time there was no Boolean data type.

I think that a trivial mechanical change like this had better be applied to the oldest supported major version branch, which currently is 10.5. It would also be good to file an MDEV ticket for this at https://jira.mariadb.org, like our pull request template encourages you to do.

dr-m · 2024-09-20T08:45:03Z

sql/ha_partition.h

    virtual int get_foreign_key_list(THD *thd,
    List<FOREIGN_KEY_INFO> *f_key_list)
-    virtual uint referenced_by_foreign_key()
+    virtual bool referenced_by_foreign_key()


It’s nice that you are updating this comment to be consistent with your change. I’d suggest to make it even more consistent, by removing the virtual and adding override.

dr-m · 2024-09-20T08:47:16Z

sql/handler.h

  virtual int
  get_parent_foreign_key_list(const THD *thd, List<FOREIGN_KEY_INFO> *f_key_list)
  { return 0; }
-  virtual uint referenced_by_foreign_key() { return 0;}
+  virtual bool referenced_by_foreign_key() { return false;}


While you are changing this, I would suggest that you also add const qualifiers and change also the int return type to void.

Usually, I avoid doing two unrelated things in one commit, and I'm not sure if it's possible to add const in all implementations. But if you say so, I'll amend the commit.

Only InnoDB supports foreign keys, and both member functions are related to that. Actually, also handler::get_foreign_key_list() should return void.

More than a decade ago, there was another storage engine PBXT that supported FOREIGN KEY constraints, but it seems that they did not switch to MariaDB when Oracle changes policies around MySQL.

There is a thin wrapper in Mroonga that would invoke InnoDB, and in fact it does not look like the macro MRN_SET_WRAP_TABLE_KEY would be const compatible.

If the return value of get_foreign_key_list() is not needed to signal error conditions, why not just return the List<> from it?

I was thinking the same, but that collection is actually allocated by the caller, typically from the stack. Only in the rare case that FOREIGN KEY constraints exist, some heap memory could be allocated and attached to the object. I assume that there are specific reasons why the custom List template is being used instead of something like std::list or the C++11 std::forward_list.

But what is really being allocated for List? It's a trivial class that contains just three fields: two pointers and an integer. Nothing is allocated until something gets pushed to the list (except for 24 bytes of stack space, which is always needed, no matter how you pass it).
For the generated machine code, there's no fundamental difference whether you pass it as a reference parameter or as return value - in both cases, the caller allocates stack space. For the coding style, it makes a big difference because now your List is truly declared as a return value, and not as some sort of out-parameter kludge.

You may very well be right. I usually don’t write functions that would return more than would fit in a single register; page_id_t would be an exception on 32-bit targets. This should be easy to check with https://godbolt.org. Feel free to change the return type. There might be some additional overhead in the Mroonga call wrapper, but that storage engine is hardly maintained or used. Even that overhead should be rather negligible, assuming that the List is simply copied byte by byte, without any deep-copy constructor.

dr-m · 2024-09-20T09:03:02Z

I don't understand the buildbot/amd64-debian-11-msan failure and I assume it's not related to this change.

In https://ci.mariadb.org/50358/logs/amd64-debian-11-msan/var.tar.gz the file mysql-test/var/35/log/mysqld.1.errcontains the following, which looks like there was a hang.

2024-09-20  7:46:59 0 [Note] Plugin 'parsec' is disabled.
2024-09-20  7:46:59 0 [Note] Plugin 'wsrep_provider' is disabled.
2024-09-20  7:46:59 0 [Warning] /home/buildbot/amd64-debian-11-msan/build/sql/mariadbd: unknown option '--loose-pam-debug'
2024-09-20  7:46:59 0 [Warning] /home/buildbot/amd64-debian-11-msan/build/sql/mariadbd: unknown option '--loose-aria'
2024-09-20  7:46:59 0 [Warning] /home/buildbot/amd64-debian-11-msan/build/sql/mariadbd: unknown option '--loose-enable-named-pipe'
CURRENT_TEST: perfschema.table_lock_aggregate_hist_2u_3t
$ /home/buildbot/amd64-debian-11-msan/build/sql/mariadbd --defaults-group-suffix=.1 --defaults-file=/home/buildbot/amd64-debian-11-msan/build/mysql-test/var/35/my.cnf --log-output=file --core-file --loose-debug-sync-timeout=300
2024-09-20  7:50:20 0 [Warning] Could not increase number of max_open_files to more than 1024 (request: 32310)

It looks like nothing was happening for almost 4 minutes. The mariadbd process was killed and a new one (for a different test) started. Unfortunately, the mtr test driver does not always produce core dumps or stack traces for hangs. Maybe @vuvova can comment on that. In any case, it definitely is unrelated to your change.

In case you are interested, there are many other builders that for some reason do not report any status to GitHub but are aggregated in the Buildbot grid view.

CLAassistant · 2024-09-23T07:03:42Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ MaxKellermann
❌ dr-m
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

MaxKellermann · 2024-09-23T07:05:28Z

Rebased on 10.5 and added const & noexcept.
I think changing other methods to return List<> (or change the unnecessary return value to void) is out of this PR's scope.

@dr-m, do you want me to write a JIRA ticket for this?

dr-m

Thank you, this looks OK to me, with one minor nitpick.

I don’t know if filing a MDEV for this is mandatory.

storage/innobase/handler/ha_innodb.cc

The method was declared to return an unsigned integer, but it is really a boolean (and used as such by all callers). A secondary change is the addition of "const" and "noexcept" to this method. In ha_mroonga.cpp, I also added "inline" to the two helper methods of referenced_by_foreign_key(). This allows the compiler to flatten the method.

dr-m · 2024-09-30T06:48:16Z

@MaxKellermann, I understood that it is not needed to file an MDEV for a trivial change like this, which does not need to appear in the release notes.

However, it looks like you haven’t signed a contributor agreement or declared that these changes are under the BSD-new license; see the FAQ.

MaxKellermann · 2024-09-30T08:17:51Z

However, it looks like you haven’t signed a contributor agreement or declared that these changes are under the BSD-new license; see the FAQ.

I did, and the check succeeded on my other PRs. I think this is a bug in the CLA check bot.

dr-m · 2024-09-30T13:08:04Z

Oh, sorry, I see that the CLA check is actually complaining about myself. I have not signed the CLA but something equivalent. So, this should be ready to merge.

dr-m reviewed Sep 20, 2024

View reviewed changes

MaxKellermann force-pushed the bool_referenced_by_foreign_key branch from 6011698 to 23a9d39 Compare September 23, 2024 07:03

MaxKellermann changed the base branch from main to 10.5 September 23, 2024 07:04

MaxKellermann force-pushed the bool_referenced_by_foreign_key branch from 23a9d39 to e45c915 Compare September 23, 2024 11:04

dr-m approved these changes Sep 27, 2024

View reviewed changes

storage/innobase/handler/ha_innodb.cc Outdated Show resolved Hide resolved

MaxKellermann force-pushed the bool_referenced_by_foreign_key branch from e45c915 to e1ff5d3 Compare September 27, 2024 13:33

Merge branch '10.5' into bool_referenced_by_foreign_key

a43910d

dr-m enabled auto-merge (rebase) September 30, 2024 06:04

dr-m disabled auto-merge September 30, 2024 06:43

Merge branch '10.5' into bool_referenced_by_foreign_key

f6dcf36

dr-m enabled auto-merge (rebase) September 30, 2024 13:07

dr-m merged commit 45298b7 into MariaDB:10.5 Sep 30, 2024
8 of 11 checks passed

MaxKellermann deleted the bool_referenced_by_foreign_key branch October 1, 2024 06:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql/handler: referenced_by_foreign_key() returns bool #3530

sql/handler: referenced_by_foreign_key() returns bool #3530

MaxKellermann commented Sep 20, 2024

MaxKellermann commented Sep 20, 2024

dr-m left a comment

dr-m Sep 20, 2024

dr-m Sep 20, 2024

MaxKellermann Sep 20, 2024

dr-m Sep 20, 2024

MaxKellermann Sep 20, 2024

dr-m Sep 20, 2024

MaxKellermann Sep 20, 2024

dr-m Sep 20, 2024

dr-m commented Sep 20, 2024

CLAassistant commented Sep 23, 2024 •

edited

Loading

MaxKellermann commented Sep 23, 2024

dr-m left a comment

dr-m commented Sep 30, 2024

MaxKellermann commented Sep 30, 2024

dr-m commented Sep 30, 2024

sql/handler: referenced_by_foreign_key() returns bool #3530

sql/handler: referenced_by_foreign_key() returns bool #3530

Conversation

MaxKellermann commented Sep 20, 2024

Description

Release Notes

How can this PR be tested?

Basing the PR against the correct MariaDB version

PR quality check

MaxKellermann commented Sep 20, 2024

dr-m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-m commented Sep 20, 2024

CLAassistant commented Sep 23, 2024 • edited Loading

MaxKellermann commented Sep 23, 2024

dr-m left a comment

Choose a reason for hiding this comment

dr-m commented Sep 30, 2024

MaxKellermann commented Sep 30, 2024

dr-m commented Sep 30, 2024

CLAassistant commented Sep 23, 2024 •

edited

Loading