Fixes according to Svoj's review #2

[sjaakola]

This pull request contains fixes for issues mentioned in Svoj's review #2 email.

There was request to remove current_thd uses in to he code. Most current_thd references are now cleaned out, but there still remains a number of DBUG_ASSERTs where current_thd is checked. These assert statements can also be removed, if need be.

[janlindstrom]

This commented line could be removed.

[svoj]

NB: if you plan to update this PR, please just push additional patches on top of this one (instead of force pushing). I'll squash them in one commit when merging.

List of things that were not addressed or commented:

> diff --git a/extra/mariabackup/CMakeLists.txt b/extra/mariabackup/CMakeLists.txt
> index 7df5fa1..08745ea 100644
> --- a/extra/mariabackup/CMakeLists.txt
> +++ b/extra/mariabackup/CMakeLists.txt
> @@ -41,7 +41,7 @@ IF(NOT HAVE_SYSTEM_REGEX)
>  ENDIF()
>
>  IF(WITH_WSREP)
> -  INCLUDE_DIRECTORIES(${CMAKE_SOURCE_DIR}/wsrep)
> +  INCLUDE_DIRECTORIES(${CMAKE_SOURCE_DIR}/wsrep-lib)
>  ENDIF()
>
>  ADD_DEFINITIONS(-UMYSQL_SERVER)

wsrep and wsrep-lib have different hierarchy. However other files under
mariabackup were not adjusted accordingly, which means this code has no
effect?

> diff --git a/mysql-test/include/mtr_warnings.sql b/mysql-test/include/mtr_warnings.sql
> index 524ea0f..481f924 100644
> --- a/mysql-test/include/mtr_warnings.sql
> +++ b/mysql-test/include/mtr_warnings.sql
> @@ -229,6 +229,52 @@ INSERT INTO global_suppressions VALUES
>     MDEV-12501 -- set --maturity-level by default
>   */
>   ("Plugin .* is of maturity level .* while the server is .*"),
> + ("WSREP:*down context*"),
> + ("WSREP: Failed to send state UUID:*"),
> + ("WSREP: wsrep_sst_receive_address is set to '127.0.0.1"),
> + ("WSREP: option --wsrep-causal-reads is deprecated"),
> + ("WSREP: --wsrep-causal-reads=ON takes precedence over --wsrep-sync-wait=0"),
> + ("WSREP: Could not open saved state file for reading: "),
> + ("WSREP: Could not open state file for reading: "),
> + ("WSREP: access file\\(.*gvwstate\\.dat\\) failed\\(No such file or directory\\)"),
> + ("WSREP: Gap in state sequence\\. Need state transfer\\."),
> + ("WSREP: Failed to prepare for incremental state transfer: Local state UUID \\(00000000-0000-0000-0000-000000000000\\) does not match group state UUID"),
> + ("WSREP: No existing UUID has been found, so we assume that this is the first time that this server has been started\\. Generating a new UUID: "),
> + ("WSREP: last inactive check more than"),
> + ("WSREP: binlog cache not empty \\(0 bytes\\) at connection close"),
> + ("WSREP: SQL statement was ineffective"),
> + ("WSREP: Refusing exit for the last slave thread"),
> + ("WSREP: Quorum: No node with complete state"),
> + ("WSREP: Failed to report last committed"),
> + ("Slave SQL: Error 'Duplicate entry"),
> + ("Query apply warning:"),
> + ("WSREP: Ignoring error"),
> + ("WSREP: Initial position was provided by configuration or SST, avoiding override"),
> + ("Warning: Using a password on the command line interface can be insecure"),
> + ("InnoDB: Error: Table \"mysql\"\\.\"innodb_table_stats\" not found"),
> + ("but it is impossible to select State Transfer donor: Resource temporarily unavailable"),
> + ("WSREP: Could not find peer"),
> + ("WSREP: discarding established \\(time wait\\)"),
> + ("sending install message failed: Resource temporarily unavailable"),
> + ("WSREP: Ignoring possible split-brain \\(allowed by configuration\\) from view"),
> + ("WSREP: no nodes coming from prim view, prim not possible"),
> + ("WSREP: Failed to prepare for incremental state transfer: Local state seqno is undefined:"),
> + ("WSREP: gcs_caused\\(\\) returned -107 \\(Transport endpoint is not connected\\)"),
> + ("WSREP: gcs_caused\\(\\) returned -57 \\(Socket is not connected\\)"),
> + ("WSREP: gcs_caused\\(\\) returned -1 \\(Operation not permitted\\)"),
> + ("Action message in non-primary configuration from member"),
> + ("SYNC message from member"),
> + ("InnoDB: Resizing redo log from"),
> + ("InnoDB: Starting to delete and rewrite log files"),
> + ("InnoDB: New log files created, LSN="),
> +-- WSREP: Send action {0x7f86280147f0, 73, STATE_REQUEST} returned -107 (Transport endpoint is not connected)
> + ("Transport endpoint is not connected"),
> + ("Socket is not connected"),
> +-- "WSREP: Protocol violation. JOIN message sender 1.0 (host-91-221-67-96) is not in state transfer (SYNCED). Message ignored.
> + ("is not in state transfer"),
> + ("JOIN message from member .* in non-primary configuration"),
> + ("install timer expired"),
> + ("Last Applied Action message in non-primary configuration from member"),
>
>   ("THE_LAST_SUPPRESSION")||

This was introduced in 08f3a7f to
"Fix galera.galera_as_slave_nonprim and galera_3nodes.galera_pc_weight on FreeBSD"

If this claim is valid, we shouldn't extend global suppressions list and move
it inside approrpiate test cases.

Suggested changes:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 0dba394..269aa71 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -382,12 +382,6 @@ INCLUDE(mariadb_connector_c) # this does ADD_SUBDIRECTORY(libmariadb)
 # Add storage engines and plugins.
 CONFIGURE_PLUGINS()

-IF(NOT WITHOUT_SERVER)
-  IF(WITH_WSREP)
-    ADD_SUBDIRECTORY(wsrep-lib)
-  ENDIF()
-ENDIF()
-
 ADD_SUBDIRECTORY(include)
 ADD_SUBDIRECTORY(dbug)
 ADD_SUBDIRECTORY(strings)
@@ -409,6 +403,10 @@ IF(NOT WITHOUT_SERVER)
    ADD_SUBDIRECTORY(unittest/embedded)
   ENDIF(WITH_EMBEDDED_SERVER)

+  IF(WITH_WSREP)
+    ADD_SUBDIRECTORY(wsrep-lib)
+  ENDIF()
+
   ADD_SUBDIRECTORY(mysql-test)
   ADD_SUBDIRECTORY(mysql-test/lib/My/SafeProcess)
   ADD_SUBDIRECTORY(sql-bench)
@@ -427,6 +425,7 @@ INCLUDE(cmake/tags.cmake)



+
 INCLUDE(for_clients)
 ADD_SUBDIRECTORY(scripts)
 ADD_SUBDIRECTORY(support-files)
diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc
index 606b939..d566652 100644
--- a/sql/ha_partition.cc
+++ b/sql/ha_partition.cc
@@ -405,13 +405,6 @@ const char *ha_partition::table_type() const
   // we can do this since we only support a single engine type
   return m_file[0]->table_type();
 }
-#ifdef WITH_WSREP
-int ha_partition::wsrep_db_type() const
-{
-  // we can do this since we only support a single engine type
-  return ha_legacy_type(m_file[0]->ht);
-}
-#endif /* WITH_WSREP */


 /*
diff --git a/sql/ha_partition.h b/sql/ha_partition.h
index 000bf36..f79d71a 100644
--- a/sql/ha_partition.h
+++ b/sql/ha_partition.h
@@ -1506,8 +1506,5 @@ class ha_partition :public handler

   friend int cmp_key_rowid_part_id(void *ptr, uchar *ref1, uchar *ref2);
   friend int cmp_key_part_id(void *key_p, uchar *ref1, uchar *ref2);
-#ifdef WITH_WSREP
-    virtual int wsrep_db_type() const;
-#endif /* WITH_WSREP */
 };
 #endif /* HA_PARTITION_INCLUDED */
diff --git a/sql/handler.cc b/sql/handler.cc
index 4c69d9f..ecde03d 100644
--- a/sql/handler.cc
+++ b/sql/handler.cc
@@ -6280,7 +6280,6 @@ int binlog_log_row(TABLE* table, const uchar *before_record,

   /* only InnoDB tables will be replicated through binlog emulation */
   if ((WSREP_EMULATE_BINLOG(thd) &&
-       table->file->ht->db_type != DB_TYPE_INNODB &&
        table->file->partition_ht()->db_type != DB_TYPE_INNODB) ||
       (thd->wsrep_ignore_table == true))
     return 0;
diff --git a/sql/wsrep_mysqld.cc b/sql/wsrep_mysqld.cc
index 5e59bf8..e0e0f40 100644
--- a/sql/wsrep_mysqld.cc
+++ b/sql/wsrep_mysqld.cc
@@ -2959,7 +2959,6 @@ void* start_wsrep_THD(void *arg)
   thd->proc_info= 0;
   thd->set_command(COM_SLEEP);
   thd->init_for_queries();
-  DBUG_ASSERT(current_thd);
   mysql_mutex_lock(&LOCK_thread_count);
   wsrep_running_threads++;
   mysql_cond_broadcast(&COND_thread_count);
@@ -2985,7 +2984,6 @@ void* start_wsrep_THD(void *arg)
   WSREP_DEBUG("wsrep running threads now: %lu", wsrep_running_threads);
   mysql_cond_broadcast(&COND_thread_count);
   mysql_mutex_unlock(&LOCK_thread_count);
-  DBUG_ASSERT(current_thd);
   /*
     Note: We can't call THD destructor without crashing
     if plugins have not been initialized. However, in most of the

[svoj]

My suggestion was to revert the whole hunk:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 5415efb..ed55e7a 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -382,6 +382,10 @@ INCLUDE(mariadb_connector_c) # this does ADD_SUBDIRECTORY(libmariadb)
 # Add storage engines and plugins.
 CONFIGURE_PLUGINS()

+IF(WITH_WSREP)
+  ADD_SUBDIRECTORY(wsrep-lib)
+ENDIF()
+
 ADD_SUBDIRECTORY(include)
 ADD_SUBDIRECTORY(dbug)
 ADD_SUBDIRECTORY(strings)
@@ -403,10 +407,6 @@ IF(NOT WITHOUT_SERVER)
    ADD_SUBDIRECTORY(unittest/embedded)
   ENDIF(WITH_EMBEDDED_SERVER)

-  IF(WITH_WSREP)
-    ADD_SUBDIRECTORY(wsrep)
-  ENDIF()
-
   ADD_SUBDIRECTORY(mysql-test)
   ADD_SUBDIRECTORY(mysql-test/lib/My/SafeProcess)
   ADD_SUBDIRECTORY(sql-bench)

This way we could avoid redundant IF(NOT WITHOUT_SERVER). OTOH this form looks acceptable as well.

[svoj]

I meant the opposite: add it back. Looks like it is used for generating mariadb.pc. @vuvova?

[svoj]

According to @vuvova this line is probably not needed anymore. Feel free to ignore my request, unless Sergei suggests the opposite.

However I must emphasise probably. What was actual reason for commenting this line out?

[svoj]

Thanks, but please remove include/have_ipv6.inc file as well.

[svoj]

This one should be reverted. The above 2 fixes are correct.

[svoj]

Frankly speaking original check looks more meaningful.

table->file->partition_ht()->db_type != DB_TYPE_INNODB

is equivalent of

table->file->ht->db_type != DB_TYPE_INNODB &&
!(table->file->ht->db_type == DB_TYPE_PARTITION_DB &&
  (((ha_partition*)(table->file))->wsrep_db_type() == DB_TYPE_INNODB))

I think we can keep original check and revert wsrep_db_type().