Re: [Maria-developers] 8a84f5a40c1: MDEV-24176 Preparations
Hi, Aleksey! On Oct 17, Aleksey Midenkov wrote:
commit 8a84f5a40c1 Author: Aleksey Midenkov <midenok@gmail.com> Date: Thu May 27 17:00:14 2021 +0300
MDEV-24176 Preparations
1. moved fix_vcol_exprs() call to open_table()
mysql_alter_table() doesn't do lock_tables() so it cannot win from fix_vcol_exprs() from there. Tests affected: main.default_session
This is likely wrong, but the old code was wrong too. Neither open_table nor lock_tables is called under LOCK TABLES, but the session environment can change, I suspect.
2. cleanup_excluding_fields_processor removed.
That was just a quick hack to exclude wrongly working Item_field from processing. Now it works due to correct execution environment (see next commit). Related to MDEV-10355
does it work now, in that commit, or only after the next commit? what exactly do you mean by correct execution environment?
3. Vanilla cleanups and comments.
diff --git a/sql/item.h b/sql/item.h index cc1914a7ad4..7b7fe04f0b2 100644 --- a/sql/item.h +++ b/sql/item.h @@ -2586,6 +2585,12 @@ class Item_ident :public Item_result_field const char *db_name; const char *table_name; const char *field_name; + /* + NOTE: came from TABLE::alias_name_used and this is only a hint! It works + only in need_correct_ident() condition. On other cases it is FALSE even if + table_name is alias! It cannot be TRUE in these cases, lots of spaghetti + logic depends on that.
could you elaborate on that?
+ */ bool alias_name_used; /* true if item was resolved against alias */ /* Cached value of index for this field in table->field array, used by prep.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei! On Sun, Oct 17, 2021 at 4:55 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Oct 17, Aleksey Midenkov wrote:
commit 8a84f5a40c1 Author: Aleksey Midenkov <midenok@gmail.com> Date: Thu May 27 17:00:14 2021 +0300
MDEV-24176 Preparations
1. moved fix_vcol_exprs() call to open_table()
mysql_alter_table() doesn't do lock_tables() so it cannot win from fix_vcol_exprs() from there. Tests affected: main.default_session
This is likely wrong, but the old code was wrong too. Neither open_table nor lock_tables is called under LOCK TABLES, but the session environment can change, I suspect.
Why, open_table() is called under LOCK TABLES. Please see there `if (best_table)` and a jump to reset where vcol_fix_exprs() is called. Added test case for LOCK TABLES.
2. cleanup_excluding_fields_processor removed.
That was just a quick hack to exclude wrongly working Item_field from processing. Now it works due to correct execution environment (see next commit). Related to MDEV-10355
does it work now, in that commit, or only after the next commit?
It is for the next commit actually. Moved that there.
what exactly do you mean by correct execution environment?
This is what Vcol_expr_context does.
3. Vanilla cleanups and comments.
diff --git a/sql/item.h b/sql/item.h index cc1914a7ad4..7b7fe04f0b2 100644 --- a/sql/item.h +++ b/sql/item.h @@ -2586,6 +2585,12 @@ class Item_ident :public Item_result_field const char *db_name; const char *table_name; const char *field_name; + /* + NOTE: came from TABLE::alias_name_used and this is only a hint! It works + only in need_correct_ident() condition. On other cases it is FALSE even if + table_name is alias! It cannot be TRUE in these cases, lots of spaghetti + logic depends on that.
could you elaborate on that?
See in next commit check_vcol_func_processor(). I could use `if (alias_name_used)` instead of `if (table_name)` if alias_name_used were updated correctly. But it is used only for limited subset of cases (so is not always true when alias is specified). Improving alias_name_used caused me some failures (in find_field_in_tables() AFAIR), so I had to mark VCOL_TABLE_ALIAS for every existing Item_ident::table_name. That triggers redundant expr update for some cases.
+ */ bool alias_name_used; /* true if item was resolved against alias */ /* Cached value of index for this field in table->field array, used by prep.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- @midenok
Hi, Aleksey! On Oct 18, Aleksey Midenkov wrote:
On Sun, Oct 17, 2021 at 4:55 PM Sergei Golubchik <serg@mariadb.org> wrote:
On Oct 17, Aleksey Midenkov wrote:
commit 8a84f5a40c1 Author: Aleksey Midenkov <midenok@gmail.com> Date: Thu May 27 17:00:14 2021 +0300
MDEV-24176 Preparations
1. moved fix_vcol_exprs() call to open_table()
mysql_alter_table() doesn't do lock_tables() so it cannot win from fix_vcol_exprs() from there. Tests affected: main.default_session
This is likely wrong, but the old code was wrong too. Neither open_table nor lock_tables is called under LOCK TABLES, but the session environment can change, I suspect.
Why, open_table() is called under LOCK TABLES. Please see there `if (best_table)` and a jump to reset where vcol_fix_exprs() is called. Added test case for LOCK TABLES.
Yes, you're right
3. Vanilla cleanups and comments.
diff --git a/sql/item.h b/sql/item.h index cc1914a7ad4..7b7fe04f0b2 100644 --- a/sql/item.h +++ b/sql/item.h @@ -2586,6 +2585,12 @@ class Item_ident :public Item_result_field const char *db_name; const char *table_name; const char *field_name; + /* + NOTE: came from TABLE::alias_name_used and this is only a hint! It works + only in need_correct_ident() condition. On other cases it is FALSE even if + table_name is alias! It cannot be TRUE in these cases, lots of spaghetti + logic depends on that.
could you elaborate on that?
See in next commit check_vcol_func_processor(). I could use `if (alias_name_used)` instead of `if (table_name)` if alias_name_used were updated correctly.
I see that. What I mean was, can you show an example? Say, a query that sets TABLE::alias_name_used incorrectly? I wasn't able to find a test case for that. Also, why do you even want to re-fix items when a table alias is used? Is it for MDEV-25672? But that's already fixed for half a year. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei! On Tue, Dec 28, 2021 at 12:03 AM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Oct 18, Aleksey Midenkov wrote:
On Sun, Oct 17, 2021 at 4:55 PM Sergei Golubchik <serg@mariadb.org> wrote:
On Oct 17, Aleksey Midenkov wrote:
commit 8a84f5a40c1 Author: Aleksey Midenkov <midenok@gmail.com> Date: Thu May 27 17:00:14 2021 +0300
MDEV-24176 Preparations
1. moved fix_vcol_exprs() call to open_table()
mysql_alter_table() doesn't do lock_tables() so it cannot win from fix_vcol_exprs() from there. Tests affected: main.default_session
This is likely wrong, but the old code was wrong too. Neither open_table nor lock_tables is called under LOCK TABLES, but the session environment can change, I suspect.
Why, open_table() is called under LOCK TABLES. Please see there `if (best_table)` and a jump to reset where vcol_fix_exprs() is called. Added test case for LOCK TABLES.
Yes, you're right
3. Vanilla cleanups and comments.
diff --git a/sql/item.h b/sql/item.h index cc1914a7ad4..7b7fe04f0b2 100644 --- a/sql/item.h +++ b/sql/item.h @@ -2586,6 +2585,12 @@ class Item_ident :public Item_result_field const char *db_name; const char *table_name; const char *field_name; + /* + NOTE: came from TABLE::alias_name_used and this is only a hint! It works + only in need_correct_ident() condition. On other cases it is FALSE even if + table_name is alias! It cannot be TRUE in these cases, lots of spaghetti + logic depends on that.
could you elaborate on that?
See in next commit check_vcol_func_processor(). I could use `if (alias_name_used)` instead of `if (table_name)` if alias_name_used were updated correctly.
I see that. What I mean was, can you show an example? Say, a query that sets TABLE::alias_name_used incorrectly? I wasn't able to find a test case for that.
That's the original test case of the task. It has alias_name_used false. Well, early I had more problems with that. Now I did the PoC fix again and I see no big problems with the tests (bb-10.2-midenok-tmp).
Also, why do you even want to re-fix items when a table alias is used? Is it for MDEV-25672? But that's already fixed for half a year.
Originally this fix was done before the pushed one. And do you really want to keep bad value in Item_field? Even if you avoid using it now someone surely will suffer from that after some new development or when new use cases happen. If there is a ready fix that eliminates such an accident in the future why don't you want to push it?
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- @midenok
Hi, Aleksey! On Dec 29, Aleksey Midenkov wrote:
+ /* + NOTE: came from TABLE::alias_name_used and this is only a hint! It works + only in need_correct_ident() condition. On other cases it is FALSE even if + table_name is alias! It cannot be TRUE in these cases, lots of spaghetti + logic depends on that.
could you elaborate on that? I see that. What I mean was, can you show an example? Say, a query that sets TABLE::alias_name_used incorrectly? I wasn't able to find a test case for that.
That's the original test case of the task. It has alias_name_used false. Well, early I had more problems with that. Now I did the PoC fix again and I see no big problems with the tests (bb-10.2-midenok-tmp).
Does it mean, you'll remove the comment (and workarounds, if any) in your branch?
Also, why do you even want to re-fix items when a table alias is used? Is it for MDEV-25672? But that's already fixed for half a year.
Originally this fix was done before the pushed one. And do you really want to keep bad value in Item_field? Even if you avoid using it now someone surely will suffer from that after some new development or when new use cases happen. If there is a ready fix that eliminates such an accident in the future why don't you want to push it?
I generally prefer not to do work that's not needed. That's called lazy :) If there's a value that's not used, ever, then I would suggest not to spend time cleaning it up. Re-preparing vcols after every DML that happened to use table aliases - that's not lazy, that's too much work. As far as MDEV-25672 is concerned - making ALTER TABLE to check that existing vcols refer to the correct table - that's too much work too, we already know they do. Only new columns need to be checked. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei, On Mon, Jan 3, 2022 at 2:44 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Dec 29, Aleksey Midenkov wrote:
+ /* + NOTE: came from TABLE::alias_name_used and this is only a hint! It works + only in need_correct_ident() condition. On other cases it is FALSE even if + table_name is alias! It cannot be TRUE in these cases, lots of spaghetti + logic depends on that.
could you elaborate on that? I see that. What I mean was, can you show an example? Say, a query that sets TABLE::alias_name_used incorrectly? I wasn't able to find a test case for that.
That's the original test case of the task. It has alias_name_used false. Well, early I had more problems with that. Now I did the PoC fix again and I see no big problems with the tests (bb-10.2-midenok-tmp).
Does it mean, you'll remove the comment (and workarounds, if any) in your branch?
Actually, no. That means it is possible to fix alias_name_used. I just removed "It cannot be TRUE in these cases, lots of spaghetti logic depends on that." But I now suspect if I go further in fixing, it will turn out to be also true.
Also, why do you even want to re-fix items when a table alias is used? Is it for MDEV-25672? But that's already fixed for half a year.
Originally this fix was done before the pushed one. And do you really want to keep bad value in Item_field? Even if you avoid using it now someone surely will suffer from that after some new development or when new use cases happen. If there is a ready fix that eliminates such an accident in the future why don't you want to push it?
I generally prefer not to do work that's not needed. That's called lazy :) If there's a value that's not used, ever, then I would suggest not to spend time cleaning it up. Re-preparing vcols after every DML that happened to use table aliases - that's not lazy, that's too much work.
Lazy update is the pattern to address specific performance issues. I don't believe it should be used everywhere or at every convenient occasion. I'm not really sure if Item_field was designed to keep dirty values. That at least deserves a comment.
As far as MDEV-25672 is concerned - making ALTER TABLE to check that existing vcols refer to the correct table - that's too much work too, we already know they do. Only new columns need to be checked.
Agree about new columns, but not about "much work". Performance optimization should be done where it does matter. If something is done per-row, that is always performance-sensitive. In the per-statement case we may or may not optimize, that depends on specific cases and trade-offs.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- @midenok
participants (2)
-
Aleksey Midenkov -
Sergei Golubchik