There were numerous memory leaks in plugin's code and associated code.
During typical run of MTR tests it leaked around 65 megabytes of
objects. As a result they may severely affect long-lived connections.
This patch fixes (almost) all leaks found in the plugin. The exceptions
are two leaks associated with SHOW CREATE TABLE columnstore_table and
getting information of columns of columnstore-handled table. These
should be fixed on the server side and work is on the way.
ColumnStore used to include server's mysql.h
but link all tools with libmariadb.so
There's no guarantee that this would work, even with workarounds
it had in dbcon/mysql/sm.cpp
Fix:
* tools (linked with libmariadb.so) *must* include libmariadb's mysql.h
* as a hack prevent service_thd_timezone.h from being loaded into tools,
as it conflicts with libmariadb's mysql.h
* server plugin *must* include server's mysql.h
* also don't link every tool with libmariadb.so, link the helper library
(liblibmysqlclient.so) that actually needs it, tools use this
helper library, not libmariadb.so directly
mcsconfig.h and my_config.h have the following
pre-processor definitions:
1. Conflicting definitions coming from the standard cmake definitions:
- PACKAGE
- PACKAGE_BUGREPORT
- PACKAGE_NAME
- PACKAGE_STRING
- PACKAGE_TARNAME
- PACKAGE_VERSION
- VERSION
2. Conflicting definitions of other kinds:
- HAVE_STRTOLL - this is a dirt in MariaDB headers.
Should be fixed in the server code. my_config.h erroneously
performs "#define HAVE_STRTOLL" instead of "#define HAVE_STRTOLL 1".
in some cases. The former is not CMake compatible style. The latter is.
3. Non-conflicting definitions:
Otherwise, mcsconfig.h and my_config.h should be mutually compatible,
because both are generated by cmake on the same host machine. So
they should have exactly equal definitions like "HAVE_XXX", "SIZEOF_XXX", etc.
Observations:
- It's OK to include both mcsconfig.h and my_config.h providing that we
suppress duplicate definition of the above conflicting types #1 and #2.
- There is no a need to suppress duplicate definitions mentioned in #3,
as they are compatible!
- my_sys.h and m_ctype.h must always follow a CMake configuation header,
either my_config.h or mcsconfig.h (or both).
They must never be included without any preceeding configuration header.
This change make sure that we resolve conflicts by:
- either disallowing inclusion of mcsconfig.h and my_config.h
at the same time
- or by hiding conflicting definitions #1 and #2
(with their later restoring).
- also, by making sure that my_sys.h and m_ctype.h always follow
a CMake configuration file.
Details:
- idb_mysql.h can now only be included only after my_config.h
An attempt to use idb_mysql.h with mcsconfig.h instead of
my_config.h is caught by the "#error" preprocessor directive.
- mariadb_my_sys.h can now be only included after mcsconfig.h.
An attempt to use mariadb_my_sys.h without mcscofig.h
(e.g. with my_config.h) is also caught by "#error".
- collation.h now can now be included in two ways.
It now has the following effective structure:
#if defined(PREFER_MY_CONFIG_H) && defined(MY_CONFIG_H)
// Remember current conflicting definitions on the preprocessor stack
// Undefine current conflicting definitions
#endif
#include "mcsconfig.h"
#include "m_ctype.h"
#if defined(PREFER_MY_CONFIG_H) && defined(MY_CONFIG_H)
# Restore conflicting definitions from the preprocessor stack
#endif
and can be included as follows:
a. using only mcsconfig.h as a configuration header:
// my_config.h must not be included so far
#include "collation.h"
b. using my_config.h as the first included configuration file:
#define PREFER_MY_CONFIG_H // Force conflict resolution
#include "my_config.h" // can be included directly or indirectly
...
#include "collation.h"
Other changes:
- Adding helper header files
utils/common/mcsconfig_conflicting_defs_remember.h
utils/common/mcsconfig_conflicting_defs_restore.h
utils/common/mcsconfig_conflicting_defs_undef.h
to perform conflict resolution easier.
- Removing `#include "collation.h"` from a number of files,
as it's automatically included from rowgroup.h.
- Removing redundant `#include "utils_utf8.h"`.
This change is not directly related to the problem being fixed,
but it's nice to remove redundant directives for both collation.h
and utils_utf8.h from all the files that do not really need them.
(this change could probably have gone as a separate commit)
- Changing my_init() to MY_INIT(argv[0]) in the MCS services sources.
After the fix of the complitation failure it appeared that ColumnStore
services compiled with the debug build crash due to recent changes in
safemalloc. The crash happened in strcmp() with `my_progname` as an argument
(where my_progname is a mysys global variable). This problem should
probably be fixed on the server side as well to avoid passing NULL.
But, the majority of MariaDB executable programs also use MY_INIT(argv[0])
rather than my_init(). So let's make MCS do like the other programs do.
Disabled 4th if block in buildOuterJoin to handle non-optimized MDB query
structures.
Broke getSelectPlan into pieces: processFrom, processWhere.
MCOL-3593 UNION processing depends on two flags isUnion that comes as
arg of getSelectPlan and unionSel that is a local variable in
getSelectPlan. Modularization of getSelectPlan broke the mechanizm.
This patch is supposed to partially fix it.
MCOL-3593 Removed unused if condition from buildOuterJoin that allows
unsupported construct subquery in ON expression.
Fixed an improper if condition that ignors tableMap entries w/o condition
in external_lock thus external_lock doesn't clean up when the query
finishes.
Fixed wrong logging for queries processed in tableMode. Now rnd_init
properly sends queryText down to ExeMgr to be logged.
MCOL-3593 Unused attribute FromSubQuery::fFromSub was removed.
getSelectPlan has been modularized into: setExecutionParams,
processFrom, processWhere. SELECT, HAVING, GROUP BY, ORDER BY
still lives in getSelectPlan.
Copied optimization function simplify_joins_ into our pushdown
code to provide the plugin code with some rewrites from MDB it
expects.
The columnstore_processing_handlers_fallback session variable
has been removed thus CS can't fallback from SH to partial
execution paths, e.g. DH, GBH or plugin API.
MCOL-3602 Moved MDB optimizer rewrites into a separate file.
Add SELECT_LEX::optimize_unflattened_subqueries() call to fix IN
into EXISTS rewrite for semi-JOINs with subqueries.
disable_indices_for_CEJ() add index related hints to disable
index access methods in Cross Engine Joins.
create_SH() now flattens JOIN that has both physical tables and
views. This fixes most of views related tests in the regression.
SH query execution migrated from SH::init() into create_SH().
There is a session variable columnstore_processing_handlers_fallback
that allows to fallback to DH, GBH if SH fails. DH now uses semantic
tree check for unsupported features to allow to fallback to GBH or
storage API.
Fixes GBH related bug when create_GBH() returns a handler for
queries with impossible WHERE/HAVING.
Fixed bug in FromSubquery::transform() where isUnion is set to true.
Enabled RTTI b/c server team enabled it for MDB.
Removed unused code supposed to be used with vtable.
sortItemIsInGroupRec() now has a base case for Item_Field and this prevents unbound recursion.
Fixes fromSub() call with incorrect number of arguments.
CS now doesn't ask for statistics unless a user explicitly asks for it using calsettrace().
With 1.1 we have removed libdrizzle and used MariaDB's client library
instead for both CrossEngine and QueryStats. Unfortunately MariaDB 10.2
has two client libraries which have different structs with the same
name. When QueryStats was running inside the ColumnStore plugin this
symbol conflict was causing a crash.
The server's built-in client API has several different and several
missing functions so some additions to sm.cpp were made to fill the
gaps.
This patch does the following:
* Make sure that libmariadb is only linked to executables, not the
ColumnStore Plugin (to avoid symbol conflicts). Note that all
executables that link to CrossEngine and/or QueryStats need to link to
libmariadb to avoid missing symbol issues.
* Use the server's built-in client API for QueryStats when run in the
plugin
* Replace missing server built-in client API calls in sm.cpp (this is
for QueryStats and CrossEngine to keep the dynamic linker happy)
* Fixes issue where using 'localhost' as the MariaDB Server hostname
would fail in QueryStats.
