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.

Sign up for GitHub

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

Jump to bottom

sql/handler: referenced_by_foreign_key() returns bool #3530

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

sql/handler: referenced_by_foreign_key() returns bool #3530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sql/ha_partition.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1465,7 +1465,7 @@ class ha_partition final :public handler

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()
Comment on lines 1466 to +1468
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

*/
bool can_switch_engines() override;
/*
Expand Down
2 changes: 1 addition & 1 deletion sql/handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4552,7 +4552,7 @@ class handler :public Sql_alloc
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;}
Comment on lines 4552 to +4555
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

virtual void init_table_handle_for_HANDLER()
{ return; } /* prepare InnoDB for HANDLER */
virtual void free_foreign_key_create_info(char* str) {}
Expand Down
2 changes: 1 addition & 1 deletion storage/innobase/handler/ha_innodb.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15640,7 +15640,7 @@ delete is then allowed internally to resolve a duplicate key conflict in
REPLACE, not an update.
@return > 0 if referenced by a FOREIGN KEY */

uint ha_innobase::referenced_by_foreign_key()
bool ha_innobase::referenced_by_foreign_key()
{
dict_sys.freeze(SRW_LOCK_CALL);
const bool empty= m_prebuilt->table->referenced_set.empty();
Expand Down
2 changes: 1 addition & 1 deletion storage/innobase/handler/ha_innodb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ class ha_innobase final : public handler

bool can_switch_engines() override;

uint referenced_by_foreign_key() override;
bool referenced_by_foreign_key() override;

void free_foreign_key_create_info(char* str) override { my_free(str); }

Expand Down
12 changes: 6 additions & 6 deletions storage/mroonga/ha_mroonga.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16846,10 +16846,10 @@ int ha_mroonga::get_parent_foreign_key_list(const THD *thd,
DBUG_RETURN(res);
}

uint ha_mroonga::wrapper_referenced_by_foreign_key()
bool ha_mroonga::wrapper_referenced_by_foreign_key()
{
MRN_DBUG_ENTER_METHOD();
uint res;
bool res;
MRN_SET_WRAP_SHARE_KEY(share, table->s);
MRN_SET_WRAP_TABLE_KEY(this, table);
res = wrap_handler->referenced_by_foreign_key();
Expand All @@ -16858,17 +16858,17 @@ uint ha_mroonga::wrapper_referenced_by_foreign_key()
DBUG_RETURN(res);
}

uint ha_mroonga::storage_referenced_by_foreign_key()
bool ha_mroonga::storage_referenced_by_foreign_key()
{
MRN_DBUG_ENTER_METHOD();
uint res = handler::referenced_by_foreign_key();
bool res = handler::referenced_by_foreign_key();
DBUG_RETURN(res);
}

uint ha_mroonga::referenced_by_foreign_key()
bool ha_mroonga::referenced_by_foreign_key()
{
MRN_DBUG_ENTER_METHOD();
uint res;
bool res;
if (share->wrapper_mode)
{
res = wrapper_referenced_by_foreign_key();
Expand Down
6 changes: 3 additions & 3 deletions storage/mroonga/ha_mroonga.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,7 @@ class ha_mroonga: public handler
bool can_switch_engines() mrn_override;
int get_foreign_key_list(const THD *thd, List<FOREIGN_KEY_INFO> *f_key_list) mrn_override;
int get_parent_foreign_key_list(const THD *thd, List<FOREIGN_KEY_INFO> *f_key_list) mrn_override;
uint referenced_by_foreign_key() mrn_override;
bool referenced_by_foreign_key() mrn_override;
void init_table_handle_for_HANDLER() mrn_override;
void free_foreign_key_create_info(char* str) mrn_override;
#ifdef MRN_HAVE_HA_REBIND_PSI
Expand Down Expand Up @@ -1274,8 +1274,8 @@ class ha_mroonga: public handler
int storage_get_foreign_key_list(const THD *thd, List<FOREIGN_KEY_INFO> *f_key_list);
int wrapper_get_parent_foreign_key_list(const THD *thd, List<FOREIGN_KEY_INFO> *f_key_list);
int storage_get_parent_foreign_key_list(const THD *thd, List<FOREIGN_KEY_INFO> *f_key_list);
uint wrapper_referenced_by_foreign_key();
uint storage_referenced_by_foreign_key();
bool wrapper_referenced_by_foreign_key();
bool storage_referenced_by_foreign_key();
void wrapper_init_table_handle_for_HANDLER();
void storage_init_table_handle_for_HANDLER();
void wrapper_free_foreign_key_create_info(char* str);
Expand Down