MDEV-15990 REPLACE on a precise-versioned table returns ER_DUP_ENTRY

[FooBarrior]

• [x] The Jira issue number for this PR is: MDEV-15990

Description

This PR attempts to handle several issues related to system versioned history collision handling.

First, the subject of MDEV-15990: REPLACE shouldn't end up with duplicate key error.

Secondly, the user should not experience lost history, when possible, if several updates on the row happened to applty at the same timestamp. This is a contrary to trx_id, where a collision can only happen in a single transaction, and shouldn't be preserved by design, see MDEV-15427.

In addition, don't save history with row_start>row_end. This conforms current historical row insetion behavior. Though, in production this may happen f.ex. after ntp time correction. Then we'd better want to update row_start=row_end and save the row. I'm open to discussion.

Release Notes

Fix REPLACE behavior on a TRX-ID versioned table, when a single row was rewritten more than once. Now this doesn't cause duplicate key error. Improve timestamp-based versioned history collision handling, when several changs of a single row happen at the same timestamp.

How can this PR be tested?

For 1 and 2 the test case is added to versioning.replace. For 3 the test is in versioning.misc.

Basing the PR against the correct MariaDB version

• [ ] This is a new feature and the PR is based against the latest MariaDB development branch.
• [x] This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

• [x] I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
• [x] For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

[montywi]

Hi!

On Sat, 25 May 2024, 18:42 'Nikita Malyavin' via Michael Widenius, < @.***> wrote:

@.**** commented on this pull request.

In sql/field.cc https://github.com/MariaDB/server/pull/3229#discussion_r1614556036:

@@ -4626,17 +4626,17 @@ void Field_longlong::set_max() int8store(ptr, unsigned_flag ? ULONGLONG_MAX : LONGLONG_MAX); }

-bool Field_longlong::is_max() +bool Field_longlong::is_max(const uchar *ptr_arg) const

I think the opposite: a pointer should ALWAYS be supplied. And it makes no sense to store ptr in Field (offset should be stored, rather). You already may have noticed, how hard it can be to make literally anything with a record that is not table->record[0]. Many things get broken because of that.

We should not change interfaces for how records are accessed just for one case and in an old MariaDB version. When we do that, we should do that systematic and fix all interfaces at the same time.

There are plans of how to do this, and the suggested change is not the way to do that.

Regards, Monty

[FooBarrior]

@montywi This approach of accessing the rocerds has already been presented in a number of Field methods:

int cmp(const uchar *a,const uchar *b) const;
int cmp_binary(const uchar *a,const uchar *b, uint32 max_length) const;
int cmp_prefix(const uchar *a, const uchar *b, size_t prefix_char_len) const;
int key_cmp(const uchar *,const uchar*) const override;
uchar *pack(uchar *to, const uchar *from, uint max_length);
const uchar *unpack(uchar* to, const uchar *from, const uchar *from_end, uint param_data);
void val_str_from_ptr(String *val, const uchar *ptr) const;

So that is not something new for the Field interface, but an organic continuation.

We should not change interfaces for how records are accessed just for one case and in an old MariaDB version. When we do that, we should do that systematic and fix all interfaces at the same time.

There are plans of how to do this, and the suggested change is not the way to do that.