sql/handler: referenced_by_foreign_key() returns bool

[MaxKellermann]

• 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]

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]

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]

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

[MaxKellermann]

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.

[dr-m]

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.

[MaxKellermann]

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

[dr-m]

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.

[MaxKellermann]

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.

[dr-m]

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]

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.