maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Andrei!

Thanks! We're almost there.
just a couple of minor comments re. format strings, see below.

On Dec 18, andrei.elkin@xxxxxxxxxx wrote:
> revision-id: c3ef531aaff39d3262882213157d36a545b600dd (mariadb-10.1.37-31-gc3ef531aaff)
> parent(s): 541500295abdba7fa619065069291ac9c1dd6e83
> author: Andrei Elkin
> committer: Andrei Elkin
> timestamp: 2018-12-18 18:28:59 +0200
> message:
> 
> MDEV-10963 Fragmented BINLOG query
...
> diff --git a/sql/log_event.cc b/sql/log_event.cc
> --- a/sql/log_event.cc
> +++ b/sql/log_event.cc
> @@ -10491,12 +10505,128 @@ void Rows_log_event::pack_info(Protocol *protocol)
>  #endif
>  
>  #ifdef MYSQL_CLIENT
> +/**
> +  Print an event "body" cache to @c file possibly in two fragments.
> +  Each fragement is optionally per @c do_wrap to produce an SQL statement.
> +
> +  @param file      a file to print to
> +  @param body      the "body" IO_CACHE of event
> +  @param do_wrap   whether to wrap base64-encoded strings with
> +                   SQL cover.
> +  @param delimiter delimiter string
> +
> +  The function signals on any error through setting @c body->error to -1.
> +*/
> +void copy_cache_to_file_wrapped(FILE *file,
> +                                IO_CACHE *body,
> +                                bool do_wrap,
> +                                const char *delimiter)
> +{
> +  const char str_binlog[]= "\nBINLOG '\n";
> +  const char fmt_delim[]=     "'%s\n";
> +  const char fmt_n_delim[]= "\n'%s\n";
> +  const my_off_t cache_size= my_b_tell(body);
> +
> +  if (reinit_io_cache(body, READ_CACHE, 0L, FALSE, FALSE))
> +  {
> +    body->error= -1;
> +    goto end;
> +  }
> +
> +  if (!do_wrap)
> +  {
> +    my_b_copy_to_file(body, file, SIZE_T_MAX);
> +  }
> +  else if (4 + sizeof(str_binlog) + cache_size + sizeof(fmt_delim) >
> +           opt_binlog_rows_event_max_encoded_size)
> +  {
> +    /*
> +      2 fragments can always represent near 1GB row-based
> +      base64-encoded event as two strings each of size less than
> +      max(max_allowed_packet). Greater number of fragments does not
> +      save from potential need to tweak (increase) @@max_allowed_packet
> +      before to process the fragments. So 2 is safe and enough.
> +
> +      Split the big query when its packet size's estimation exceeds a
> +      limit. The estimate includes the maximum packet header
> +      contribution of non-compressed packet.
> +    */
> +    const char fmt_frag[]= "%sSET @binlog_fragment_%d ='\n";
> +
> +    my_fprintf(file, fmt_frag, "\n", 0);
> +    if (my_b_copy_to_file(body, file, cache_size/2 + 1))
> +    {
> +      body->error= -1;
> +      goto end;
> +    }
> +    my_fprintf(file, fmt_n_delim, delimiter);

see, you use fmt_n_delim only once. here.
but you end it with \n. And it forced you to start fmt_frag with %s.

please, remove \n from the end of fmt_n_delim and add it to the beginning
of fmt_frag

> +
> +    my_fprintf(file, fmt_frag, "", 1);
> +    if (my_b_copy_to_file(body, file, SIZE_T_MAX))
> +    {
> +      body->error= -1;
> +      goto end;
> +    }
> +    my_fprintf(file, fmt_delim, delimiter);

and here - no \n before ' ?
I've got lost in all these new lines and format constants.
can you please show what the binlog dump will look like?

> +
> +    my_fprintf(file, "BINLOG @binlog_fragment_%d, @binlog_fragment_%d%s\n",
> +               0, 1, delimiter);

don't pass 0 and 1 as parameters, it's not like they can ever change :)

> +  }
> +  else
> +  {
> +    my_fprintf(file, str_binlog);
> +    if (my_b_copy_to_file(body, file, SIZE_T_MAX))
> +    {
> +      body->error= -1;
> +      goto end;
> +    }
> +    my_fprintf(file, fmt_delim, delimiter);
> +  }
> +  reinit_io_cache(body, WRITE_CACHE, 0, FALSE, TRUE);
> +
> +end:
> +  return;
> +}
> +
> +/**
> +  The function invokes base64 encoder to run on the current
> +  event string and store the result into two caches.
> +  When the event ends the current statement the caches are is copied into
> +  the argument file.
> +  Copying is also concerned how to wrap the event, specifically to produce
> +  a valid SQL syntax.
> +  When the encoded data size is within max(MAX_ALLOWED_PACKET)
> +  a regular BINLOG query is composed. Otherwise it is build as fragmented
> +
> +    SET @binlog_fragment_0='...';
> +    SET @binlog_fragment_1='...';
> +    BINLOG @binlog_fragment_0, @binlog_fragment_1;
> +
> +  where fragments are represented by a pair of indexed user
> +  "one shot" variables.
> +
> +  NOTE.

still @note

> +  If any changes made don't forget to duplicate them to
> +  Old_rows_log_event as long as it's supported.
> +
> +  @param file               pointer to IO_CACHE
> +  @param print_event_info   pointer to print_event_info specializing
> +                            what out of and how to print the event
> +  @param name               the name of a table that the event operates on
> +
> +  The function signals on any error of cache access through setting
> +  that cache's @c error to -1.
> +*/
>  void Rows_log_event::print_helper(FILE *file,
>                                    PRINT_EVENT_INFO *print_event_info,
>                                    char const *const name)
>  {
>    IO_CACHE *const head= &print_event_info->head_cache;
>    IO_CACHE *const body= &print_event_info->body_cache;
> +  bool do_print_encoded=
> +    print_event_info->base64_output_mode != BASE64_OUTPUT_DECODE_ROWS &&
> +    !print_event_info->short_form;
> +
>    if (!print_event_info->short_form)
>    {
>      bool const last_stmt_event= get_flags(STMT_END_F);

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx