Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-23446

UPDATE does not insert history row if the row is not changed

Details

    Description

      UPDATE queries that do not change any values in the rows they match on versioned tables do not insert historical rows.

      Example:

      CREATE TABLE test (a INT) WITH SYSTEM VERSIONING;
      INSERT INTO test (a) VALUES (1);
      UPDATE test SET a = 1 WHERE a = 1;
      SELECT *, row_start, row_end FROM test FOR SYSTEM_TIME ALL;
      

      Disclaimer: I don't know whether this is a bug or was purposefully designed like this. In the latter case, I want to make a case to reconsider it:

      • This behavior contrasts with triggers: UPDATE queries that do not change any values in the rows they match still carry out registered triggers for each row.
      • Now some UPDATEs cannot be logged historically. Versioned table fail to capture that they happened. Yet logging such queries and the timestamps at which they were issued can still be valuable information.

      Attachments

        Issue Links

          Activity

            I will make urgent fix with the variable, plase fix it properly.

            sanja Oleksandr Byelkin added a comment - I will make urgent fix with the variable, plase fix it properly.

            midenok, I think that we must absolutely cover this code path in multi_update::send_data() with a test case:

                  if (has_vers_fields && table->versioned(VERS_TIMESTAMP))
                  {
                    store_record(table, record[2]);
                    if (vers_insert_history_row(table))
                    {
                      restore_record(table, record[2]);
                      goto error;
                    }
            

            This is causing an uninitialized variable to be used in another scope:

                  if (!can_compare_record || compare_record(table))
                  {
            	int error;
            …
                      {
            error:
                        /*
                          If (ignore && error == is ignorable) we don't have to
                          do anything; otherwise...
                        */
                        myf flags= 0;
             
                        if (table->file->is_fatal_error(error, HA_CHECK_ALL))
                          flags|= ME_FATAL; /* Other handler errors are fatal */
             
                        prepare_record_for_error_message(error, table);
                        table->file->print_error(error,MYF(flags));
                        DBUG_RETURN(1);
            

            If I understood it correctly, the fix that sanja is considering would only silence the GCC -Wmaybe-uninitialized in optimized builds, but we would use error=0 (no error) in that error handling code path, which feels wrong to me. Hence, we should make sure that this code path is covered by a test, and an appropriate error will be reported.

            marko Marko Mäkelä added a comment - midenok , I think that we must absolutely cover this code path in multi_update::send_data() with a test case: if (has_vers_fields && table->versioned(VERS_TIMESTAMP)) { store_record(table, record[2]); if (vers_insert_history_row(table)) { restore_record(table, record[2]); goto error; } This is causing an uninitialized variable to be used in another scope: if (!can_compare_record || compare_record(table)) { int error; … { error: /* If (ignore && error == is ignorable) we don't have to do anything; otherwise... */ myf flags= 0;   if (table->file->is_fatal_error(error, HA_CHECK_ALL)) flags|= ME_FATAL; /* Other handler errors are fatal */   prepare_record_for_error_message(error, table); table->file->print_error(error,MYF(flags)); DBUG_RETURN(1); If I understood it correctly, the fix that sanja is considering would only silence the GCC -Wmaybe-uninitialized in optimized builds, but we would use error=0 (no error) in that error handling code path, which feels wrong to me. Hence, we should make sure that this code path is covered by a test, and an appropriate error will be reported.

            188b328335d5c2a61d21f528fad19a685f9511ef

            sanja Oleksandr Byelkin added a comment - 188b328335d5c2a61d21f528fad19a685f9511ef

            marko Agree and already started that in MDEV-24451. Some errors are hard to trigger without DBUG_EXECUTE_IF() (like failure of insert history row). Do you have an idea how to fail row insert in release build?

            midenok Aleksey Midenkov added a comment - marko Agree and already started that in MDEV-24451 . Some errors are hard to trigger without DBUG_EXECUTE_IF() (like failure of insert history row). Do you have an idea how to fail row insert in release build?

            midenok, an insert should be able to fail on lock wait timeout. Maybe you could issue a locking read (SELECT…LOCK IN SHARE MODE) from another connection?

            marko Marko Mäkelä added a comment - midenok , an insert should be able to fail on lock wait timeout. Maybe you could issue a locking read ( SELECT…LOCK IN SHARE MODE ) from another connection?

            People

              midenok Aleksey Midenkov
              user2180613 Remy Fox
              Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Git Integration

                  Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.