Add primary key to selectors table #10586

zhangbutao · 2022-01-13T09:14:05Z

Percona-XtraDB-Cluster prohibit insert on table without primary key. And if adding primary key also help to select specific row.
Fixes #10585

Documentation

(x) No documentation is needed.

Release notes

(x) No release notes entries required.

cla-bot · 2022-01-13T09:14:07Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: zhangbutao.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

kokosing · 2022-01-13T11:00:24Z

...source-group-managers/src/main/java/io/trino/plugin/resourcegroups/db/ResourceGroupsDao.java

@@ -69,6 +69,7 @@
    List<SelectorRecord> getSelectors(@Bind("environment") String environment);

    @SqlUpdate("CREATE TABLE IF NOT EXISTS selectors (\n" +
+            "  id BIGINT NOT NULL AUTO_INCREMENT PRIMARY KEY,\n" +


I think we should use https://flywaydb.org/ to handle the schema changes. Also this is very vendor specific syntax. I am not sure what RDBMS vendors we support here.

See in-flight PR #9812 for Flyway.

I beleive we plan to support Postgres, MySQL and Oracle only.

I have changed the syntax of primary key to make more generic. I don't know if it's a reasonable fix here, but i see resource_groups table also has similar primary key syntax.

cla-bot · 2022-01-13T11:41:41Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

findepi · 2022-01-13T16:02:33Z

...in/trino-resource-group-managers/src/main/resources/db/migration/mysql/V3__add_selectors.sql

@@ -1,10 +1,12 @@
 CREATE TABLE IF NOT EXISTS selectors (
+     id BIGINT NOT NULL AUTO_INCREMENT,


I believe these files should never be mutated. @hashhar can we have a test?
For example use previous version to populate the DB state and use current to update it?

Firstly, these tables will not be changed after trino first startup unless updated artificially. And if we update table by adding primary key, mysql will automatically update the historical rows with adding the primary key value. So , there is nothing effect on previous version DB state.

The whole https://github.com/trinodb/trino/tree/master/plugin/trino-resource-group-managers/src/main/resources/db/migration/mysql is used with Flyway, it's a reply-log of DB schema changes, and so individual changes shouldn't change.

@hashhar does Flyway record checksum of the changes files, would it detect an accidental modification?

does Flyway record checksum of the changes files, would it detect an accidental modification?

It does.

What happens then?

does Flyway record checksum of the changes files, would it detect an accidental modification?

It does.

@aalbu we need a test ensuring no accidental modifications are done.
Could be as benign as someone reformatting large number of files, or Flyway changes checksum result in a corner case. #10586 (comment) would catch any such issue, let's plan on doing it.

cc @hashhar

zhangbutao · 2022-01-24T08:37:39Z

If adding primary key is a reasonable optimization, how to do it? Should i add a new sql file named like V6__add_primary_key_selectors.sql with contents alter table selectors add id int unsigned not Null auto_increment primary key ? cc @findepi @hashhar

hashhar · 2022-01-24T09:37:51Z

@zhangbutao Rebase on current master to include changes merged in #9812.

Add all migrations as new files (and maybe for all supported databases - wdyt @nineinchnick @aalbu ), also notice that now only Flyway is used for managing database state so you don't need the changes to the Jdbi objects.

Separate we'll look into adding tests which ensure that changes to older migrations are caught.

findepi · 2022-01-24T10:18:33Z

Add all migrations as new files (and maybe for all supported databases - wdyt @nineinchnick @aalbu ),

👍

now only Flyway is used for managing database state so you don't need the changes to the Jdbi objects.

do we still need those jdbi annotations?

Separate we'll look into adding tests which ensure that changes to older migrations are caught.

@nineinchnick @aalbu @hashhar do we have an issue?

aalbu · 2022-01-25T02:48:34Z

Separate we'll look into adding tests which ensure that changes to older migrations are caught.

@nineinchnick @aalbu @hashhar do we have an issue?

#10776

aalbu · 2022-01-25T02:49:49Z

Add all migrations as new files (and maybe for all supported databases - wdyt @nineinchnick @aalbu )

Agreed, we should mirror the changes for all supported databases.

cla-bot · 2022-01-25T07:33:24Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

zhangbutao · 2022-01-25T07:49:02Z

I have added new migrations files for all supported databases and no changes to the Jdbi objects. What else need I do? I have sent cla email to [email protected] , please check. thx
cc @hashhar @findepi @aalbu

hashhar

nit: add newlines to all migrations.

LGTM otherwise.

...oup-managers/src/main/resources/db/migration/postgresql/V5__add_primary_key_to_selectors.sql

hashhar · 2022-01-27T11:54:13Z

@cla-bot check

cc: @martint

cla-bot · 2022-01-27T11:54:14Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

cla-bot · 2022-01-27T11:54:15Z

The cla-bot has been summoned, and re-checked this pull request!

cla-bot · 2022-01-27T12:18:12Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

hashhar · 2022-02-08T11:28:25Z

@cla-bot check

cla-bot · 2022-02-08T11:28:27Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

cla-bot · 2022-02-08T11:28:28Z

The cla-bot has been summoned, and re-checked this pull request!

zhangbutao · 2022-02-23T02:31:16Z

Gentle ping. Can someone merge this request?

Some databases like Percona XtraDB Cluster prohibit inserts into tables without primary keys. This change also makes this table consistent with other tables in the resource groups database.

kokosing reviewed Jan 13, 2022

View reviewed changes

findepi reviewed Jan 13, 2022

View reviewed changes

aalbu mentioned this pull request Jan 26, 2022

Add tests to guard against accidental modification of Flyway migrations #10776

Open

nineinchnick approved these changes Jan 27, 2022

View reviewed changes

hashhar approved these changes Jan 27, 2022

View reviewed changes

...oup-managers/src/main/resources/db/migration/postgresql/V5__add_primary_key_to_selectors.sql Outdated Show resolved Hide resolved

cla-bot bot added the cla-signed label Feb 23, 2022

Add primary key to selectors table in resource groups database

74f6f8b

Some databases like Percona XtraDB Cluster prohibit inserts into tables without primary keys. This change also makes this table consistent with other tables in the resource groups database.

hashhar approved these changes Feb 23, 2022

View reviewed changes

hashhar merged commit 93bfb11 into trinodb:master Feb 23, 2022

mosabua mentioned this pull request Feb 24, 2022

Add Trino 372 release notes #11094

Merged

github-actions bot added this to the 372 milestone Feb 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add primary key to selectors table #10586

Add primary key to selectors table #10586

zhangbutao commented Jan 13, 2022 •

edited by hashhar

Loading

cla-bot bot commented Jan 13, 2022

kokosing Jan 13, 2022

hashhar Jan 13, 2022

zhangbutao Jan 13, 2022

cla-bot bot commented Jan 13, 2022

findepi Jan 13, 2022

zhangbutao Jan 14, 2022

findepi Jan 14, 2022

aalbu Jan 14, 2022

kokosing Jan 14, 2022

findepi Jan 15, 2022

zhangbutao commented Jan 24, 2022

hashhar commented Jan 24, 2022 •

edited

Loading

findepi commented Jan 24, 2022

aalbu commented Jan 25, 2022

aalbu commented Jan 25, 2022

cla-bot bot commented Jan 25, 2022

zhangbutao commented Jan 25, 2022

hashhar left a comment

hashhar commented Jan 27, 2022 •

edited

Loading

cla-bot bot commented Jan 27, 2022

cla-bot bot commented Jan 27, 2022

cla-bot bot commented Jan 27, 2022

hashhar commented Feb 8, 2022

cla-bot bot commented Feb 8, 2022

cla-bot bot commented Feb 8, 2022

zhangbutao commented Feb 23, 2022

		@@ -1,10 +1,12 @@
		CREATE TABLE IF NOT EXISTS selectors (
		id BIGINT NOT NULL AUTO_INCREMENT,

Add primary key to selectors table #10586

Add primary key to selectors table #10586

Conversation

zhangbutao commented Jan 13, 2022 • edited by hashhar Loading

Documentation

Release notes

cla-bot bot commented Jan 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cla-bot bot commented Jan 13, 2022

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

zhangbutao commented Jan 24, 2022

hashhar commented Jan 24, 2022 • edited Loading

findepi commented Jan 24, 2022

aalbu commented Jan 25, 2022

aalbu commented Jan 25, 2022

cla-bot bot commented Jan 25, 2022

zhangbutao commented Jan 25, 2022

hashhar left a comment

Choose a reason for hiding this comment

hashhar commented Jan 27, 2022 • edited Loading

cla-bot bot commented Jan 27, 2022

cla-bot bot commented Jan 27, 2022

cla-bot bot commented Jan 27, 2022

hashhar commented Feb 8, 2022

cla-bot bot commented Feb 8, 2022

cla-bot bot commented Feb 8, 2022

zhangbutao commented Feb 23, 2022

zhangbutao commented Jan 13, 2022 •

edited by hashhar

Loading

hashhar commented Jan 24, 2022 •

edited

Loading

hashhar commented Jan 27, 2022 •

edited

Loading