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.
This patch improves/fixes the existing handling of CHARSET and
COLLATION symbols in the ColumnStore DDL parser.
Also, add fCollate and fCharsetNum member variables to the
ddlpackage::ColumnType class.
- occured -> occurred
- reponse -> response
- seperated -> separated
All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the
BSD-new license. I am contributing on behalf of my employer Amazon Web
Services, Inc.
* Fix clang warnings
* Remove vim tab guides
* initialize variables
* 'strncpy' output truncated before terminating nul copying as many bytes from a string as its length
* Fix ISO C++17 does not allow 'register' storage class specifier for outdated bison
* chars are unsigned on ARM, having if (ival < 0) always false
* chars are unsigned by default on ARM and comparison with -1 if always true
Part 1:
As part of MCOL-3776 to address synchronization issue while accessing
the fTimeZone member of the Func class, mutex locks were added to the
accessor and mutator methods. However, this slows down processing
of TIMESTAMP columns in PrimProc significantly as all threads across
all concurrently running queries would serialize on the mutex. This
is because PrimProc only has a single global object for the functor
class (class derived from Func in utils/funcexp/functor.h) for a given
function name. To fix this problem:
(1) We remove the fTimeZone as a member of the Func derived classes
(hence removing the mutexes) and instead use the fOperationType
member of the FunctionColumn class to propagate the timezone values
down to the individual functor processing functions such as
FunctionColumn::getStrVal(), FunctionColumn::getIntVal(), etc.
(2) To achieve (1), a timezone member is added to the
execplan::CalpontSystemCatalog::ColType class.
Part 2:
Several functors in the Funcexp code call dataconvert::gmtSecToMySQLTime()
and dataconvert::mySQLTimeToGmtSec() functions for conversion between seconds
since unix epoch and broken-down representation. These functions in turn call
the C library function localtime_r() which currently has a known bug of holding
a global lock via a call to __tz_convert. This significantly reduces performance
in multi-threaded applications where multiple threads concurrently call
localtime_r(). More details on the bug:
https://sourceware.org/bugzilla/show_bug.cgi?id=16145
This bug in localtime_r() caused processing of the Functors in PrimProc to
slowdown significantly since a query execution causes Functors code to be
processed in a multi-threaded manner.
As a fix, we remove the calls to localtime_r() from gmtSecToMySQLTime()
and mySQLTimeToGmtSec() by performing the timezone-to-offset conversion
(done in dataconvert::timeZoneToOffset()) during the execution plan
creation in the plugin. Note that localtime_r() is only called when the
time_zone system variable is set to "SYSTEM".
This fix also required changing the timezone type from a std::string to
a long across the system.
* Adds CompressInterfaceLZ4 which uses LZ4 API for compress/uncompress.
* Adds CMake machinery to search LZ4 on running host.
* All methods which use static data and do not modify any internal data - become `static`,
so we can use them without creation of the specific object. This is possible, because
the header specification has not been modified. We still use 2 sections in header, first
one with file meta data, the second one with pointers for compressed chunks.
* Methods `compress`, `uncompress`, `maxCompressedSize`, `getUncompressedSize` - become
pure virtual, so we can override them for the other compression algos.
* Adds method `getChunkMagicNumber`, so we can verify chunk magic number
for each compression algo.
* Renames "s/IDBCompressInterface/CompressInterface/g" according to requirement.
This patch:
1. Removes the option to declare uncompressed columns (set columnstore_compression_type = 0).
2. Ignores [COMMENT '[compression=0] option at table or column level (no error messages, just disregard).
3. Removes the option to set more than 2 extents per file (ExtentsPreSegmentFile).
4. Updates rebuildEM tool to support up to 10 dictionary extent per dictionary segment file.
5. Adds check for `DBRootStorageType` for rebuildEM tool.
6. Renamed rebuildEM to mcsRebuildEM.
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.
for a generated replacement statements of original statements:
* `CREATE TABLE .. LIKE ..`
* `ALTER TABLE .. ENGINE=Columnstore`
* `CREATE TABLE .. AS ..`
MCOL-2000 Process charset definitions in the ALTER TABLE .. ADD COLUMN
MCOL-2000 Yet another fixes for column charsets
* make respect for column (including table/db/server default) charsets
for the TEXT(n) fields
* round TEXT(n) column length up to the next default length of TEXT-like
subtypes, 255 (TINYTEXT), 65535 (TEXT) and so on up to 2100000000
(LONGTEXT)
Create tables and schemas with lower case name only if the flag is set.
During operations, convert to lowercase in plugin. Byt the time a query gets to ExeMgr, DDLProc etc., everything must be lower case if the flag is set, and undisturbed if not.
when a non-existing table is dropped.
This patch accomodates the changes made to server 10.5 as part
of MDEV-11412, where the server now tries to drop the table from
all storage engines when a .frm table does not exist.
We were earlier retuning a warning to the client and setting the
return code to 0. We now instead return ER_NO_SUCH_TABLE_IN_ENGINE
error code to the server if the table does not exist in
ColumnStore.
this is executed when ha_columnstore is loaded, at dlopen time
before any initialization code is run and crashes any dlopen
attemps.
Crashes - because a missing config file is a crashing offence,
but it's an issue for another day
This patch fixes:
MCOL-3557 - Row Based Replication events to ColumnStore tables will no
longer cause MariaDB to crash, it will error instead.
MCOL-3556 - Remove the Columnstore.xml variable to turn on ColumnStore
tables applying replication events and instead make it a system variable
that can be set in my.cnf called "columnstore_replication_slave". This
allows it to be set per-UM.
