maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx