Details

    Description

      MySQL 5.6 added the RANDOM_BYTES function: https://dev.mysql.com/doc/refman/5.6/en/encryption-functions.html#function_random-bytes

      This is needed for compatibility purposes.

      Attachments

        Issue Links

          Activity

            danblack Daniel Black added a comment -

            https://github.com/MariaDB/server/pull/2016 that serg looked over and I'm pretty sure every comment was addressed.

            I'll look at rebasing (10.9) and squashing commits once the release is over.

            danblack Daniel Black added a comment - https://github.com/MariaDB/server/pull/2016 that serg looked over and I'm pretty sure every comment was addressed. I'll look at rebasing (10.9) and squashing commits once the release is over.
            danblack Daniel Black added a comment -

            Pushed as preview-10.10-MDEV-25704-random_bytes

            This is identical to the updated PR2016.

            danblack Daniel Black added a comment - Pushed as preview-10.10- MDEV-25704 -random_bytes This is identical to the updated PR2016.

            It's in this branch: preview-10.10-misc.

            serg Sergei Golubchik added a comment - It's in this branch: preview-10.10-misc .

            also in bb-10.10-MDEV-25704 without other preview features

            serg Sergei Golubchik added a comment - also in bb-10.10- MDEV-25704 without other preview features

            In my opinion, the feature as of bb-10.10-MDEV-25704 bb1469017 can be pushed into 10.10 main and released with 10.10.1.

            Note: The remark about NULL-ability of a resulting column upon CREATE TABLE .. SELECT RANDOM_BYTES(N), repeated twice in the pull request discussion, has not been addressed. I assume it was a conscious decision to keep it in line with existing functions, specifically REPEAT mentioned as a counter-argument.

            elenst Elena Stepanova added a comment - In my opinion, the feature as of bb-10.10- MDEV-25704 bb1469017 can be pushed into 10.10 main and released with 10.10.1. Note: The remark about NULL-ability of a resulting column upon CREATE TABLE .. SELECT RANDOM_BYTES(N) , repeated twice in the pull request discussion , has not been addressed. I assume it was a conscious decision to keep it in line with existing functions, specifically REPEAT mentioned as a counter-argument.
            serg Sergei Golubchik added a comment - - edited

            nikitamalyavin, let's revive this NULL-ability question. You wrote (I'll quote from the PR to move the discussion here):

            I've checked several Item_str_func descendants – they all call set_maybe_null, like the Item_func_repeat mentioned in the related comment on the test result.
            I suggest to remain consistent – if we change this behavior, then we change it everywhere

            Indeed, Item_func_repeat always is always nullable. Because, see Item_func_repeat::val_str() it returns NULL when length of the result is larger than the max_allowed_packet. That is, REPEAT() can return NULL when both arguments are not NULL.

            As far as I can see, RANDOM_BYTES(N) cannot return NULL when its argument is not NULL.

            serg Sergei Golubchik added a comment - - edited nikitamalyavin , let's revive this NULL-ability question. You wrote (I'll quote from the PR to move the discussion here): I've checked several Item_str_func descendants – they all call set_maybe_null , like the Item_func_repeat mentioned in the related comment on the test result. I suggest to remain consistent – if we change this behavior, then we change it everywhere Indeed, Item_func_repeat always is always nullable. Because, see Item_func_repeat::val_str() it returns NULL when length of the result is larger than the max_allowed_packet . That is, REPEAT() can return NULL when both arguments are not NULL. As far as I can see, RANDOM_BYTES(N) cannot return NULL when its argument is not NULL.

            serg I didn't know about that variable. Shouldn't RANDOM_BYTES do the same?
            BTW sohuldn't the same apply to sql_mode=EMPTY_STRING_IS_NULL, or is it handled somewhere separately, too?

            nikitamalyavin Nikita Malyavin added a comment - serg I didn't know about that variable. Shouldn't RANDOM_BYTES do the same? BTW sohuldn't the same apply to sql_mode=EMPTY_STRING_IS_NULL, or is it handled somewhere separately, too?

            N in RANDOM_BYTES(N) must be between 1 and 1024, so RANDOM_BYTES can never return an empty string or anything larger than max_allowed_packet (which, itself, cannot be less than 1024)

            serg Sergei Golubchik added a comment - N in RANDOM_BYTES(N) must be between 1 and 1024, so RANDOM_BYTES can never return an empty string or anything larger than max_allowed_packet (which, itself, cannot be less than 1024)

            Right... sorry, i didn't check Item_func_random_bytes::val_str before asking, only RAND_bytes function description, since I remembered something about the length restriction, but nothing was there.

            So, why "if (length > 1024) then NULL", and "if (length > max_allowed_packet) then NULL" should behave differently?

            EDIT: sorry, got the difference. One is empty, and anothe one is NULL.

            nikitamalyavin Nikita Malyavin added a comment - Right... sorry, i didn't check Item_func_random_bytes::val_str before asking, only RAND_bytes function description, since I remembered something about the length restriction, but nothing was there. So, why "if (length > 1024) then NULL", and "if (length > max_allowed_packet) then NULL" should behave differently? EDIT: sorry, got the difference. One is empty, and anothe one is NULL.

            Ok, so now I remember my thoughts around sql_mode=EMPTY_STRING_IS_NULL.

            It is set in oracle mode, I guess it means that RANDOM_BYTES(1025) is NULL as well as empty in mysql (or is this mode about Oracle database, not mysql?)

            We can either stick with current behavior, but change fix_length_and_dec to:

            if (thd->variables.sql_mode & MODE_EMPTY_STRING_IS_NULL)
              set_maybe_null()
            

            or change it to return NULL, which is, maybe, more conformant with our style (i guess)

            nikitamalyavin Nikita Malyavin added a comment - Ok, so now I remember my thoughts around sql_mode=EMPTY_STRING_IS_NULL. It is set in oracle mode, I guess it means that RANDOM_BYTES(1025) is NULL as well as empty in mysql (or is this mode about Oracle database, not mysql?) We can either stick with current behavior, but change fix_length_and_dec to: if (thd->variables.sql_mode & MODE_EMPTY_STRING_IS_NULL) set_maybe_null() or change it to return NULL, which is, maybe, more conformant with our style (i guess)

            I suggest:

            • allow RANDOM_BYTES(0) and return an empty string. Why should this be disabled?
            • return NULL for N > 1024, that's more in line with what other functions do for invalid argument values.

            of course, the function should stay nullable, if we do the above.

            serg Sergei Golubchik added a comment - I suggest: allow RANDOM_BYTES(0) and return an empty string. Why should this be disabled? return NULL for N > 1024, that's more in line with what other functions do for invalid argument values. of course, the function should stay nullable, if we do the above.
            danblack Daniel Black added a comment -

            As suggested, I implemented this at the end of bb-10.10-MDEV-25704 and tested for mysql 8,0 compatibility. select random_bytes(0) is our extension. Note the fixme on TIME conversion in the test. Suggestions on implementing this much appreciated.

            danblack Daniel Black added a comment - As suggested, I implemented this at the end of bb-10.10- MDEV-25704 and tested for mysql 8,0 compatibility. select random_bytes(0) is our extension. Note the fixme on TIME conversion in the test. Suggestions on implementing this much appreciated.

            bb-10.10-MDEV-25704 as of ce1c50665 is NOT OK to push into the main tree, it fails in buildbot and otherwise.

            elenst Elena Stepanova added a comment - bb-10.10- MDEV-25704 as of ce1c50665 is NOT OK to push into the main tree, it fails in buildbot and otherwise.
            danblack Daniel Black added a comment - - edited

            fixed most. Debug builds only of query_cache test failing because args[0]->can_eval_in_optimize() changes from false to true between Item_func_random_bytes::fix_length_and_dec and Item_func_random_bytes::val_str. This is due to args[0]->table->const_table becoming true.

            danblack Daniel Black added a comment - - edited fixed most. Debug builds only of query_cache test failing because args[0]->can_eval_in_optimize() changes from false to true between Item_func_random_bytes::fix_length_and_dec and Item_func_random_bytes::val_str . This is due to args[0]->table->const_table becoming true.
            danblack Daniel Black added a comment -

            Ok, buildbot happy again. serg, nikitamalyavin can I get a review on the last two commits on the bb-10.10-MDEV-25704 branch.

            danblack Daniel Black added a comment - Ok, buildbot happy again. serg , nikitamalyavin can I get a review on the last two commits on the bb-10.10- MDEV-25704 branch.

            I have no further objections against pushing bb-10.10-MDEV-25704 as of 7f422495 into 10.10 main and releasing it with 10.10.1.

            elenst Elena Stepanova added a comment - I have no further objections against pushing bb-10.10- MDEV-25704 as of 7f422495 into 10.10 main and releasing it with 10.10.1.

            People

              serg Sergei Golubchik
              greenman Ian Gilfillan
              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.