Hi, Alexey! Please, see few comments below. On Dec 07, Alexey Botchkov wrote:

revision-id: b610986c9c226e9b9cbe7c80ceaead1de0a55535 (mariadb-10.2.11-26-gb610986) parent(s): 4d016e6ed2d32298977a66e125cbfcae39e23847 committer: Alexey Botchkov timestamp: 2017-12-07 23:49:12 +0400 message:

MDEV-14593 human-readable XA RECOVER.

diff --git a/sql/handler.cc b/sql/handler.cc index 7eed722..128721c 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -1978,6 +1978,97 @@ int ha_recover(HASH *commit_list) }

/** + return the XID as it appears in the SQL function's arguments. + So this string can be passed to XA START, XA PREPARE etc... + + @note + the 'buf' has to have space for at least SQL_XIDSIZE bytes. +*/ + + +/* + 'a'..'z' 'A'..'Z', '0'..'9' + and '-' '_' ' ' symbols don't have to be + converted.

I'd say it's overcomplication, why not to convert everything to hex?

+*/ + +static const char xid_needs_conv[128]= +{ + 1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1, + 1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1, + 0,1,1,1,1,1,1,1,1,1,1,1,1,0,1,1, + 0,0,0,0,0,0,0,0,0,0,1,1,1,1,1,1, + 1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0, + 0,0,0,0,0,0,0,0,0,0,0,1,1,1,1,0, + 1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0, + 0,0,0,0,0,0,0,0,0,0,0,1,1,1,1,1 +}; + +uint get_sql_xid(XID *xid, char *buf) +{ + int tot_len= xid->gtrid_length + xid->bqual_length; + int i; + const char *orig_buf= buf; + + for (i=0; i<tot_len; i++) + { + uchar c= ((uchar *) xid->data)[i] + if (c >= 128 || xid_needs_conv[c]) + break; + } + + if (i >= tot_len) + { + /* No need to convert characters to hexadecimals. */ + *buf++= '\''; + memcpy(buf, xid->data, xid->gtrid_length); + buf+= xid->gtrid_length; + *buf++= '\''; + if (xid->bqual_length > 0 || xid->formatID != 1) + { + *buf++= ','; + *buf++= '\''; + memcpy(buf, xid->data+xid->gtrid_length, xid->bqual_length); + buf+= xid->bqual_length; + *buf++= '\''; + } + } + else + { + *buf++= 'X'; + *buf++= '\''; + for (i= 0; i < xid->gtrid_length; i++) + { + *buf++=_dig_vec_lower[((uchar*) xid->data)[i] >> 4]; + *buf++=_dig_vec_lower[((uchar*) xid->data)[i] & 0x0f]; + } + *buf++= '\''; + if (xid->bqual_length > 0 || xid->formatID != 1) + { + *buf++= ','; + *buf++= 'X'; + *buf++= '\''; + for (; i < tot_len; i++) + { + *buf++=_dig_vec_lower[((uchar*) xid->data)[i] >> 4]; + *buf++=_dig_vec_lower[((uchar*) xid->data)[i] & 0x0f]; + } + *buf++= '\''; + } + } + + if (xid->formatID != 1) + { + *buf++= ','; + buf+= my_longlong10_to_str_8bit(&my_charset_bin, buf, + MY_INT64_NUM_DECIMAL_DIGITS, -10, xid->formatID); + } + + return buf - orig_buf; +} + + +/** return the list of XID's to a client, the same way SHOW commands do.

@note diff --git a/sql/handler.h b/sql/handler.h index 486ba56..91e3168 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -628,6 +628,16 @@ struct xid_t { }; typedef struct xid_t XID;

+/* + The size of XID string representation in the form + 'gtrid', 'bqual', formatID + see xid_t::get_sql_string() for details. +*/ +#define SQL_XIDSIZE (XIDDATASIZE * 2 + 8 + MY_INT64_NUM_DECIMAL_DIGITS) +/* The 'buf' has to have space for at least SQL_XIDSIZE bytes. */ +uint get_sql_xid(XID *xid, char *buf);

Not sure you need this as an extern function in the header. Why not to keep it static in handler.cc?

/* for recover() handlerton call */ #define MIN_XID_LIST_SIZE 128 #define MAX_XID_LIST_SIZE (1024*128) diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 17e6904..d0fa642 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -1705,6 +1705,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, ulong *yystacksize); %token WRITE_SYM /* SQL-2003-N */ %token X509_SYM %token XA_SYM +%token XID_SYM %token XML_SYM %token XOR %token YEAR_MONTH_SYM

You forgot to add XID_SYM to the keyword_sp rule. Also I don't see why we should have CONVERT XID syntax if the output is incompatible. Perhaps it'd be better to have a different syntax? FORMAT=SQL? Regards, Sergei Chief Architect MariaDB and security@mariadb.org