maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Jan,


I checked changes to the /sql directory so far.
Sorry, I'm completely out of this topic.
So I have only general suggestions.



1. General code structure.

There are a lot of new

#ifdef WITH_WRESP
  do_something_with_wresp();
#else
  do_something_without_wresp();
#endif

This make the server code harder to follow.
I'd prefer a more modular way.
Would it be possible to introduce some abstract class with virtual
methods? One instantiable class would implement behavior for wresp,
and other class would implement behavior for "without wresp".

Instead #ifdefs, we would do:

  behavour->do_something();


WRESP support could be then turned into a real dynamic loadable plugin,
the server would be able to load it independently from the server
compilation flags.

Structures that need extra members,  for example, "struct THD_TRANS"
could be implemented like this:

struct THD_TRANS_WRESP: public THD_TRANS
{
public:
  int         rw_ha_count;
  void reset()
  {
    THD_TRANS::reset();
    rw_ha_count= 0;
  }
};

The virtual interface would do either "new THD_TRANS()" or "new
THD_TRANS_WRESP()",
depending on the WSREP support availability.


2. Please fix coding style in some places:


+  bool fix_length_and_dec()
+  {
+    max_length = WSREP_GTID_STR_LEN;
+    maybe_null = true;
+    return FALSE;
+  }

It should:

+  bool fix_length_and_dec()
+  {
+    max_length= WSREP_GTID_STR_LEN;
+    maybe_null= true;
+    return false;
+  }

There are more places that do not follow the coding style.
Please check.

3. Why do we need a separate directory /wresp with pure C files?
Are they shared by the server and by the clients? If not,
why not to put this code together with other wsrep related files?

4. Some code was commented. Why not just remove it?

Example:

+#ifdef TODO
     DBUG_SLOW_ASSERT(global_status_var.global_memory_used == 0);
+#endif


Example:

 #ifdef WITH_WSREP
-  if (thd->wsrep_exec_mode == LOCAL_STATE)
+  //  if (thd->wsrep_exec_mode == LOCAL_STATE)
     WSREP_TO_ISOLATION_BEGIN(WSREP_MYSQL_DB, NULL, NULL);
 #endif


Example:

+#include "log_event.h"
+// #include "binlog.h"
+

Example:

+int wsrep_write_dummy_event_low(THD *thd, const char *msg)
+{
+  ::abort();
+  return 0;
+#if 0
+  int ret= 0;
.... a lot of dead code
+  return ret;
+#endif
+}


Example:

+int wsrep_write_dummy_event(THD *orig_thd, const char *msg)
+{
+  return 0;
+#if 0
... a lot of dead code
+#endif
+}


Example:

+//#ifdef GTID_SUPPORT
+void wsrep_init_sidno(const wsrep::id& uuid)
.....
+//#endif /* GTID_SUPPORT */


Example:

+static std::string wsrep_server_id()
+{
+#ifdef WSREP_TODO
+  std::stringstream ss;
+  ss << server_id;
+  return(ss.str());
+#endif
+  /* us


Example:

+    // enum wsrep::provider::status ret = wsrep_sync_wait_upto(thd,
NULL, -1);
+    if (thd->wsrep_cs().sync_wait(-1))


Example:


+#if UNUSED /* 323f269d4099 (Jan Lindström     2018-07-19) */
 static const char* wsrep_get_query_or_msg(const THD* thd)
 {
   sw
... a lot of unused code
 }
+#endif //UNUSED


Example:

+#if 0
+    /* todo */
+    wsrep_xid_init(&thd->wsrep_xid,
+                   thd->wsrep_trx_meta.gtid.uuid,
+                   thd->wsrep_trx_meta.gtid.seqno);
+    if (tc_log) tc_log->commit(thd, true);
+#endif /* 0 */


Example:

+void* start_wsrep_THD(void *arg)
+{
+  THD *thd;
+  //wsrep_thd_processor_fun processor= (wsrep_thd_processor_fun)arg;


Example:

+//wsrep_t *get_wsrep()
+//{
+//  return wsrep;
+//}


Example:

+//int  wsrep_show_status(THD *thd, SHOW_VAR *var, char *buff,
+//                       enum enum_var_type scope);


There are many other pieces of the dead code.


5. Why this change?

--- a/sql/sql_basic_types.h
+++ b/sql/sql_basic_types.h
@@ -20,6 +20,8 @@
 #ifndef SQL_TYPES_INCLUDED
 #define SQL_TYPES_INCLUDED

+#include <my_global.h>
+


6. sql/wsrep_mysqld.cc: something is wrong with indentation:

   case 1:
   case 2:
   case 3:
+    case 4:


7. According to our coding style, multi-line comments like this:

+  // Note: We can't call THD destructor without crashing
+  // if plugins have not been initialized. However, in most of the
+  // cases this means that pre SE initialization SST failed and
+  // we are going to exit anyway.

should be:

  /*
    Note: We can't call THD destructor without crashing
    if plugins have not been initialized. However, in most of the
    cases this means that pre SE initialization SST failed and
  */
    we are going to exit anyway.


8. This looks strange:

+const char wsrep_defaults_group_suffix[256] = {0};

Why do you need an array of 256 zero bytes in the const static area?


9. You remove a lot of code in one place and add new code in other places.
It seems you move some old code to new locations.

For example, this code was removed from sql/wsrep_thd.cc.diff:

-void wsrep_replay_transaction(THD *thd)

I can see its pieces in wsrep_high_priority_service.h,
in the class Wsrep_replayer_service.

Can refactoring changes like this be done in a separate patch?

It would make reviewing easier.


10. Please don't use redundant current_thd calls when a THD pointer
is already available in a local variable or a function parameter.

Example:

@@ -2457,7 +2460,7 @@ void Item_func_rand::seed_random(Item *arg)
   THD *thd= current_thd;
   if (WSREP(thd))
   {
-    if (thd->wsrep_exec_mode==REPL_RECV)
+    if (wsrep_thd_is_applying(current_thd))


11. When does current_thd return NULL?

+  if (current_thd)
+  {
+    /* Undocking the thread specific data. */
+    my_pthread_setspecific_ptr(THR_THD, NULL);
+    /

12. Perhaps ErrConv family classes could be used in some cases:

This:

+  std::ostringstream os;
+  os << view.state_id().id();
+  wsrep_update_cluster_state_uuid(os.str().c_str());

can probably be shortened to something like this:


wsrep_update_cluster_state_uuid(ErrConvInteger(view.state_id().id()).ptr());


13. There's something wrong with semicolons here:

+  /* DTRACE begin */
+  MYSQL_QUERY_START(rawbuf, thd->thread_id,
+                   (char *)(thd->db.str ? : thd->db.str : "unknown"),
+                    &thd->security_ctx->priv_user[0],
+                    (char *) thd->security_ctx->host_or_ip);


Notice, "?" immediately followed by ":".
This should not compile. Is it an optionally compiled code?
Please make sure to compile in all possible build option
combinations, to check all optional code.