Re: [Maria-developers] 7198e7c71fc: MDEV-15458 Segfault in heap_scan() upon UPDATE after ADD SYSTEM VERSIONING
Hi, Aleksey! On Dec 23, Aleksey Midenkov wrote:
revision-id: 7198e7c71fc (versioning-1.0.6-82-g7198e7c71fc) parent(s): 6be155757b7 author: Aleksey Midenkov
committer: Aleksey Midenkov timestamp: 2018-12-20 13:25:00 +0300 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
As far as I can see, there are two approaches to fixing this bug. And if we go the way of HA_EXTRA_REMEMBER_POS/HA_EXTRA_RESTORE_POS then your fix is good. But I'd still like to understand whether write_row is supposed to disturb the scan. Why should heap_write() overwrite info->current_ptr=pos and info->current_hash_ptr? Inserting a row doesn't happen at the "current position", so I don't quite understand why it modifies current_ptr at all. There is no functionality like "continue the scan from the last inserted row", right? It just doesn't make sense to me. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi!
On Tue, Dec 25, 2018 at 1:27 PM Sergei Golubchik
Hi, Aleksey!
On Dec 23, Aleksey Midenkov wrote:
revision-id: 7198e7c71fc (versioning-1.0.6-82-g7198e7c71fc) parent(s): 6be155757b7 author: Aleksey Midenkov
committer: Aleksey Midenkov timestamp: 2018-12-20 13:25:00 +0300 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
As far as I can see, there are two approaches to fixing this bug. And if we go the way of HA_EXTRA_REMEMBER_POS/HA_EXTRA_RESTORE_POS then your fix is good.
But I'd still like to understand whether write_row is supposed to disturb the scan. Why should heap_write() overwrite info->current_ptr=pos and info->current_hash_ptr? Inserting a row doesn't happen at the "current position", so I don't quite understand why it modifies current_ptr at all. There is no functionality like "continue the scan from the last inserted row", right? It just doesn't make sense to me.
I don't know why it works like this, maybe to keep consistency at API level: we insert record and then we able to access it without search. HEAP storage was copypasted from MyISAM where `lastpos` analogy also is updated at mi_write(). Without updating `current_ptr` and `lastpos` tests don't fail (main, funcs_1, heap). But I can't say whether the performance is affected and how it will go in production.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok
Hi, Aleksey! On Dec 27, Aleksey Midenkov wrote:
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
As far as I can see, there are two approaches to fixing this bug. And if we go the way of HA_EXTRA_REMEMBER_POS/HA_EXTRA_RESTORE_POS then your fix is good.
But I'd still like to understand whether write_row is supposed to disturb the scan. Why should heap_write() overwrite info->current_ptr=pos and info->current_hash_ptr? Inserting a row doesn't happen at the "current position", so I don't quite understand why it modifies current_ptr at all. There is no functionality like "continue the scan from the last inserted row", right? It just doesn't make sense to me.
I don't know why it works like this, maybe to keep consistency at API level: we insert record and then we able to access it without search. HEAP storage was copypasted from MyISAM where `lastpos` analogy also is updated at mi_write(). Without updating `current_ptr` and `lastpos` tests don't fail (main, funcs_1, heap). But I can't say whether the performance is affected and how it will go in production.
Thanks for the idea. I've tried to fix handler::write_row to not reset the scan. This, indeed, worked. I've also asked Monty and he also was of the opinion that handler::write_row should not reset the scan. I've pushed into a stage tree a fix with your test cases. Let's see how it'll go. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
participants (2)
-
Aleksey Midenkov
-
Sergei Golubchik