-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
sql/handler: referenced_by_foreign_key() returns bool #3530
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While you are changing this, I would suggest that you also add There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 More than a decade ago, there was another storage engine PBXT that supported There is a thin wrapper in Mroonga that would invoke InnoDB, and in fact it does not look like the macro There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the return value of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But what is really being allocated for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; |
||
virtual void init_table_handle_for_HANDLER() | ||
{ return; } /* prepare InnoDB for HANDLER */ | ||
virtual void free_foreign_key_create_info(char* str) {} | ||
|
There was a problem hiding this comment.
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 addingoverride
.