Sergei, hallo. Thanks for your notes, their matters are covered in a new patch, I am about to publish.
Considering the fragment option keyword though (please read on below), > Hi, Andrei! > > On Sep 07, andrei.el...@pp.inet.fi wrote: >> revision-id: 9b453c1ebf47a20ea1436de3284f8810d0e64c4c >> (mariadb-10.1.34-8-g9b453c1ebf4) >> parent(s): c09a8b5b36edb494e2bcc93074c06e26cd9f2b92 >> author: Andrei Elkin >> committer: Andrei Elkin >> timestamp: 2018-09-07 20:36:16 +0300 >> message: >> >> MDEV-10963 Fragmented BINLOG query >> >> The problem was originally stated in >> http://bugs.mysql.com/bug.php?id=82212 >> The size of an base64-encoded Rows_log_event exceeds its >> vanilla byte representation in 4/3 times. >> When a binlogged event size is about 1GB mysqlbinlog generates >> a BINLOG query that can't be send out due to its size. >> >> It is fixed with fragmenting the BINLOG argument C-string into >> (approximate) halves when the base64 encoded event is over 1GB size. >> The mysqlbinlog in such case puts out >> >> SET @binlog_fragment_0='base64-encoded-fragment_0'; >> SET @binlog_fragment_1='base64-encoded-fragment_1'; >> BINLOG 2, 'binlog_fragment'; > > No, that's totally ridiculous. A statement that internally walks the > list of user variables and collects values from variables named > according to a specific pattern, seriously? > > Please, make it > > BINLOG CONCAT(@binlog_fragment_0, @binlog_fragment_1) > > that'll work with no questions asked, everybody understands what it > means. The parser doesn't need to accept an arbitrary expression here, > it'd be simpler and safer to hard-code the syntax as above. after some struggling with the parser I "succumbed" to chose BINLOG DEFRAGMENT(@binlog_fragment_0, @binlog_fragment_1) Parser was too cruel on me thinking of CONCAT '(' as a function_call_generic to conduct all those actions. And while BINLOG CONCAT(...) remained working, an ordinary SET @var=CONCAT(...) errored out wit wrong syntax. I hope the chosen compromise is still pretty much readable. > > A couple of minor generic comments below, but I've stopped reviewing, > because the code will change anyway. > >> to represent a big BINLOG 'base64-encoded-"total"'. >> Two more statements are composed to promptly release memory >> SET @binlog_fragment_0=NULL; >> SET @binlog_fragment_1=NULL; >> >> diff --git a/client/mysqlbinlog.cc b/client/mysqlbinlog.cc >> index 9753125dd67..f0689631cfb 100644 >> --- a/client/mysqlbinlog.cc >> +++ b/client/mysqlbinlog.cc >> @@ -56,7 +56,13 @@ Rpl_filter *binlog_filter= 0; >> >> #define BIN_LOG_HEADER_SIZE 4 >> #define PROBE_HEADER_LEN (EVENT_LEN_OFFSET+4) >> - >> +/* >> + 2 fragments can always represent near 1GB row-based base64-encoded event >> as >> + two strings each of size less than max(max_allowed_packet). >> + Bigger number of fragments does not safe from potential need to tune >> (increase) >> + @@max_allowed_packet before to process the fragments. So 2 is safe and >> enough. >> +*/ >> +#define BINLOG_ROWS_EVENT_ENCODED_FRAGMENTS 2 >> >> #define CLIENT_CAPABILITIES (CLIENT_LONG_PASSWORD | CLIENT_LONG_FLAG | >> CLIENT_LOCAL_FILES) >> >> @@ -71,6 +77,9 @@ ulong bytes_sent = 0L, bytes_received = 0L; >> ulong mysqld_net_retry_count = 10L; >> ulong open_files_limit; >> ulong opt_binlog_rows_event_max_size; >> +#ifndef DBUG_OFF >> +ulong opt_binlog_rows_event_max_encoded_size; >> +#endif > > Try to avoid unnecessary ifdefs. Here you could've defined the variable > unconditionally and suppressed the warning with __attribute__((unused)), > of you could've used it unconditionally as a splitting threshold below > and only hidden the command line option. A good point. > >> uint test_flags = 0; >> static uint opt_protocol= 0; >> static FILE *result_file; >> @@ -813,7 +822,14 @@ write_event_header_and_base64(Log_event *ev, FILE >> *result_file, >> >> /* Write header and base64 output to cache */ >> ev->print_header(head, print_event_info, FALSE); >> - ev->print_base64(body, print_event_info, FALSE); >> + >> + /* the assert states the only current use case for the function */ >> + DBUG_ASSERT(print_event_info->base64_output_mode == >> + BASE64_OUTPUT_ALWAYS); > > 1. don't document the obvious > 2. don't wrap the line if it fits in 80-char line > >> + >> + ev->print_base64(body, print_event_info, >> + print_event_info->base64_output_mode != >> + BASE64_OUTPUT_DECODE_ROWS); >> >> /* Read data from cache and write to result file */ >> if (copy_event_cache_to_file_and_reinit(head, result_file) || >> @@ -1472,6 +1490,15 @@ that may lead to an endless loop.", >> "This value must be a multiple of 256.", >> &opt_binlog_rows_event_max_size, &opt_binlog_rows_event_max_size, 0, >> GET_ULONG, REQUIRED_ARG, UINT_MAX, 256, ULONG_MAX, 0, 256, 0}, >> +#ifndef DBUG_OFF >> + {"binlog-row-event-max-encoded-size", 0, > > all debug command-line options must start with "debug-", that is, > either rename it to "debug-binlog-row-event-max-encoded-size" or > make it non-debug. > >> + "The maximum size of base64-encoded rows-event in one BINLOG >> pseudo-query " >> + "instance. When the computed actual size exceeds the limit " >> + "the BINLOG's argument string is fragmented in two.", >> + &opt_binlog_rows_event_max_encoded_size, >> + &opt_binlog_rows_event_max_encoded_size, 0, >> + GET_ULONG, REQUIRED_ARG, UINT_MAX/4, 256, ULONG_MAX, 0, 256, 0}, >> +#endif >> {"verify-binlog-checksum", 'c', "Verify checksum binlog events.", >> (uchar**) &opt_verify_binlog_checksum, (uchar**) >> &opt_verify_binlog_checksum, >> 0, GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0}, > > Regards, > Sergei > Chief Architect MariaDB > and secur...@mariadb.org Cheers, Andrei _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp