FB8-120: Added db_metadata options to create database command #972

dutow · 2019-02-27T10:54:57Z

JIRA ticket: https://jira.percona.com/browse/FB8-120

Reference commit: 2f08e3a
Reference commit: a21e84f
Reference commit: 5983ba3181a
Reference commit: 414870abeee

-------- 2f08e3a --------

Added new option db_metadata to the create database
CREATE DATABASE [IF NOT EXISTS] db_name DB_METADATA [=] metadata

The new option is stored in the db.opt file along with other database options like charset and read_only.
The options accepts any string value. If not specified, it writes empty string as the default value in the db.opt file.
To retrieve the option value for a particular database, the show command can be used as follows:
SHOW CREATE DATABASE db_name

Example Usage:

create database test2 charset utf8 read_only = true db_metadata '{'shard_name':'myshard_for_test2'}';
show create database test2;

Output:

+----------+--------------------------------------------------------------------------------------------------------------------------+
| Database | Create Database |
+----------+--------------------------------------------------------------------------------------------------------------------------+
| test2 | CREATE DATABASE test2 /*!40100 DEFAULT CHARACTER SET utf8 READ_ONLY DB_METADATA {'shard_name':'myshard_for_test2'} */ |
+----------+--------------------------------------------------------------------------------------------------------------------------+

percona-ysorokin

Mostly stylistic comments + one potential buffer overflow problem.

Please, also extend commit message with some info that in 8.0 database metadata is stored in Data Dictionary rather than in an .opt file.

percona-ysorokin · 2019-02-27T13:57:14Z

sql/dd/impl/types/schema_impl.cc

+  return true;
+}
+
+String_type const &Schema_impl::get_db_metadata() const { return m_metadata; }


Suggested change

String_type const &Schema_impl::get_db_metadata() const { return m_metadata; }

String_type const &Schema_impl::get_db_metadata() const noexcept { return m_metadata; }

percona-ysorokin · 2019-02-27T13:58:21Z

sql/dd/impl/types/schema_impl.h

@@ -127,6 +128,9 @@ class Schema_impl : public Entity_object_impl, public Schema {

  virtual bool set_options_raw(const String_type &options_raw);

+  virtual bool set_db_metadata(const String_type &metadata) override;
+  virtual String_type const &get_db_metadata() const override;


Suggested change

virtual String_type const &get_db_metadata() const override;

virtual String_type const &get_db_metadata() const noexcept override;

percona-ysorokin · 2019-02-27T13:59:52Z

sql/dd/impl/types/schema_impl.h

            id(), name().c_str(), m_default_collation_id, m_created,
-            m_last_altered);
+            m_last_altered, m_metadata.c_str());


Potential buffer overflow here with very long metadata.
Consider snprintf().

percona-ysorokin · 2019-02-27T14:00:37Z

sql/dd/types/schema.h

@@ -110,6 +110,9 @@ class Schema : virtual public Entity_object {

  virtual bool set_options_raw(const String_type &options_raw) = 0;

+  virtual bool set_db_metadata(const String_type &metadata) = 0;
+  virtual String_type const &get_db_metadata() const = 0;


Suggested change

virtual String_type const &get_db_metadata() const = 0;

virtual String_type const &get_db_metadata() const noexcept = 0;

percona-ysorokin · 2019-02-27T14:03:09Z

sql/sql_yacc.yy

+                boost::property_tree::json_parser::read_json(is,
+                                                             db_metadata_root);
+              }
+              catch (std::exception)


Suggested change

catch (std::exception)

catch (const std::exception&)

percona-ysorokin · 2019-02-27T14:04:55Z

sql/dd/impl/types/schema_impl.h

@@ -66,7 +66,8 @@ class Schema_impl : public Entity_object_impl, public Schema {
      : m_created(0),
        m_last_altered(0),
        m_default_collation_id(INVALID_OBJECT_ID),
-        m_options(new Properties_impl()) {}
+        m_options(new Properties_impl()),
+        m_metadata("") {}


Suggested change

m_metadata("") {}

m_metadata() {}

percona-ysorokin · 2019-02-27T14:08:23Z

sql/dd/dd_schema.cc

  // Create dd::Schema object.
  std::unique_ptr<dd::Schema> schema(dd::create_object<dd::Schema>());

  // Set schema name and collation id.
  schema->set_name(schema_name);
  DBUG_ASSERT(charset_info);
  schema->set_default_collation_id(charset_info->number);
+  schema->set_db_metadata(db_metadata ? db_metadata : "");


no implicit conversions

Suggested change

schema->set_db_metadata(db_metadata ? db_metadata : "");

schema->set_db_metadata(db_metadata != nullptr ? db_metadata : "");

laurynas-biveinis · 2019-02-28T13:05:33Z

mysql-test/t/db_metadata.test

@@ -0,0 +1,163 @@
+# Test per-database database-metadata attribute
+
+--disable_query_log


I don't think there is need to disable query log here, please check

laurynas-biveinis · 2019-02-28T13:06:18Z

mysql-test/t/db_metadata.test

+
+
+# cleanup
+--disable_warnings


Is the warning disable required here? What are the warnings?

laurynas-biveinis · 2019-02-28T13:08:23Z

sql/sql_yacc.yy

@@ -1236,6 +1239,7 @@ void warn_about_deprecated_national(THD *thd)
 %token<keyword> GTID_SYM
 %token<keyword> GTID_EXECUTED                 /* MYSQL */
 %token<keyword> SUPER_READ_ONLY_SYM           /* MYSQL */
+%token  DB_METADATA_SYM


8.0 has some attempts to have parser token ID (and resulting query digest) stability, please check: might be a good idea to skip 100 IDs and start a FB-specific section

The 3 tokens above this are also added by facebook - I simply added this one to the end of the existing list.

Are you sure? All three look like MySQL ones

There is a comment at line 1236: "Tokens from FB MySQL"

You are right, sorry. And there is no provision to skip IDs, thus everything already is as good as it could be

dutow · 2019-03-01T07:49:00Z

updated

percona-ysorokin

LGTM

lth · 2019-03-07T23:15:22Z

You might want to follow how the DB read only feature was done, because I believe by changing the schema, you need to bump some data dictionary version or something. The FIELD_OPTIONS column is meant to be a generic place to add key/values, and I'm wondering if that's a better place for this.

b9ec08a

dutow · 2019-04-03T17:41:43Z

Updated: I removed the additional columns from the dd, and used the options field. Tests pass now.

facebook-github-bot

@hermanlee has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

hermanlee

Code looks good overall. Just some small minor comments.

hermanlee · 2019-04-18T19:48:38Z

share/errmsg-utf8.txt

@@ -18728,6 +18728,15 @@ ER_RPL_FAILED_IN_RLI_INIT_INFO
 ER_SEMISYNC_FORCE_SHUTDOWN
   eng "Force shutdown: Semi-sync master is being switched off while there are active un-acked transactions"

+ER_DB_METADATA_TOO_LONG


This should replace ER_PLACEHOLDER_50063.

hermanlee · 2019-04-18T19:48:51Z

share/errmsg-utf8.txt

+ER_DB_METADATA_TOO_LONG
+  eng "Metadata for the database is too long. Max length is %d bytes"
+
+ER_DB_METADATA_INVALID_JSON


Replace ER_PLACEHOLDER_50064

hermanlee · 2019-04-18T19:49:13Z

share/errmsg-utf8.txt

+ER_DB_METADATA_INVALID_JSON
+  eng "Invalid JSON for DB_METADATA attribute: %s."
+
+ER_DB_METADATA_READ_ERROR


Replace ER_PLACEHOLDER_50065

hermanlee · 2019-04-23T17:30:29Z

sql/dd/impl/types/schema_impl.h

@@ -127,6 +128,9 @@ class Schema_impl : public Entity_object_impl, public Schema {

  virtual bool set_options_raw(const String_type &options_raw);

+  virtual bool set_db_metadata(const String_type &metadata) override;


We also compile with -Winconsistent-missing-override, so adding this override cause the compiler to warn about the rest of the object:

In file included from ../../sql/dd/impl/bootstrapper.cc:58: ../../sql/dd/impl/types/schema_impl.h:75:31: error: 'object_table' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] virtual const Object_table &object_table() const; ../../sql/dd/impl/types/weak_object_impl.h:50:31: note: overridden virtual function is here virtual const Object_table &object_table() const = 0; ^ In file included from ../../sql/dd/impl/bootstrapper.cc:58: ../../sql/dd/impl/types/schema_impl.h:77:16: error: 'validate' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] virtual bool validate() const; ^ ../../sql/dd/impl/types/weak_object_impl.h:52:16: note: overridden virtual function is here virtual bool validate() const = 0; ^

hermanlee · 2019-04-23T18:24:34Z

sql/sql_const.h

@@ -286,6 +286,9 @@ static const ulong EVENT_DEF_CACHE_MIN = 256;
 */
 #define MAX_TIME_ZONE_NAME_LENGTH (NAME_LEN + 1)

+#define DB_METADATA_MAX_LENGTH 1024 /* Max length for the db-metadata option \


Seems like "*/" should also fit onto this line too?

hermanlee · 2019-04-23T20:23:14Z

sql/dd/upgrade/schema.cc

@@ -155,7 +155,7 @@ bool migrate_schema_to_dd(THD *thd, const char *dbname) {
  // Disable autocommit option
  Disable_autocommit_guard autocommit_guard(thd);

-  if (dd::create_schema(thd, schema_name, schema_charset)) {
+  if (dd::create_schema(thd, schema_name, schema_charset, nullptr)) {


In 5.6, we store the db_metadata information in the db_opt file using the keyword db-metadata. Since there are in-place upgrade paths planned to 8.0, it should be fine to have nullptr here.

hermanlee · 2019-04-23T20:39:26Z

sql/dd/impl/types/schema_impl.cc

@@ -106,6 +106,25 @@ bool Schema_impl::set_options_raw(const String_type &options_raw) {
  return false;
 }

+bool Schema_impl::set_db_metadata(const String_type &metadata) {
+  if (metadata.empty()) {
+    if (options().exists("db_metadata")) {


Can "db_metadata" be made into a constant string, similar to read_only_options_key?

hermanlee

Requesting changes for the db_metadata key.

JIRA ticket: https://jira.percona.com/browse/FB8-120 Reference commit: 2f08e3a Reference commit: a21e84f Reference commit: 5983ba3181a Reference commit: 414870abeee In 8.0, db_metadata is stored in the data dictionary instead of opt files. -------- 2f08e3a -------- Added new option db_metadata to the create database `CREATE DATABASE [IF NOT EXISTS] db_name DB_METADATA [=] metadata` The new option is stored in the db.opt file along with other database options like charset and read_only. The options accepts any string value. If not specified, it writes empty string as the default value in the db.opt file. To retrieve the option value for a particular database, the show command can be used as follows: `SHOW CREATE DATABASE db_name` Example Usage: create database test2 charset utf8 read_only = true db_metadata '{\'shard_name\':\'myshard_for_test2\'}'; show create database test2; Output: +----------+--------------------------------------------------------------------------------------------------------------------------+ | Database | Create Database | +----------+--------------------------------------------------------------------------------------------------------------------------+ | test2 | CREATE DATABASE `test2` /*!40100 DEFAULT CHARACTER SET utf8 READ_ONLY DB_METADATA {'shard_name':'myshard_for_test2'} */ | +----------+--------------------------------------------------------------------------------------------------------------------------+

facebook-github-bot · 2019-04-29T04:34:24Z

@dutow has updated the pull request. Re-import the pull request

dutow · 2019-04-30T07:40:07Z

Updated.

facebook-github-bot

@hermanlee has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: JIRA ticket: https://jira.percona.com/browse/FB8-120 Reference commit: 2f08e3a Reference commit: a21e84f Reference commit: 5983ba3181a Reference commit: 414870abeee Added new option db_metadata to the create database `CREATE DATABASE [IF NOT EXISTS] db_name DB_METADATA [=] metadata` The new option is stored in the db.opt file along with other database options like charset and read_only. The options accepts any string value. If not specified, it writes empty string as the default value in the db.opt file. To retrieve the option value for a particular database, the show command can be used as follows: `SHOW CREATE DATABASE db_name` Example Usage: create database test2 charset utf8 read_only = true db_metadata '{\'shard_name\':\'myshard_for_test2\'}'; show create database test2; Output: +----------+--------------------------------------------------------------------------------------------------------------------------+ | Database | Create Database | +----------+--------------------------------------------------------------------------------------------------------------------------+ | test2 | CREATE DATABASE `test2` /*!40100 DEFAULT CHARACTER SET utf8 READ_ONLY DB_METADATA {'shard_name':'myshard_for_test2'} */ | +----------+--------------------------------------------------------------------------------------------------------------------------+ Pull Request resolved: #972 Reviewed By: yizhang82 Differential Revision: D14883632 Pulled By: yizhang82 fbshipit-source-id: 3af0fc8

Summary: JIRA ticket: https://jira.percona.com/browse/FB8-120 Reference commit: 2f08e3a Reference commit: a21e84f Reference commit: 5983ba3181a Reference commit: 414870abeee Added new option db_metadata to the create database `CREATE DATABASE [IF NOT EXISTS] db_name DB_METADATA [=] metadata` The new option is stored in the db.opt file along with other database options like charset and read_only. The options accepts any string value. If not specified, it writes empty string as the default value in the db.opt file. To retrieve the option value for a particular database, the show command can be used as follows: `SHOW CREATE DATABASE db_name` Example Usage: create database test2 charset utf8 read_only = true db_metadata '{\'shard_name\':\'myshard_for_test2\'}'; show create database test2; Output: +----------+--------------------------------------------------------------------------------------------------------------------------+ | Database | Create Database | +----------+--------------------------------------------------------------------------------------------------------------------------+ | test2 | CREATE DATABASE `test2` /*!40100 DEFAULT CHARACTER SET utf8 READ_ONLY DB_METADATA {'shard_name':'myshard_for_test2'} */ | +----------+--------------------------------------------------------------------------------------------------------------------------+ Pull Request resolved: #972 Reviewed By: yizhang82 Differential Revision: D14883632 Pulled By: yizhang82 fbshipit-source-id: 84fda3f

…ok#972) Summary: JIRA ticket: https://jira.percona.com/browse/FB8-120 Reference commit: facebook@2f08e3a Reference commit: facebook@a21e84f Reference commit: facebook@5983ba3181a Reference commit: facebook@414870abeee Added new option db_metadata to the create database `CREATE DATABASE [IF NOT EXISTS] db_name DB_METADATA [=] metadata` The new option is stored in the db.opt file along with other database options like charset and read_only. The options accepts any string value. If not specified, it writes empty string as the default value in the db.opt file. To retrieve the option value for a particular database, the show command can be used as follows: `SHOW CREATE DATABASE db_name` Example Usage: create database test2 charset utf8 read_only = true db_metadata '{\'shard_name\':\'myshard_for_test2\'}'; show create database test2; Output: +----------+--------------------------------------------------------------------------------------------------------------------------+ | Database | Create Database | +----------+--------------------------------------------------------------------------------------------------------------------------+ | test2 | CREATE DATABASE `test2` /*!40100 DEFAULT CHARACTER SET utf8 READ_ONLY DB_METADATA {'shard_name':'myshard_for_test2'} */ | +----------+--------------------------------------------------------------------------------------------------------------------------+ Pull Request resolved: facebook#972 Reviewed By: yizhang82 Differential Revision: D14883632 Pulled By: yizhang82 fbshipit-source-id: 84fda3f

…ok#972) (facebook#972) Summary: JIRA ticket: https://jira.percona.com/browse/FB8-120 Reference commit: facebook@2f08e3a Reference commit: facebook@a21e84f Reference commit: facebook@5983ba3181a Reference commit: facebook@414870abeee Added new option db_metadata to the create database `CREATE DATABASE [IF NOT EXISTS] db_name DB_METADATA [=] metadata` The new option is stored in the db.opt file along with other database options like charset and read_only. The options accepts any string value. If not specified, it writes empty string as the default value in the db.opt file. To retrieve the option value for a particular database, the show command can be used as follows: `SHOW CREATE DATABASE db_name` Example Usage: create database test2 charset utf8 read_only = true db_metadata '{\'shard_name\':\'myshard_for_test2\'}'; show create database test2; Output: +----------+--------------------------------------------------------------------------------------------------------------------------+ | Database | Create Database | +----------+--------------------------------------------------------------------------------------------------------------------------+ | test2 | CREATE DATABASE `test2` /*!40100 DEFAULT CHARACTER SET utf8 READ_ONLY DB_METADATA {'shard_name':'myshard_for_test2'} */ | +----------+--------------------------------------------------------------------------------------------------------------------------+ Pull Request resolved: facebook#972 Differential Revision: D14883632 Pulled By: yizhang82

…a#972) (percona#972) Summary: JIRA ticket: https://jira.percona.com/browse/FB8-120 Reference commit: facebook/mysql-5.6@2f08e3a Reference commit: facebook/mysql-5.6@a21e84f Reference commit: facebook/mysql-5.6@5983ba3181a Reference commit: facebook/mysql-5.6@414870abeee Added new option db_metadata to the create database `CREATE DATABASE [IF NOT EXISTS] db_name DB_METADATA [=] metadata` The new option is stored in the db.opt file along with other database options like charset and read_only. The options accepts any string value. If not specified, it writes empty string as the default value in the db.opt file. To retrieve the option value for a particular database, the show command can be used as follows: `SHOW CREATE DATABASE db_name` Example Usage: create database test2 charset utf8 read_only = true db_metadata '{\'shard_name\':\'myshard_for_test2\'}'; show create database test2; Output: +----------+--------------------------------------------------------------------------------------------------------------------------+ | Database | Create Database | +----------+--------------------------------------------------------------------------------------------------------------------------+ | test2 | CREATE DATABASE `test2` /*!40100 DEFAULT CHARACTER SET utf8 READ_ONLY DB_METADATA {'shard_name':'myshard_for_test2'} */ | +----------+--------------------------------------------------------------------------------------------------------------------------+ Pull Request resolved: facebook/mysql-5.6#972 Differential Revision: D14883632 Pulled By: yizhang82

…ok#972) (facebook#972) Summary: JIRA ticket: https://jira.percona.com/browse/FB8-120 Reference commit: facebook@2f08e3a Reference commit: facebook@a21e84f Reference commit: facebook@5983ba3181a Reference commit: facebook@414870abeee Added new option db_metadata to the create database `CREATE DATABASE [IF NOT EXISTS] db_name DB_METADATA [=] metadata` The new option is stored in the db.opt file along with other database options like charset and read_only. The options accepts any string value. If not specified, it writes empty string as the default value in the db.opt file. To retrieve the option value for a particular database, the show command can be used as follows: `SHOW CREATE DATABASE db_name` Example Usage: create database test2 charset utf8 read_only = true db_metadata '{\'shard_name\':\'myshard_for_test2\'}'; show create database test2; Output: +----------+--------------------------------------------------------------------------------------------------------------------------+ | Database | Create Database | +----------+--------------------------------------------------------------------------------------------------------------------------+ | test2 | CREATE DATABASE `test2` /*!40100 DEFAULT CHARACTER SET utf8 READ_ONLY DB_METADATA {'shard_name':'myshard_for_test2'} */ | +----------+--------------------------------------------------------------------------------------------------------------------------+ Pull Request resolved: facebook#972 Differential Revision: D14883632 Pulled By: yizhang82

facebook-github-bot added the CLA Signed label Feb 27, 2019

dutow changed the title ~~FB8-120: Added db_metadata options to create database command~~ WIP FB8-120: Added db_metadata options to create database command Feb 27, 2019

percona-ysorokin suggested changes Feb 27, 2019

View reviewed changes

laurynas-biveinis suggested changes Feb 28, 2019

View reviewed changes

percona-ysorokin approved these changes Mar 4, 2019

View reviewed changes

dutow changed the title ~~WIP FB8-120: Added db_metadata options to create database command~~ FB8-120: Added db_metadata options to create database command Apr 8, 2019

facebook-github-bot reviewed Apr 10, 2019

View reviewed changes

hermanlee reviewed Apr 23, 2019

View reviewed changes

hermanlee requested changes Apr 23, 2019

View reviewed changes

facebook-github-bot reviewed Apr 30, 2019

View reviewed changes

hermanlee approved these changes May 9, 2019

View reviewed changes

hermanlee closed this Jun 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FB8-120: Added db_metadata options to create database command #972

FB8-120: Added db_metadata options to create database command #972

dutow commented Feb 27, 2019

percona-ysorokin left a comment

percona-ysorokin Feb 27, 2019

percona-ysorokin Feb 27, 2019

percona-ysorokin Feb 27, 2019

percona-ysorokin Feb 27, 2019

percona-ysorokin Feb 27, 2019

percona-ysorokin Feb 27, 2019

percona-ysorokin Feb 27, 2019

laurynas-biveinis Feb 28, 2019

laurynas-biveinis Feb 28, 2019

laurynas-biveinis Feb 28, 2019

dutow Mar 1, 2019

laurynas-biveinis Mar 1, 2019

dutow Mar 1, 2019

laurynas-biveinis Mar 1, 2019

dutow commented Mar 1, 2019

percona-ysorokin left a comment

lth commented Mar 7, 2019

dutow commented Apr 3, 2019

facebook-github-bot left a comment

hermanlee left a comment

hermanlee Apr 18, 2019

hermanlee Apr 18, 2019

hermanlee Apr 18, 2019

hermanlee Apr 23, 2019

hermanlee Apr 23, 2019

hermanlee Apr 23, 2019

hermanlee Apr 23, 2019

hermanlee left a comment

facebook-github-bot commented Apr 29, 2019

dutow commented Apr 30, 2019

facebook-github-bot left a comment

	String_type const &Schema_impl::get_db_metadata() const { return m_metadata; }
	String_type const &Schema_impl::get_db_metadata() const noexcept { return m_metadata; }

	virtual String_type const &get_db_metadata() const override;
	virtual String_type const &get_db_metadata() const noexcept override;

	schema->set_db_metadata(db_metadata ? db_metadata : "");
	schema->set_db_metadata(db_metadata != nullptr ? db_metadata : "");

		@@ -0,0 +1,163 @@
		# Test per-database database-metadata attribute

		--disable_query_log

		@@ -127,6 +128,9 @@ class Schema_impl : public Entity_object_impl, public Schema {

		virtual bool set_options_raw(const String_type &options_raw);

		virtual bool set_db_metadata(const String_type &metadata) override;



		# cleanup
		--disable_warnings

FB8-120: Added db_metadata options to create database command #972

FB8-120: Added db_metadata options to create database command #972

Conversation

dutow commented Feb 27, 2019

percona-ysorokin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dutow commented Mar 1, 2019

percona-ysorokin left a comment

Choose a reason for hiding this comment

lth commented Mar 7, 2019

dutow commented Apr 3, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

hermanlee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hermanlee left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 29, 2019

dutow commented Apr 30, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment