Re: [Maria-developers] 74b2eba1ca6: MDEV-15458 Segfault in heap_scan() upon UPDATE after ADD SYSTEM VERSIONING
Hi, Aleksey! On Mar 26, Aleksey Midenkov wrote:
revision-id: 74b2eba1ca6 (mariadb-10.3.12-115-g74b2eba1ca6) parent(s): 2c0901d808b author: Aleksey Midenkov <midenok@gmail.com> committer: Sergei Golubchik <serg@mariadb.com> timestamp: 2019-03-26 17:43:38 +0100 message:
MDEV-15458 Segfault in heap_scan() upon UPDATE after ADD SYSTEM VERSIONING
* HA_EXTRA_REMEMBER_POS, HA_EXTRA_RESTORE_POS for HEAP storage engine * Versioning tests support
diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index abe4254db3e..1d51947cc86 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -1643,7 +1643,12 @@ int vers_insert_history_row(TABLE *table) if (row_start->cmp(row_start->ptr, row_end->ptr) >= 0) return 0;
- return table->file->ha_write_row(table->record[0]); + int res; + if ((res= table->file->extra(HA_EXTRA_REMEMBER_POS))) + return res; + if ((res= table->file->ha_write_row(table->record[0]))) + return res; + return table->file->extra(HA_EXTRA_RESTORE_POS); }
Frankly, I don't like it. I mean, this particular fix is fine, but here's the problem: We now have (at least) three features where the server might need to do something (insert/update/search) during the index/rnd scan. And three different solutions: you use HA_EXTRA_REMEMBER_POS in system versioning, Nikita simply sets delete_while_scanning=false in application time period tables, and Sachin uses handler::clone() to create a second handler that can be used to check for hash collisions without disrupting the scan on the primary handler. I think it's getting somewhat out of hands. Can we please reduce the number of approaches to fix the same issue? Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Sergei! On Wed, Mar 27, 2019 at 1:04 AM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Mar 26, Aleksey Midenkov wrote:
revision-id: 74b2eba1ca6 (mariadb-10.3.12-115-g74b2eba1ca6) parent(s): 2c0901d808b author: Aleksey Midenkov <midenok@gmail.com> committer: Sergei Golubchik <serg@mariadb.com> timestamp: 2019-03-26 17:43:38 +0100 message:
MDEV-15458 Segfault in heap_scan() upon UPDATE after ADD SYSTEM VERSIONING
* HA_EXTRA_REMEMBER_POS, HA_EXTRA_RESTORE_POS for HEAP storage engine * Versioning tests support
diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index abe4254db3e..1d51947cc86 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -1643,7 +1643,12 @@ int vers_insert_history_row(TABLE *table) if (row_start->cmp(row_start->ptr, row_end->ptr) >= 0) return 0;
- return table->file->ha_write_row(table->record[0]); + int res; + if ((res= table->file->extra(HA_EXTRA_REMEMBER_POS))) + return res; + if ((res= table->file->ha_write_row(table->record[0]))) + return res; + return table->file->extra(HA_EXTRA_RESTORE_POS); }
Frankly, I don't like it. I mean, this particular fix is fine, but here's the problem:
We now have (at least) three features where the server might need to do something (insert/update/search) during the index/rnd scan.
And three different solutions: you use HA_EXTRA_REMEMBER_POS in system versioning, Nikita simply sets delete_while_scanning=false in application time period tables, and Sachin uses handler::clone() to create a second handler that can be used to check for hash collisions without disrupting the scan on the primary handler.
I think it's getting somewhat out of hands. Can we please reduce the number of approaches to fix the same issue?
What solution do you propose to use?
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok
I personally like this one more then mine. BTW I also clone the handler to do something similar in WITHOUT OVERLAPS. Maybe I should also consider to rewrite it to HA_EXTRA_REMEMBER_POS On Wed, 27 Mar 2019 at 20:37, Aleksey Midenkov <midenok@gmail.com> wrote:
Hi, Sergei!
On Wed, Mar 27, 2019 at 1:04 AM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Mar 26, Aleksey Midenkov wrote:
revision-id: 74b2eba1ca6 (mariadb-10.3.12-115-g74b2eba1ca6) parent(s): 2c0901d808b author: Aleksey Midenkov <midenok@gmail.com> committer: Sergei Golubchik <serg@mariadb.com> timestamp: 2019-03-26 17:43:38 +0100 message:
MDEV-15458 Segfault in heap_scan() upon UPDATE after ADD SYSTEM VERSIONING
* HA_EXTRA_REMEMBER_POS, HA_EXTRA_RESTORE_POS for HEAP storage engine * Versioning tests support
diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index abe4254db3e..1d51947cc86 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -1643,7 +1643,12 @@ int vers_insert_history_row(TABLE *table) if (row_start->cmp(row_start->ptr, row_end->ptr) >= 0) return 0;
- return table->file->ha_write_row(table->record[0]); + int res; + if ((res= table->file->extra(HA_EXTRA_REMEMBER_POS))) + return res; + if ((res= table->file->ha_write_row(table->record[0]))) + return res; + return table->file->extra(HA_EXTRA_RESTORE_POS); }
Frankly, I don't like it. I mean, this particular fix is fine, but here's the problem:
We now have (at least) three features where the server might need to do something (insert/update/search) during the index/rnd scan.
And three different solutions: you use HA_EXTRA_REMEMBER_POS in system versioning, Nikita simply sets delete_while_scanning=false in application time period tables, and Sachin uses handler::clone() to create a second handler that can be used to check for hash collisions without disrupting the scan on the primary handler.
I think it's getting somewhat out of hands. Can we please reduce the number of approaches to fix the same issue?
What solution do you propose to use?
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
-- All the best,
Aleksey Midenkov @midenok
-- Yours truly, Nikita Malyavin
Hi, Aleksey! It seems that everyone prefers your approach :) I also think this it's easier to use and it's a more lightweight than the clone. A clone is justified when one wants to do two scans in parallel (like, read few rows from one handler, read few rows from the clone, than from the first handler, and so on). Which is not the case here, so the clone looks like an overkill. The only problem here - not all engines might implement HA_EXTRA_REMEMBER_POS. But it looks like it's generally easy to implement. On Mar 27, Aleksey Midenkov wrote:
- return table->file->ha_write_row(table->record[0]); + int res; + if ((res= table->file->extra(HA_EXTRA_REMEMBER_POS))) + return res; + if ((res= table->file->ha_write_row(table->record[0]))) + return res; + return table->file->extra(HA_EXTRA_RESTORE_POS); }
Frankly, I don't like it. I mean, this particular fix is fine, but here's the problem:
We now have (at least) three features where the server might need to do something (insert/update/search) during the index/rnd scan.
And three different solutions: you use HA_EXTRA_REMEMBER_POS in system versioning, Nikita simply sets delete_while_scanning=false in application time period tables, and Sachin uses handler::clone() to create a second handler that can be used to check for hash collisions without disrupting the scan on the primary handler.
I think it's getting somewhat out of hands. Can we please reduce the number of approaches to fix the same issue?
What solution do you propose to use?
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Serg! On Tue, Mar 26, 2019 at 10:48 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Mar 26, Aleksey Midenkov wrote:
revision-id: 74b2eba1ca6 (mariadb-10.3.12-115-g74b2eba1ca6) parent(s): 2c0901d808b author: Aleksey Midenkov <midenok@gmail.com> committer: Sergei Golubchik <serg@mariadb.com> timestamp: 2019-03-26 17:43:38 +0100 message:
MDEV-15458 Segfault in heap_scan() upon UPDATE after ADD SYSTEM VERSIONING
* HA_EXTRA_REMEMBER_POS, HA_EXTRA_RESTORE_POS for HEAP storage engine * Versioning tests support
diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index abe4254db3e..1d51947cc86 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -1643,7 +1643,12 @@ int vers_insert_history_row(TABLE *table) if (row_start->cmp(row_start->ptr, row_end->ptr) >= 0) return 0;
- return table->file->ha_write_row(table->record[0]); + int res; + if ((res= table->file->extra(HA_EXTRA_REMEMBER_POS))) + return res; + if ((res= table->file->ha_write_row(table->record[0]))) + return res; + return table->file->extra(HA_EXTRA_RESTORE_POS); }
Frankly, I don't like it. I mean, this particular fix is fine, but here's the problem:
We now have (at least) three features where the server might need to do something (insert/update/search) during the index/rnd scan.
And three different solutions: you use HA_EXTRA_REMEMBER_POS in system versioning, Nikita simply sets delete_while_scanning=false in application time period tables, and Sachin uses handler::clone() to create a second handler that can be used to check for hash collisions without disrupting the scan on the primary handler.
Actually I liked the Aleksey approach it is much easier then cloning and I don't have to free handler in close table Although I tried with long unique update I was getting assert fail in ha_index_read_map DBUG_ASSERT(inited==INDEX); Can we do (different )index scan after table->file->extra(HA_EXTRA_REMEMBER_POS) ?
I think it's getting somewhat out of hands. Can we please reduce the number of approaches to fix the same issue?
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
participants (4)
-
Aleksey Midenkov -
Nikita Malyavin -
sachin setiya -
Sergei Golubchik