Re: [Maria-developers] c91ec05e01b: MDEV-20865 Store foreign key info in TABLE_SHARE
Hi, Aleksey! Just a question for now (and a couple of style comments, I didn't look at the logic yet) On Dec 04, Aleksey Midenkov wrote:
revision-id: c91ec05e01b (mariadb-10.4.4-427-gc91ec05e01b) parent(s): e6d653b448d author: Aleksey Midenkov <midenok@gmail.com> committer: Aleksey Midenkov <midenok@gmail.com> timestamp: 2019-11-26 13:04:07 +0300 message:
MDEV-20865 Store foreign key info in TABLE_SHARE
1. Access foreign keys via TABLE_SHARE::foreign_keys and TABLE_SHARE::referenced_keys;
2. Remove handler FK interface:
- get_foreign_key_list() - get_parent_foreign_key_list() - referenced_by_foreign_key()
Good, that was the goal
3. Invalidate referenced shares on:
- RENAME TABLE - DROP TABLE - RENAME COLUMN - CREATE TABLE - ADD FOREIGN KEY
When foreign table is created or altered by the above operations all referenced shares are closed. This blocks the operation while any referenced shares are used (when at least one its TABLE instance is locked).
And this is the main question of this email: Why do you close referenced tables? Minor comments below
diff --git a/sql/handler.h b/sql/handler.h index e913af1d15d..10984505f70 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -1030,6 +1031,15 @@ struct TABLE_SHARE; struct HA_CREATE_INFO; struct st_foreign_key_info; typedef struct st_foreign_key_info FOREIGN_KEY_INFO; +class Table_ident; +class FK_list : public List<FOREIGN_KEY_INFO> +{ +public: + /* Get all referenced tables for foreign key fk_name. */ + bool get(THD *thd, std::set<Table_ident> &result, LEX_CSTRING &fk_name, bool foreign); + /* Get all referenced or foreign tables. */ + bool get(THD *thd, std::set<Table_ident> &result, bool foreign);
Seems unnecessary. Copying a list into a std::set _could_ be justified, if you'd later used it for quick checks "if this table belongs to a set" - something you cannot quickly do with a List. But as far as I can see you copy the List into std::set and then iterate this set. This doesn't make much sense, you can iterate the original list just fine.
+}; typedef bool (stat_print_fn)(THD *thd, const char *type, size_t type_len, const char *file, size_t file_len, const char *status, size_t status_len); diff --git a/sql/lex_string.h b/sql/lex_string.h index a62609c6b60..769f4dcbf5e 100644 --- a/sql/lex_string.h +++ b/sql/lex_string.h @@ -41,11 +41,47 @@ class Lex_cstring : public LEX_CSTRING str= start; length= end - start; } + Lex_cstring(const LEX_CSTRING &src) + { + str= src.str; + length= src.length; + } void set(const char *_str, size_t _len) { str= _str; length= _len; } + Lex_cstring *strdup_root(MEM_ROOT &mem_root)
The way you use it, it looks like you really need a constructor, not a strdup.
+ { + Lex_cstring *dst= + (Lex_cstring *) alloc_root(&mem_root, sizeof(Lex_cstring)); + if (!dst) + return NULL; + if (!str) + { + dst->str= NULL; + dst->length= 0; + return dst; + } + dst->str= (const char *) memdup_root(&mem_root, str, length + 1); + if (!dst->str) + return NULL; + dst->length= length; + return dst; + } + bool operator< (const Lex_cstring& rhs) const + { + return length < rhs.length || (length == rhs.length && memcmp(str, rhs.str, length) < 0); + } + bool operator== (const Lex_cstring& rhs) const + { + return length == rhs.length && 0 == memcmp(str, rhs.str, length); + } + bool operator> (const Lex_cstring& rhs) const + { + return length > rhs.length || (length == rhs.length && memcmp(str, rhs.str, length) > 0); + }
Nope. We've been here before, haven't we? Don't overload operators. If you want a method to compare, call it cmp(), or greater_than(), or something btw your comparison has quite weird and unconventional semantics
+ };
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei! On Wed, Dec 4, 2019 at 10:10 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
Just a question for now (and a couple of style comments, I didn't look at the logic yet)
On Dec 04, Aleksey Midenkov wrote:
revision-id: c91ec05e01b (mariadb-10.4.4-427-gc91ec05e01b) parent(s): e6d653b448d author: Aleksey Midenkov <midenok@gmail.com> committer: Aleksey Midenkov <midenok@gmail.com> timestamp: 2019-11-26 13:04:07 +0300 message:
MDEV-20865 Store foreign key info in TABLE_SHARE
1. Access foreign keys via TABLE_SHARE::foreign_keys and TABLE_SHARE::referenced_keys;
2. Remove handler FK interface:
- get_foreign_key_list() - get_parent_foreign_key_list() - referenced_by_foreign_key()
Good, that was the goal
3. Invalidate referenced shares on:
- RENAME TABLE - DROP TABLE - RENAME COLUMN - CREATE TABLE - ADD FOREIGN KEY
When foreign table is created or altered by the above operations all referenced shares are closed. This blocks the operation while any referenced shares are used (when at least one its TABLE instance is locked).
And this is the main question of this email: Why do you close referenced tables?
Table cannot operate with wrong TABLE_SHARE::referenced_keys, so it must be locked anyway whether referenced_keys are modified or reloaded. The choice was made to reload as the first simplest implementation. And unless we know real performance drawback I'd like to keep it that way. Modifying TABLE_SHARE::referenced_keys requires several different algorithms for the above operations.
Minor comments below.
diff --git a/sql/handler.h b/sql/handler.h index e913af1d15d..10984505f70 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -1030,6 +1031,15 @@ struct TABLE_SHARE; struct HA_CREATE_INFO; struct st_foreign_key_info; typedef struct st_foreign_key_info FOREIGN_KEY_INFO; +class Table_ident; +class FK_list : public List<FOREIGN_KEY_INFO> +{ +public: + /* Get all referenced tables for foreign key fk_name. */ + bool get(THD *thd, std::set<Table_ident> &result, LEX_CSTRING &fk_name, bool foreign); + /* Get all referenced or foreign tables. */ + bool get(THD *thd, std::set<Table_ident> &result, bool foreign);
Seems unnecessary. Copying a list into a std::set _could_ be justified, if you'd later used it for quick checks "if this table belongs to a set" - something you cannot quickly do with a List.
But as far as I can see you copy the List into std::set and then iterate this set. This doesn't make much sense, you can iterate the original list just fine.
std::set does duplicate elimination. Iterating a list would require an algorithm to skip the duplicates.
+}; typedef bool (stat_print_fn)(THD *thd, const char *type, size_t type_len, const char *file, size_t file_len, const char *status, size_t status_len); diff --git a/sql/lex_string.h b/sql/lex_string.h index a62609c6b60..769f4dcbf5e 100644 --- a/sql/lex_string.h +++ b/sql/lex_string.h @@ -41,11 +41,47 @@ class Lex_cstring : public LEX_CSTRING str= start; length= end - start; } + Lex_cstring(const LEX_CSTRING &src) + { + str= src.str; + length= src.length; + } void set(const char *_str, size_t _len) { str= _str; length= _len; } + Lex_cstring *strdup_root(MEM_ROOT &mem_root)
The way you use it, it looks like you really need a constructor, not a strdup.
Probably, yes.
+ { + Lex_cstring *dst= + (Lex_cstring *) alloc_root(&mem_root, sizeof(Lex_cstring)); + if (!dst) + return NULL; + if (!str) + { + dst->str= NULL; + dst->length= 0; + return dst; + } + dst->str= (const char *) memdup_root(&mem_root, str, length + 1); + if (!dst->str) + return NULL; + dst->length= length; + return dst; + } + bool operator< (const Lex_cstring& rhs) const + { + return length < rhs.length || (length == rhs.length && memcmp(str, rhs.str, length) < 0); + } + bool operator== (const Lex_cstring& rhs) const + { + return length == rhs.length && 0 == memcmp(str, rhs.str, length); + } + bool operator> (const Lex_cstring& rhs) const + { + return length > rhs.length || (length == rhs.length && memcmp(str, rhs.str, length) > 0); + }
Nope. We've been here before, haven't we? Don't overload operators. If you want a method to compare, call it cmp(), or greater_than(), or something
btw your comparison has quite weird and unconventional semantics
Ok. I really like the idea of overloading the operators. Maybe one day we'll adopt them...
+ };
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok
Hi, Aleksey! On Dec 05, Aleksey Midenkov wrote:
3. Invalidate referenced shares on:
- RENAME TABLE - DROP TABLE - RENAME COLUMN - CREATE TABLE - ADD FOREIGN KEY
When foreign table is created or altered by the above operations all referenced shares are closed. This blocks the operation while any referenced shares are used (when at least one its TABLE instance is locked).
And this is the main question of this email: Why do you close referenced tables?
Table cannot operate with wrong TABLE_SHARE::referenced_keys, so it must be locked anyway whether referenced_keys are modified or reloaded. The choice was made to reload as the first simplest implementation. And unless we know real performance drawback I'd like to keep it that way. Modifying TABLE_SHARE::referenced_keys requires several different algorithms for the above operations.
I feel that closing tables is a bit too heavy. By the way, do you open referenced tables for SELECT? Or does table->s->referenced_keys stay NULL?
Minor comments below.
diff --git a/sql/handler.h b/sql/handler.h index e913af1d15d..10984505f70 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -1030,6 +1031,15 @@ struct TABLE_SHARE; struct HA_CREATE_INFO; struct st_foreign_key_info; typedef struct st_foreign_key_info FOREIGN_KEY_INFO; +class Table_ident; +class FK_list : public List<FOREIGN_KEY_INFO> +{ +public: + /* Get all referenced tables for foreign key fk_name. */ + bool get(THD *thd, std::set<Table_ident> &result, LEX_CSTRING &fk_name, bool foreign); + /* Get all referenced or foreign tables. */ + bool get(THD *thd, std::set<Table_ident> &result, bool foreign);
Seems unnecessary. Copying a list into a std::set _could_ be justified, if you'd later used it for quick checks "if this table belongs to a set" - something you cannot quickly do with a List.
But as far as I can see you copy the List into std::set and then iterate this set. This doesn't make much sense, you can iterate the original list just fine.
std::set does duplicate elimination. Iterating a list would require an algorithm to skip the duplicates.
Duplicates? Can there be duplicate foreign keys? Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Sergei! On Fri, Dec 6, 2019 at 5:34 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Dec 05, Aleksey Midenkov wrote:
3. Invalidate referenced shares on:
- RENAME TABLE - DROP TABLE - RENAME COLUMN - CREATE TABLE - ADD FOREIGN KEY
When foreign table is created or altered by the above operations all referenced shares are closed. This blocks the operation while any referenced shares are used (when at least one its TABLE instance is locked).
And this is the main question of this email: Why do you close referenced tables?
Table cannot operate with wrong TABLE_SHARE::referenced_keys, so it must be locked anyway whether referenced_keys are modified or reloaded. The choice was made to reload as the first simplest implementation. And unless we know real performance drawback I'd like to keep it that way. Modifying TABLE_SHARE::referenced_keys requires several different algorithms for the above operations.
I feel that closing tables is a bit too heavy.
By the way, do you open referenced tables for SELECT? Or does table->s->referenced_keys stay NULL?
Now it stays NULL, but next implementation with FRM files will see foreign tables list referencing it and it will need to open their shares to update its referenced_keys. I don't know if we need such level of independence as to not open foreign tables when not needed. But yes, that could be a feature to load foreign tables on demand.
Minor comments below.
diff --git a/sql/handler.h b/sql/handler.h index e913af1d15d..10984505f70 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -1030,6 +1031,15 @@ struct TABLE_SHARE; struct HA_CREATE_INFO; struct st_foreign_key_info; typedef struct st_foreign_key_info FOREIGN_KEY_INFO; +class Table_ident; +class FK_list : public List<FOREIGN_KEY_INFO> +{ +public: + /* Get all referenced tables for foreign key fk_name. */ + bool get(THD *thd, std::set<Table_ident> &result, LEX_CSTRING &fk_name, bool foreign); + /* Get all referenced or foreign tables. */ + bool get(THD *thd, std::set<Table_ident> &result, bool foreign);
Seems unnecessary. Copying a list into a std::set _could_ be justified, if you'd later used it for quick checks "if this table belongs to a set" - something you cannot quickly do with a List.
But as far as I can see you copy the List into std::set and then iterate this set. This doesn't make much sense, you can iterate the original list just fine.
std::set does duplicate elimination. Iterating a list would require an algorithm to skip the duplicates.
Duplicates? Can there be duplicate foreign keys?
No, but duplicates of Table_ident can be, because multiple foreign keys can reference same table.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok
Hi Sergei! On Wed, Dec 4, 2019 at 10:10 PM Sergei Golubchik <serg@mariadb.org> wrote:
...
+ Lex_cstring *strdup_root(MEM_ROOT &mem_root)
The way you use it, it looks like you really need a constructor, not a strdup.
On second thought, it can't be a constructor because it can fail with allocation error. But it's better to be from the other side: bool Lex_cstring::strdup(MEM_ROOT *mem_root, const Lex_cstring &src) { // allocate and deep-copy from src to this } I'd really like to use such utility methods instead of C variants like thd_make_lex_string(). For plugins we can have strdup() method accepting THD * and compiled inside server (non-inline): bool Lex_cstring::strdup(THD *thd, const Lex_cstring &src) { strdup(thd->mem_root, src); } We better go away from this C service layer of thd_*() functions between server and plugins and use class methods instead. We can't use THD directly though, because we don't want it compiled in plugins. So we compile non-inline methods in server that are used in plugin. -- All the best, Aleksey Midenkov @midenok
Hi, Aleksey! On Dec 18, Aleksey Midenkov wrote:
On Wed, Dec 4, 2019 at 10:10 PM Sergei Golubchik wrote:
But it's better to be from the other side:
bool Lex_cstring::strdup(MEM_ROOT *mem_root, const Lex_cstring &src) { // allocate and deep-copy from src to this }
I'd really like to use such utility methods instead of C variants like thd_make_lex_string(). ... We better go away from this C service layer of thd_*() functions between server and plugins and use class methods instead.
Why is it better? Isn't it just the syntax sugar? The only effect I can think of - pure C plugins won't be able to use thd_make_lex_string() if you replace it with a C++ method. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Sergei, On Wed, Dec 18, 2019 at 1:11 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Dec 18, Aleksey Midenkov wrote:
On Wed, Dec 4, 2019 at 10:10 PM Sergei Golubchik wrote:
But it's better to be from the other side:
bool Lex_cstring::strdup(MEM_ROOT *mem_root, const Lex_cstring &src) { // allocate and deep-copy from src to this }
I'd really like to use such utility methods instead of C variants like thd_make_lex_string(). ... We better go away from this C service layer of thd_*() functions between server and plugins and use class methods instead.
Why is it better? Isn't it just the syntax sugar?
Having local class interfaces is easier to maintain. Additional API layer is development costs overhead. It is good for version compatibility control, but we don't have third-party plugins, do we? And we don't strictly use API to get server services into plugin, AFAIK.
The only effect I can think of - pure C plugins won't be able to use thd_make_lex_string() if you replace it with a C++ method.
Do we have such plugins and do we need them to stay pure C?
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok
Hi, Aleksey! On Dec 18, Aleksey Midenkov wrote:
Sergei,
On Wed, Dec 18, 2019 at 1:11 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Dec 18, Aleksey Midenkov wrote:
On Wed, Dec 4, 2019 at 10:10 PM Sergei Golubchik wrote:
But it's better to be from the other side:
bool Lex_cstring::strdup(MEM_ROOT *mem_root, const Lex_cstring &src) { // allocate and deep-copy from src to this }
I'd really like to use such utility methods instead of C variants like thd_make_lex_string(). ... We better go away from this C service layer of thd_*() functions between server and plugins and use class methods instead.
Why is it better? Isn't it just the syntax sugar?
Having local class interfaces is easier to maintain. Additional API layer is development costs overhead.
That's neglectable, thd_make_lex_string() needs next to no maintainance.
It is good for version compatibility control, but we don't have third-party plugins, do we?
I don't know. There definitely were third-party storage engines, third-party fulltext parsers, etc. In fact, I'm sure there are third-party plugins that I know nothing about (besides the fact that they exist).
And we don't strictly use API to get server services into plugin, AFAIK.
This is automatic, any plugin that uses thd_make_lex_string() uses the "thd_alloc" service.
The only effect I can think of - pure C plugins won't be able to use thd_make_lex_string() if you replace it with a C++ method.
Do we have such plugins and do we need them to stay pure C?
No, not for thd_make_lex_string(). Of all the plugins I know of, only storage engines use it at the moment and they all are C++. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Sergei, On Wed, Dec 18, 2019 at 7:23 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
Sergei,
On Wed, Dec 18, 2019 at 1:11 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Dec 18, Aleksey Midenkov wrote:
On Wed, Dec 4, 2019 at 10:10 PM Sergei Golubchik wrote:
But it's better to be from the other side:
bool Lex_cstring::strdup(MEM_ROOT *mem_root, const Lex_cstring &src) { // allocate and deep-copy from src to this }
I'd really like to use such utility methods instead of C variants
On Dec 18, Aleksey Midenkov wrote: like
thd_make_lex_string(). ... We better go away from this C service layer of thd_*() functions between server and plugins and use class methods instead.
Why is it better? Isn't it just the syntax sugar?
Having local class interfaces is easier to maintain. Additional API layer is development costs overhead.
That's neglectable, thd_make_lex_string() needs next to no maintainance.
Particularly to this function I don't like its name, semantics and signature. They are over-complicated considering its frequent use. I could justify such function if it was used rarely, but when you have 8 calls of it subsequently -- it is ugly. And it takes time to check the signature because it is not clear what is NULL and what is TRUE. I mean you periodically have to check it and this is multiplied by infinite future, so yes, it takes time. Now, to the THD::make_clex_string() and THD::make_lex_string(). These methods should not be in THD at all. Its monolithic design with million of different methods looks to me as a huge mess accumulated across long time. There was no need to create proxies when there would not be such a large class in the first place.
It is good for version compatibility control, but we don't have
third-party plugins, do we?
I don't know. There definitely were third-party storage engines, third-party fulltext parsers, etc.
In fact, I'm sure there are third-party plugins that I know nothing about (besides the fact that they exist).
And we don't strictly use API to get server services into plugin, AFAIK.
This is automatic, any plugin that uses thd_make_lex_string() uses the "thd_alloc" service.
The only effect I can think of - pure C plugins won't be able to use thd_make_lex_string() if you replace it with a C++ method.
Do we have such plugins and do we need them to stay pure C?
No, not for thd_make_lex_string(). Of all the plugins I know of, only storage engines use it at the moment and they all are C++.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok
Hi, Aleksey! On Dec 18, Aleksey Midenkov wrote:
On Wed, Dec 18, 2019 at 7:23 PM Sergei Golubchik <serg@mariadb.org> wrote:
On Dec 18, Aleksey Midenkov wrote:
On Wed, Dec 18, 2019 at 1:11 PM Sergei Golubchik <serg@mariadb.org> wrote:
On Dec 18, Aleksey Midenkov wrote:
We better go away from this C service layer of thd_*() functions between server and plugins and use class methods instead.
Why is it better? Isn't it just the syntax sugar?
Having local class interfaces is easier to maintain. Additional API layer is development costs overhead.
That's neglectable, thd_make_lex_string() needs next to no maintainance.
Particularly to this function I don't like its name, semantics and signature.
As far as the service is concerned, "don't like" is too weak an argument, changing a service comes with a compatibility cost. But for the internal server use we can change thd->make_lex_string(), indeed.
They are over-complicated considering its frequent use. I could justify such function if it was used rarely, but when you have 8 calls of it subsequently -- it is ugly. And it takes time to check the signature because it is not clear what is NULL and what is TRUE. I mean you periodically have to check it and this is multiplied by infinite future, so yes, it takes time.
Yes, it could've been better, I agree. But see above.
Now, to the THD::make_clex_string() and THD::make_lex_string(). These methods should not be in THD at all. Its monolithic design with million of different methods looks to me as a huge mess accumulated across long time. There was no need to create proxies when there would not be such a large class in the first place.
They could be methods of a MEM_ROOT. One shouldn't need a complete THD to allocate memory from a memroot. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Sergei! On Thu, Dec 19, 2019 at 11:32 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Dec 18, Aleksey Midenkov wrote:
On Wed, Dec 18, 2019 at 7:23 PM Sergei Golubchik <serg@mariadb.org> wrote:
On Dec 18, Aleksey Midenkov wrote:
On Wed, Dec 18, 2019 at 1:11 PM Sergei Golubchik <serg@mariadb.org> wrote:
On Dec 18, Aleksey Midenkov wrote:
We better go away from this C service layer of thd_*() functions between server and plugins and use class methods instead.
Why is it better? Isn't it just the syntax sugar?
Having local class interfaces is easier to maintain. Additional API layer is development costs overhead.
That's neglectable, thd_make_lex_string() needs next to no maintainance.
Particularly to this function I don't like its name, semantics and signature.
As far as the service is concerned, "don't like" is too weak an argument, changing a service comes with a compatibility cost.
Of course, if there is compatibility issue. That's not the case for our in-tree plugins, I guess?
But for the internal server use we can change thd->make_lex_string(), indeed.
They are over-complicated considering its frequent use. I could justify such function if it was used rarely, but when you have 8 calls of it subsequently -- it is ugly. And it takes time to check the signature because it is not clear what is NULL and what is TRUE. I mean you periodically have to check it and this is multiplied by infinite future, so yes, it takes time.
Yes, it could've been better, I agree. But see above.
Now, to the THD::make_clex_string() and THD::make_lex_string(). These methods should not be in THD at all. Its monolithic design with million of different methods looks to me as a huge mess accumulated across long time. There was no need to create proxies when there would not be such a large class in the first place.
They could be methods of a MEM_ROOT. One shouldn't need a complete THD to allocate memory from a memroot.
I believe, that's not much better than THD method. THD, MEM_ROOT are generic classes, they should know nothing about classes they provide services for.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok
Hi, Aleksey! On Dec 22, Aleksey Midenkov wrote:
Particularly to this function I don't like its name, semantics and signature.
As far as the service is concerned, "don't like" is too weak an argument, changing a service comes with a compatibility cost.
Of course, if there is compatibility issue. That's not the case for our in-tree plugins, I guess?
You cannot base your decision on the belief that all plugins are in-tree. This API was created open for third-party plugins to use. So, we have to assume that third party plugins exist (and they do exist, check github, sourceforce, and jira).
Now, to the THD::make_clex_string() and THD::make_lex_string(). These methods should not be in THD at all. Its monolithic design with million of different methods looks to me as a huge mess accumulated across long time. There was no need to create proxies when there would not be such a large class in the first place.
They could be methods of a MEM_ROOT. One shouldn't need a complete THD to allocate memory from a memroot.
I believe, that's not much better than THD method. THD, MEM_ROOT are generic classes, they should know nothing about classes they provide services for.
okay Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Sergei! On Fri, Dec 27, 2019 at 8:22 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Dec 22, Aleksey Midenkov wrote:
Particularly to this function I don't like its name, semantics and signature.
As far as the service is concerned, "don't like" is too weak an argument, changing a service comes with a compatibility cost.
Of course, if there is compatibility issue. That's not the case for our in-tree plugins, I guess?
You cannot base your decision on the belief that all plugins are in-tree. This API was created open for third-party plugins to use. So, we have to assume that third party plugins exist (and they do exist, check github, sourceforce, and jira).
My concern is mostly about our plugins. Let the third-party plugins use that API, but our plugins are not restricted to it?
Now, to the THD::make_clex_string() and THD::make_lex_string(). These methods should not be in THD at all. Its monolithic design with million of different methods looks to me as a huge mess accumulated across long time. There was no need to create proxies when there would not be such a large class in the first place.
They could be methods of a MEM_ROOT. One shouldn't need a complete THD to allocate memory from a memroot.
I believe, that's not much better than THD method. THD, MEM_ROOT are generic classes, they should know nothing about classes they provide services for.
okay
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok
Hi, Aleksey! On Jan 01, Aleksey Midenkov wrote:
On Fri, Dec 27, 2019 at 8:22 PM Sergei Golubchik wrote:
On Dec 22, Aleksey Midenkov wrote:
My concern is mostly about our plugins. Let the third-party plugins use that API, but our plugins are not restricted to it?
Unfortunately it doesn't work that way. Third-party plugins use everything that our plugins are using, because that's what plugin authors are looking at and where they're copying from. Our plugins should be clean as much as possible, they set an example. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Sergei! On Wed, Jan 1, 2020 at 4:07 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Jan 01, Aleksey Midenkov wrote:
On Fri, Dec 27, 2019 at 8:22 PM Sergei Golubchik wrote:
On Dec 22, Aleksey Midenkov wrote:
My concern is mostly about our plugins. Let the third-party plugins use that API, but our plugins are not restricted to it?
Unfortunately it doesn't work that way. Third-party plugins use everything that our plugins are using, because that's what plugin authors are looking at and where they're copying from.
Our plugins should be clean as much as possible, they set an example.
Okay, but what is wrong having two APIs: C and C++? Let's start defining one.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok
Hi, Aleksey! On Jan 05, Aleksey Midenkov wrote:
On Wed, Jan 1, 2020 at 4:07 PM Sergei Golubchik <serg@mariadb.org> wrote:
On Jan 01, Aleksey Midenkov wrote:
On Fri, Dec 27, 2019 at 8:22 PM Sergei Golubchik wrote:
On Dec 22, Aleksey Midenkov wrote:
My concern is mostly about our plugins. Let the third-party plugins use that API, but our plugins are not restricted to it?
Unfortunately it doesn't work that way. Third-party plugins use everything that our plugins are using, because that's what plugin authors are looking at and where they're copying from.
Our plugins should be clean as much as possible, they set an example.
Okay, but what is wrong having two APIs: C and C++? Let's start defining one.
It means double the amount of APIs to learn for users and double the amount of APIs to maintain for us. But not double the usage, as users will either use C or C++ API, the total usage of both will be the same as the usage of C API only. So, benefits for plugins would be very slim, I don't see the point of duplicating the API in C++. So your methods can, perhaps, stay to be used internally, but let's keep plugins on public APIs. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Aleksey Midenkov -
Sergei Golubchik