From 5fa8d1fd00f15cdfc668bfa3bd74d878dd342575 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mikael=20Ronstr=C3=B6m?= Date: Wed, 14 Dec 2022 22:15:19 +0100 Subject: [PATCH] BUG#32959894 ClusterJ uses a scratch buffer for primary key hash calculations which is limited to 10000 bytes. This is too small in some cases. The solution is to malloc the buffer in computeHash if the buffer isn't big enough. This also solves some application problems. This patch contains a test for this bug that ensures that the primary key requires a buffer larger than 10000 bytes. --- mysql-test/suite/ndb/r/clusterj.result | 1 + mysql-test/suite/ndb/t/clusterj.test | 1 + .../ndb/clusterj/clusterj-test/CMakeLists.txt | 2 + .../testsuite/clusterj/Bug17200163Test.java | 9 +- .../clusterj/DynamicStringPKTest.java | 246 ++++++++++++++++++ .../clusterj/model/ConversationSummary.java | 32 ++- .../clusterj/model/DynamicStringPKs.java | 105 ++++++++ .../src/main/resources/schema.sql | 21 +- storage/ndb/src/ndbapi/Ndb.cpp | 30 ++- 9 files changed, 431 insertions(+), 16 deletions(-) create mode 100644 storage/ndb/clusterj/clusterj-test/src/main/java/testsuite/clusterj/DynamicStringPKTest.java create mode 100644 storage/ndb/clusterj/clusterj-test/src/main/java/testsuite/clusterj/model/DynamicStringPKs.java diff --git a/mysql-test/suite/ndb/r/clusterj.result b/mysql-test/suite/ndb/r/clusterj.result index 6840b2e214f5..842b40bb1081 100644 --- a/mysql-test/suite/ndb/r/clusterj.result +++ b/mysql-test/suite/ndb/r/clusterj.result @@ -63,5 +63,6 @@ DROP TABLE subscriber; DROP TABLE hashonlylongintstringpk; DROP TABLE varbinarypk; DROP TABLE varbinarytypes; +DROP TABLE dynamicstringpks; DROP TABLE IF EXISTS test2.t_basic2; DROP DATABASE IF EXISTS test2; diff --git a/mysql-test/suite/ndb/t/clusterj.test b/mysql-test/suite/ndb/t/clusterj.test index a526dc5a260a..7d965a1ae271 100644 --- a/mysql-test/suite/ndb/t/clusterj.test +++ b/mysql-test/suite/ndb/t/clusterj.test @@ -89,6 +89,7 @@ DROP TABLE subscriber; DROP TABLE hashonlylongintstringpk; DROP TABLE varbinarypk; DROP TABLE varbinarytypes; +DROP TABLE dynamicstringpks; DROP TABLE IF EXISTS test2.t_basic2; DROP DATABASE IF EXISTS test2; diff --git a/storage/ndb/clusterj/clusterj-test/CMakeLists.txt b/storage/ndb/clusterj/clusterj-test/CMakeLists.txt index 918eb93c77fd..3a6864365fa2 100644 --- a/storage/ndb/clusterj/clusterj-test/CMakeLists.txt +++ b/storage/ndb/clusterj/clusterj-test/CMakeLists.txt @@ -65,6 +65,7 @@ SET(JAVA_SOURCES ${CLUSTERJ_TESTSUITE_PREFIX}/DynamicBytePKTest.java ${CLUSTERJ_TESTSUITE_PREFIX}/DynamicObjectTest.java ${CLUSTERJ_TESTSUITE_PREFIX}/DynamicShortPKTest.java + ${CLUSTERJ_TESTSUITE_PREFIX}/DynamicStringPKTest.java ${CLUSTERJ_TESTSUITE_PREFIX}/FindByPrimaryKey2Test.java ${CLUSTERJ_TESTSUITE_PREFIX}/FindByPrimaryKeyErrorHandlingTest.java ${CLUSTERJ_TESTSUITE_PREFIX}/FindByPrimaryKeyTest.java @@ -182,6 +183,7 @@ SET(JAVA_SOURCES ${CLUSTERJ_TESTSUITE_PREFIX}/model/Dn2id.java ${CLUSTERJ_TESTSUITE_PREFIX}/model/DoubleTypes.java ${CLUSTERJ_TESTSUITE_PREFIX}/model/DynamicPK.java + ${CLUSTERJ_TESTSUITE_PREFIX}/model/DynamicStringPKs.java ${CLUSTERJ_TESTSUITE_PREFIX}/model/Employee.java ${CLUSTERJ_TESTSUITE_PREFIX}/model/Employee2.java ${CLUSTERJ_TESTSUITE_PREFIX}/model/FloatTypes.java diff --git a/storage/ndb/clusterj/clusterj-test/src/main/java/testsuite/clusterj/Bug17200163Test.java b/storage/ndb/clusterj/clusterj-test/src/main/java/testsuite/clusterj/Bug17200163Test.java index 1d5f3195d628..9694731ede98 100644 --- a/storage/ndb/clusterj/clusterj-test/src/main/java/testsuite/clusterj/Bug17200163Test.java +++ b/storage/ndb/clusterj/clusterj-test/src/main/java/testsuite/clusterj/Bug17200163Test.java @@ -38,7 +38,7 @@ This program is also distributed with certain software (including public class Bug17200163Test extends AbstractClusterJModelTest { - protected int NUMBER_TO_INSERT = 10; + protected int NUMBER_TO_INSERT = 40; /** The instances for testing. */ protected List instances = new ArrayList(); @@ -70,7 +70,7 @@ protected void insert() { for (int i = 0; i < NUMBER_TO_INSERT; ++i) { // must be done with an active transaction - session.makePersistent(instances.get(i)); + session.savePersistent(instances.get(i)); ++count; } tx.commit(); @@ -122,6 +122,11 @@ protected void createInstances(int number) { instance.setDestUserId(i); instance.setLastMessageById(i); instance.setQueryHistoryId(i); + instance.setKey5(i); + instance.setKey6("Text " + i); + instance.setKey7("Text " + i); + instance.setKey8("Text " + i); + instance.setKey9("Text " + i); instance.setText("Text " + i); instance.setUpdatedAt(i); instance.setViewed(0 == (i%2)?true:false); diff --git a/storage/ndb/clusterj/clusterj-test/src/main/java/testsuite/clusterj/DynamicStringPKTest.java b/storage/ndb/clusterj/clusterj-test/src/main/java/testsuite/clusterj/DynamicStringPKTest.java new file mode 100644 index 000000000000..8eecd43a4c7f --- /dev/null +++ b/storage/ndb/clusterj/clusterj-test/src/main/java/testsuite/clusterj/DynamicStringPKTest.java @@ -0,0 +1,246 @@ +/* + * Copyright (c) 2011, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2021, 2021, Logical Clocks AB and/or its affiliates. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License, version 2.0, + * as published by the Free Software Foundation. + * + * This program is also distributed with certain software (including + * but not limited to OpenSSL) that is licensed under separate terms, + * as designated in a particular file or component or in included license + * documentation. The authors of MySQL hereby grant you an additional + * permission to link the program and your derivative works with the + * separately licensed software that they have included with MySQL. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License, version 2.0, for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +*/ + +package testsuite.clusterj; + +import java.util.ArrayList; +import java.util.List; + +import com.mysql.clusterj.DynamicObject; +import testsuite.clusterj.model.DynamicStringPKs; + +public class DynamicStringPKTest extends AbstractClusterJTest { + + protected int NUMBER_OF_INSTANCES = 15; + protected List instances = new ArrayList(); + + @Override + public void localSetUp() { + createSessionFactory(); + session = sessionFactory.getSession(); + tx = session.currentTransaction(); + } + + public void test() { + run(DynamicStringPK.class); + failOnError(); + } + + public void run(Class cls) { + deleteAll(cls); + createInstances(cls); + insert(); + find(cls); + update(cls); + delete(cls); + } + + public static class DynamicStringPK extends DynamicStringPKs { + @Override + public String table() { + return "dynamicstringpks"; + } + } + + protected void deleteAll(Class cls) { + try { + tx.begin(); + session.deletePersistentAll(cls); + tx.commit(); + } catch (Throwable t) { + // ignore errors while deleting + } + } + + /** Insert all instances. + */ + protected void insert() { + session.makePersistentAll(instances); + } + + /** Find all instances. + */ + protected void find(Class cls) { + String where = "find " + cls.getName(); + for (int i = 0; i < NUMBER_OF_INSTANCES; ++i) { + tx.begin(); + String key = getPK(i); + DynamicStringPKs dynObj = (DynamicStringPKs) session.newInstance(cls); + dynObj.setKey1(key); + dynObj.setKey2(key); + dynObj.setKey3(key); + dynObj.setKey4(i); + dynObj.setKey5(key); + dynObj.setKey6(i); + dynObj.setKey7(key); + DynamicStringPKs result = session.load(dynObj); + session.flush(); + tx.commit(); + verify(where, result, i, false); + } + } + + /** Blind update every fourth instance. + */ + protected void update(Class cls) { + // update the instances + String where = "update before " + cls.getName(); + for (int i = 0; i < NUMBER_OF_INSTANCES; ++i) { + if (0 == i % 4) { + DynamicStringPKs instance = createInstance(cls, i); + instance.setName(getValue(NUMBER_OF_INSTANCES - i)); + session.savePersistent(instance); + verify(where, instance, i, true); + } + } + // verify the updated instances + where = "update after " + cls.getName(); + for (int i = 0; i < NUMBER_OF_INSTANCES; ++i) { + if (0 == i % 4) { + tx.begin(); + String key = getPK(i); + DynamicStringPKs dynObj = (DynamicStringPKs) session.newInstance(cls); + dynObj.setKey1(key); + dynObj.setKey2(key); + dynObj.setKey3(key); + dynObj.setKey4(i); + dynObj.setKey5(key); + dynObj.setKey6(i); + dynObj.setKey7(key); + DynamicStringPKs result = session.load(dynObj); + session.flush(); + tx.commit(); + verify(where, result, i, true); + } + } + } + + /** Blind delete every fifth instance. + */ + protected void delete(Class cls) { + // delete the instances + for (int i = 0; i < NUMBER_OF_INSTANCES; ++i) { + if (0 == i % 5) { + DynamicStringPKs instance = createInstance(cls, i); + session.deletePersistent(instance); + } + } + // verify they have been deleted + for (int i = 0; i < NUMBER_OF_INSTANCES; ++i) { + if (0 == i % 5) { + tx.begin(); + String key = getPK(i); + DynamicStringPKs dynObj = (DynamicStringPKs) session.newInstance(cls); + dynObj.setKey1(key); + dynObj.setKey2(key); + dynObj.setKey3(key); + dynObj.setKey4(i); + dynObj.setKey5(key); + dynObj.setKey6(i); + dynObj.setKey7(key); + DynamicStringPKs result = session.load(dynObj); + session.flush(); + tx.commit(); + Boolean found_object = session.found(dynObj); + if (!found_object || found_object == null) + { + result = null; + } + errorIfNotEqual("Failed to delete instance: " + i, null, result); + } + } + } + + protected void createInstances(Class cls) { + instances.clear(); + for (int i = 0; i < NUMBER_OF_INSTANCES; ++i) { + DynamicStringPKs instance = createInstance(cls, i); + if (getDebug()) System.out.println(toString(instance)); + instances.add(instance); + } + } + + /** Create an instance of DynamicStringPKs. + * @param index the index to use to generate data + * @return the instance + */ + protected DynamicStringPKs createInstance(Class cls, int index) { + DynamicStringPKs instance = session.newInstance(cls); + instance.setKey1(getPK(index)); + instance.setKey2(getPK(index)); + instance.setKey3(getPK(index)); + instance.setKey4(index); + instance.setKey5(getPK(index)); + instance.setKey6(index); + instance.setKey7(getPK(index)); + instance.setNumber(index); + instance.setName(getValue(index)); + return instance; + } + + protected String toString(DynamicStringPKs instance) { + StringBuffer result = new StringBuffer(instance.getClass().getName()); + result.append("["); + result.append(instance.getKey1()); + result.append(", \""); + result.append(instance.getKey2()); + result.append(", \""); + result.append(instance.getKey3()); + result.append(", \""); + result.append(instance.getKey4()); + result.append(", \""); + result.append(instance.getKey5()); + result.append(", \""); + result.append(instance.getKey6()); + result.append(", \""); + result.append(instance.getKey7()); + result.append("]: "); + result.append(instance.getNumber()); + result.append(", \""); + result.append(instance.getName()); + result.append("\"."); + return result.toString(); + } + + protected String getPK(int index) { + String result = "Text............. " + index; + return result; + } + + protected String getValue(int index) { + return "Value " + index; + } + + protected void verify(String where, DynamicStringPKs instance, int index, boolean updated) { + errorIfNotEqual(where + "id failed", getPK(index), instance.getKey1()); + errorIfNotEqual(where + "number failed", index, instance.getNumber()); + if (updated) { + errorIfNotEqual(where + " Value failed", getValue(NUMBER_OF_INSTANCES - index), instance.getName()); + } else { + errorIfNotEqual(where + " Value failed", getValue(index), instance.getName()); + + } + } +} diff --git a/storage/ndb/clusterj/clusterj-test/src/main/java/testsuite/clusterj/model/ConversationSummary.java b/storage/ndb/clusterj/clusterj-test/src/main/java/testsuite/clusterj/model/ConversationSummary.java index a74560516ccc..e0d23c81ee93 100644 --- a/storage/ndb/clusterj/clusterj-test/src/main/java/testsuite/clusterj/model/ConversationSummary.java +++ b/storage/ndb/clusterj/clusterj-test/src/main/java/testsuite/clusterj/model/ConversationSummary.java @@ -38,10 +38,15 @@ destination_user_id bigint(11) NOT NULL, last_message_user_id bigint(11) NOT NULL, text_summary varchar(255) NOT NULL DEFAULT '', query_history_id bigint(20) NOT NULL DEFAULT '0', + key5 bigint(20) NOT NULL DEFAULT '0', + key6 varchar(100) NOT NULL DEFAULT '0', + key7 varchar(100) NOT NULL DEFAULT '0', + key8 varchar(100) NOT NULL DEFAULT '0', + key9 varchar(100) NOT NULL DEFAULT '0', answerer_id bigint(11) NOT NULL, viewed bit(1) NOT NULL, updated_at bigint(20) NOT NULL, - PRIMARY KEY (source_user_id,destination_user_id,query_history_id), + PRIMARY KEY (source_user_id,destination_user_id,query_history_id,key5,key6,key7,key8,key9), KEY IX_updated_at (updated_at) ) ENGINE=ndbcluster; @@ -72,6 +77,31 @@ public interface ConversationSummary { long getQueryHistoryId(); void setQueryHistoryId(long id); + @PrimaryKey + @Column(name = "key5") + long getKey5(); + void setKey5(long id); + + @PrimaryKey + @Column(name = "key6") + String getKey6(); + void setKey6(String key); + + @PrimaryKey + @Column(name = "key7") + String getKey7(); + void setKey7(String key); + + @PrimaryKey + @Column(name = "key8") + String getKey8(); + void setKey8(String key); + + @PrimaryKey + @Column(name = "key9") + String getKey9(); + void setKey9(String key); + @Column(name = "answerer_id") long getAnswererId(); void setAnswererId(long id); diff --git a/storage/ndb/clusterj/clusterj-test/src/main/java/testsuite/clusterj/model/DynamicStringPKs.java b/storage/ndb/clusterj/clusterj-test/src/main/java/testsuite/clusterj/model/DynamicStringPKs.java new file mode 100644 index 000000000000..e37d51caf213 --- /dev/null +++ b/storage/ndb/clusterj/clusterj-test/src/main/java/testsuite/clusterj/model/DynamicStringPKs.java @@ -0,0 +1,105 @@ +/* + * Copyright (c) 2011, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2021, 2021, Logical Clocks AB and/or its affiliates. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License, version 2.0, + * as published by the Free Software Foundation. + * + * This program is also distributed with certain software (including + * but not limited to OpenSSL) that is licensed under separate terms, + * as designated in a particular file or component or in included license + * documentation. The authors of MySQL hereby grant you an additional + * permission to link the program and your derivative works with the + * separately licensed software that they have included with MySQL. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License, version 2.0, for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +*/ + +package testsuite.clusterj.model; + +import com.mysql.clusterj.DynamicObject; + +/** Schema + * +drop table if exists dynamicstringpks; +create table dynamicstringpks ( + key1 varchar(100) collate utf8_unicode_ci not null, + key2 varchar(100) collate utf8_unicode_ci not null, + key3 varchar(100) collate utf8_unicode_ci not null, + key4 bigint not null, + key5 varchar(100) collate utf8_unicode_ci not null, + key6 bigint not null, + key7 varchar(100) collate utf8_unicode_ci not null, + number int not null, + name varchar(10) not null, + primary key (key1, key2, key3, key4, key5, key6, key7) +) ENGINE=ndbcluster DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;; + + */ +public abstract class DynamicStringPKs extends DynamicObject { + + public String getKey1() { + return (String)get(0); + } + public String getKey2() { + return (String)get(1); + } + public String getKey3() { + return (String)get(2); + } + public Integer getKey4() { + return (Integer)get(3); + } + public String getKey5() { + return (String)get(4); + } + public Integer getKey6() { + return (Integer)get(5); + } + public String getKey7() { + return (String)get(6); + } + public void setKey1(String str) { + set(0, str); + } + public void setKey2(String str) { + set(1, str); + } + public void setKey3(String str) { + set(2, str); + } + public void setKey4(int val) { + set(3, val); + } + public void setKey5(String str) { + set(4, str); + } + public void setKey6(int val) { + set(5, val); + } + public void setKey7(String str) { + set(6, str); + } + + public int getNumber() { + return (Integer)get(7); + } + public void setNumber(int value) { + set(7, value); + } + + public String getName() { + return (String)get(8); + } + public void setName(String value) { + set(8, value); + } +} diff --git a/storage/ndb/clusterj/clusterj-test/src/main/resources/schema.sql b/storage/ndb/clusterj/clusterj-test/src/main/resources/schema.sql index 97f24ba3cc2e..680d377310c3 100644 --- a/storage/ndb/clusterj/clusterj-test/src/main/resources/schema.sql +++ b/storage/ndb/clusterj/clusterj-test/src/main/resources/schema.sql @@ -33,10 +33,15 @@ CREATE TABLE conversation_summary ( last_message_user_id bigint(11) NOT NULL, text_summary varchar(255) NOT NULL DEFAULT '', query_history_id bigint(20) NOT NULL DEFAULT '0', + key5 bigint(20) NOT NULL DEFAULT '0', + key6 varchar(100) NOT NULL DEFAULT '0', + key7 varchar(100) NOT NULL DEFAULT '0', + key8 varchar(100) NOT NULL DEFAULT '0', + key9 varchar(100) NOT NULL DEFAULT '0', answerer_id bigint(11) NOT NULL, viewed bit(1) NOT NULL, updated_at bigint(20) NOT NULL, - PRIMARY KEY (source_user_id,destination_user_id,query_history_id), + PRIMARY KEY (source_user_id,destination_user_id,query_history_id,key5,key6,key7,key8,key9), KEY IX_updated_at (updated_at) ) ENGINE=ndbcluster; @@ -137,6 +142,20 @@ create table varbinarytypes ( ) ENGINE=ndbcluster DEFAULT CHARSET=latin1; +drop table if exists dynamicstringpks; +create table dynamicstringpks ( + key1 VARCHAR(100) collate utf8_unicode_ci NOT NULL, + key2 VARCHAR(100) collate utf8_unicode_ci NOT NULL, + key3 VARCHAR(100) collate utf8_unicode_ci NOT NULL, + key4 INT NOT NULL, + key5 VARCHAR(100) collate utf8_unicode_ci NOT NULL, + key6 INT NOT NULL, + key7 VARCHAR(100) collate utf8_unicode_ci NOT NULL, + number INT NOT NULL, + name VARCHAR(10) NOT NULL, + PRIMARY KEY (key1, key2, key3, key4, key5, key6, key7) +) ENGINE=ndbcluster DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci; + drop table if exists binarypk; create table binarypk ( id binary(255) primary key not null, diff --git a/storage/ndb/src/ndbapi/Ndb.cpp b/storage/ndb/src/ndbapi/Ndb.cpp index a89c936ae3ef..c8b3326067c9 100644 --- a/storage/ndb/src/ndbapi/Ndb.cpp +++ b/storage/ndb/src/ndbapi/Ndb.cpp @@ -490,18 +490,19 @@ Ndb::computeHash(Uint32 *retval, sumlen += len; } - if (!buf) + while (true) { - bufLen = sumlen; - bufLen += sizeof(Uint64); /* add space for potential alignment */ - buf = malloc(bufLen); - if (unlikely(buf == 0)) - return 4000; - malloced_buf = buf; /* Remember to free */ - assert(bufLen > sumlen); - } + if (buf == nullptr) + { + bufLen = sumlen; + bufLen += sizeof(Uint64); /* add space for potential alignment */ + buf = malloc(bufLen); + if (unlikely(buf == 0)) + return 4000; + malloced_buf = buf; /* Remember to free */ + assert(bufLen > sumlen); + } - { /* Get 64-bit aligned ptr required for hashing */ assert(bufLen != 0); UintPtr org = UintPtr(buf); @@ -510,8 +511,10 @@ Ndb::computeHash(Uint32 *retval, buf = (void*)use; bufLen -= Uint32(use - org); - if (unlikely(sumlen > bufLen)) - goto ebuftosmall; + if (likely(sumlen <= bufLen)) + break; + require(malloced_buf == nullptr); + buf = nullptr; } pos= (unsigned char*) buf; @@ -582,8 +585,11 @@ Ndb::computeHash(Uint32 *retval, elentosmall: return 4277; +/** + * No longer used ebuftosmall: return 4278; + */ emalformedstring: if (malloced_buf)