Hello! On Tue, 3 Dec 2019 at 21:52, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita!
Hello, Sergei! I have updated the code, please see
- bb-10.5-MDEV-16978-without-overlaps < https://github.com/MariaDB/server/compare/bb-10.5-MDEV-16978-without-overlap...
branch.
Among the changes I, having noticed that innodb combination is missing, have added it, and some merging down from my INSERT/REPLACE patch was required to make it run successfully.
Below are the rest comments:
On Tue, 19 Nov 2019 at 15:45, Sergei Golubchik <serg@mariadb.org> wrote:
diff --git a/sql/handler.cc b/sql/handler.cc index 94cffd69b75..560f6316602 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -6913,6 +6928,130 @@ void handler::set_lock_type(enum
table->reginfo.lock_type= lock; }
+int handler::ha_check_overlaps(const uchar *old_data, const uchar* new_data) +{ + DBUG_ASSERT(new_data); + if (!table_share->period.unique_keys) + return 0; + if (table->versioned() && !table->vers_end_field()->is_max()) + return 0; + + bool is_update= old_data != NULL; + if (!check_overlaps_buffer) + check_overlaps_buffer= (uchar*)alloc_root(&table_share->mem_root, + table_share->max_unique_length + +
+ auto *record_buffer= check_overlaps_buffer +
On Nov 26, Nikita Malyavin wrote: thr_lock_type lock) table_share->reclength); table_share->max_unique_length;
+ auto *handler= this; + if (handler->inited != NONE) + { + if (!check_overlaps_handler) + { + check_overlaps_handler= clone(table_share->normalized_path.str, + &table_share->mem_root);
wouldn't it be simpler and cheaper to use HA_EXTRA_REMEMBER_POS and HA_EXTRA_RESTORE_POS, as we've discussed?
Can it stack (like, what'd be if HA_EXTRA_REMEMBER_POS is called several times)? I guess it can't, and somebody on upper layers could have already used it. This is for example done in TABLE::delete_row(). I see that you've removed it in latest 10.4+, but anyway, I think it's better to avoid remembering on lower layers
strictly speaking, cloning is rather a high-leveloperation, not for lower layers.
but I'm not as much interested in layer purity, as in the simpler and faster code. I thought that HA_EXTRA_REMEMBER_POS would be it - simpler and faster - but let me look and your new patch first, if I'll still think that I comment there again :)
+ int error= -1; + if (check_overlaps_handler != NULL) + error= check_overlaps_handler->ha_external_lock(table->in_use, F_RDLCK); + if (error) + return error; + } + handler= check_overlaps_handler; + }
why "if (handler->inited != NONE)" ? What happens if it is NONE?
then `this` is used
right, but when handler->inited is NONE? How can that happen?
When you just insert the record, for example. Neither rnd, nor index can be inited.
But it can be done better. You search for the key with the same value and a period start <= than the period start of new row. And then you have to check two rows for overlaps. If you'll search for a key with the period start <= than the _period end_ of the new row, then you'll only need to check one row (may be one more, if updating).
It can't work just that way: to handle case when period start = _period end_ of the new row, you should write even more checks, and then move cursor left.
No, I don't see that. Can you show an example where you'd need even more checks?
Ok, suppose you have to rows in table with periods: (b, c), (c, d).
Now you insert (a, c). ha_index_read_map will look for period_start <= c, so it will return (c, d). These rows do not overlap, but we yet do not know if (b, c) is there. So we need to go to the previous row. Now the algorithm looks same complex: read key, make some checks, got to the prev record.
Also, why do you store both period ends in the index? It seems like it'd be enough to store only one end - only the start or only the end. Both ends help if you use keyreads, but you don't. On the second thought, perhaps, you should use keyreads, there's no need to fetch the whole row for this overlap check. On the yet another thought it might not work with HA_EXTRA_REMEMBER_POS.
I think keyreads can work with HA_EXTRA_REMEMBER_POS. Moreover, I agree that using keyreads can be much more efficient. However, all of my latest code, namely FKs and REPLACE/IODKU, are not based on keyreads, and rewriting it now will require significant effort. Let's make it a separate task, maybe?
Why would it require significant effort? As far as I understand in your current code you only need to enable keyreads and that's all.
it'll return in different format, isn't it? You can't rely on key_part->field->ptr_in_record after that.
+ * a record we are updating. It means, that there are no overlaps + * from this side. */ + if (is_update && memcmp(old_data + table->s->null_bytes, + record_buffer + table->s->null_bytes, + table->s->reclength -
+ /* In case of update it could appear that the nearest neighbour is table->s->null_bytes) == 0)
+ { + continue; + }
I'd rather compare row positions here. What do you mean by that?
two rows are the same, if their "positions" are equal, not if their column values are equal. Also positions are much shorter to compare.
after ha_index_read_map or ha_index_next you do
handler->position(record_buffer)
and then you have a "position" stored in handler->ref, and it has the length of handler->ref_length bytes. For MyISAM it's usually the file offset, for InnoDB - PK value.
For UPDATE you can, I suppose, call this->position(old_data) to get the position.
It actually returns the ref for the last fetched row. the argument passed is not even used😐 . Only innodb uses it, and only for the primary key case.
+ if (thd->work_part_info)
+ { + my_error(ER_PERIOD_WITHOUT_OVERLAPS_PARTITIONED, MYF(0));
why?
Unfortunately partitions do not support searching upper/lower bounds (i.e. KEY_OR_PREV, KEY_OR_NEXT). But I think it is doable. For now it just doesn't work.
please add a comment about it here.
added
Regards,
Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- Yours truly, Nikita Malyavin