3197 DB schema: change_properties to text #2561

nand0p · 2017-01-04T20:10:12Z

Alters change_properties.propery_value from String(1024) to Text

http://trac.buildbot.net/ticket/3197

mention-bot · 2017-01-04T20:10:13Z

@nand0p, thanks for your PR! By analyzing the history of the files in this pull request, we identified @djmitche, @tardyp and @rutsky to be potential reviewers.

tardyp · 2017-01-05T09:07:30Z

We need a migration unit test in order to be sure this works on all db types.

We also need a releasenote newsfragment stating that one need to do buildbot upgrade-master in order to upgrade to the new version of buildbot.

nand0p · 2017-01-06T20:59:08Z

The upgrade is working, but the test is not. i will keep working thru this.

before:
sqlite3 -line state.sqlite ".schema change_properties" ==> buildbot: CREATE TABLE change_properties ( ==> buildbot: changeid INTEGER NOT NULL, ==> buildbot: property_name VARCHAR(256) NOT NULL, ==> buildbot: property_value VARCHAR(1024) NOT NULL, ==> buildbot: FOREIGN KEY(changeid) REFERENCES changes (changeid) ==> buildbot: );

after:
sqlite3 -line state.sqlite ".schema change_properties" ==> buildbot: CREATE TABLE change_properties ( ==> buildbot: changeid INTEGER NOT NULL, ==> buildbot: property_name VARCHAR(256) NOT NULL, ==> buildbot: property_value TEXT NOT NULL, ==> buildbot: FOREIGN KEY(changeid) REFERENCES changes (changeid) ==> buildbot: );

nand0p · 2017-01-09T21:00:23Z

@tardyp It looks like this issue may only be occuring with postgresql9.4 and latest sqlalchemy, as the CI tests show.

After silently executing, the data_type remains unchanged:

==> buildbot: 2017-01-09 20:19:33+0000 [-] Log opened.
==> buildbot: 2017-01-09 20:19:33+0000 [-] checking basedir
==> buildbot: 2017-01-09 20:19:33+0000 [-] checking for running master
==> buildbot: 2017-01-09 20:19:33+0000 [-] checking master.cfg
==> buildbot: 2017-01-09 20:19:33+0000 [-] Loading configuration from '/tmp/bb-mig-1483993162/master.cfg'
==> buildbot: 2017-01-09 20:19:33+0000 [-] upgrading basedir
==> buildbot: 2017-01-09 20:19:33+0000 [-] creating /tmp/bb-mig-1483993162/master.cfg.sample
==> buildbot: 2017-01-09 20:19:33+0000 [-] upgrading database (postgresql://buildbot:xxxx@localhost/bbdb)
==> buildbot: 2017-01-09 20:19:33+0000 [-] Setting up database with URL 'postgresql://buildbot:xxxx@localhost/bbdb'
==> buildbot: 2017-01-09 20:19:33+0000 [-] using SQLAlchemy-Migrate version 0.10.0
==> buildbot: 2017-01-09 20:19:33+0000 [-] migrating schema version 47 -> 48
==> buildbot: 2017-01-09 20:19:33+0000 [-] upgrade complete
==> buildbot: 2017-01-09 20:19:33+0000 [-] Main loop terminated.
==> buildbot: test db schema:
==> buildbot: column_name | data_type | character_maximum_length
==> buildbot: ----------------+-------------------+--------------------------
==> buildbot: changeid | integer |
==> buildbot: property_name | character varying | 256
==> buildbot: property_value | character varying | 1024

tardyp · 2017-01-09T22:24:12Z

@nand0p indeed, this is the pain of the db migrations it never works easily for the 3 dbs we support. That's why we require the CI to pass for those 3 :-}

There is already a change property to text migration

https://github.com/buildbot/buildbot/blob/eight/master/buildbot/db/migrate/versions/023_increase_comments_property_lengths.py

tardyp · 2017-01-09T22:26:08Z

master/buildbot/test/unit/test_db_migrate_versions_048_change_properties_to_text.py

+            metadata.bind = conn
+
+            change_properties = sautils.Table('change_properties', metadata, autoload=True)
+            self.assertIsInstance(change_properties.c.property_value.type, sa.Text)


I think for sqlite, we dont really care as TEXT and VARCHAR are stored the same anyway. We rather need to check that the property can store > 1024 string.

makes sense. i will add this test.

I dont think you added that test

tardyp · 2017-01-10T12:42:41Z

master/buildbot/newsfragments/db_schema_change_properties.bugfix

@@ -0,0 +1 @@
+Alters change properties from String to Text. Requires upgrade master. (:bug:`3197`)


Please change to more user oriented message:

Change properties can now be of unlimited size (previously was max 1024 bytes)

tardyp

need a test for >1024 property
Need better relnotes

nand0p · 2017-01-11T20:27:35Z

I can't seem to replicate the mysql issue seen in CI. @tardyp should I ignore the insert test for mysql instead?

==> buildbot: 2017-01-11 20:21:27+0000 [-] Log opened.
==> buildbot: 2017-01-11 20:21:27+0000 [-] checking basedir
==> buildbot: 2017-01-11 20:21:27+0000 [-] checking for running master
==> buildbot: 2017-01-11 20:21:27+0000 [-] checking master.cfg
==> buildbot: 2017-01-11 20:21:27+0000 [-] Loading configuration from '/tmp/bb-mig-1484166077/master.cfg'
==> buildbot: 2017-01-11 20:21:27+0000 [-] upgrading basedir
==> buildbot: 2017-01-11 20:21:27+0000 [-] creating /tmp/bb-mig-1484166077/master.cfg.sample
==> buildbot: 2017-01-11 20:21:27+0000 [-] upgrading database (mysql://buildbot:xxxx@localhost/bbdb)
==> buildbot: 2017-01-11 20:21:27+0000 [-] Setting up database with URL 'mysql://buildbot:xxxx@localhost/bbdb'
==> buildbot: 2017-01-11 20:21:27+0000 [-] using SQLAlchemy-Migrate version 0.10.0
==> buildbot: 2017-01-11 20:21:27+0000 [-] migrating schema version 47 -> 48
==> buildbot: 2017-01-11 20:21:27+0000 [-] upgrade complete
==> buildbot: 2017-01-11 20:21:27+0000 [-] Main loop terminated.
==> buildbot: test db schema:
==> buildbot: Field Type Null Key Default Extra
==> buildbot: changeid int(11) NO MUL NULL
==> buildbot: property_name varchar(256) NO NULL
==> buildbot: property_value text NO NULL
==> buildbot: mysqld Ver 5.7.17 for Linux on x86_64 (MySQL Community Server (GPL))
==> buildbot: run tests:
==> buildbot: # On branch 3197_change_properties_to_text
==> buildbot: nothing to commit, working directory clean
==> buildbot: buildbot.test.unit.test_db_migrate_versions_048_change_properties_to_text
==> buildbot: test_update ... [OK]

UPDATE: i found the issue and am working thru it now.

nand0p · 2017-01-11T21:29:07Z

@tardyp ready_for_review

tardyp · 2017-01-11T22:23:00Z

master/buildbot/test/unit/test_db_migrate_versions_048_change_properties_to_text.py

+            metadata = sa.MetaData()
+            metadata.bind = conn
+            random_length = 4096
+            random_string = ''.join(choice(ascii_lowercase) for byte in range(random_length))


well "x"*4096 would have been enough..

tardyp · 2017-01-11T22:24:17Z

master/buildbot/test/unit/test_db_migrate_versions_048_change_properties_to_text.py

+            # Using raw sql due to error via sa
+            # q = conn.execute(sa.select([sa.func.length(change_properties.c.property_value)]).where(changeid==1))
+            q = conn.execute("select length(property_value) from change_properties where changeid = 1")
+            [self.assertEqual(q_length[0], random_length) for q_length in q]


I would have prefered you just use the db api to write and read the property back and forth and make sure its not truncated

nand0p · 2017-01-13T22:46:21Z

@tardyp You're right, there does appear to be an issue with sqlalchemy truncating text fields when they contain large strings. To compensate, I kept the type as string(varchar), but upped to 14336 (1024*14), to deal with mysql limits. http://stackoverflow.com/questions/13506832/what-is-the-mysql-varchar-max-size

should we worry about truncation for any of these?

./master/buildbot/db/model.py: sa.Column('value', sa.Text, nullable=False),
./master/buildbot/db/model.py: sa.Column('state_string', sa.Text, nullable=False),
./master/buildbot/db/model.py: sa.Column('state_string', sa.Text, nullable=False),
./master/buildbot/db/model.py: sa.Column('urls_json', sa.Text, nullable=False),
./master/buildbot/db/model.py: sa.Column('name', sa.Text, nullable=False),
./master/buildbot/db/model.py: sa.Column('property_value', sa.Text, nullable=False),
./master/buildbot/db/model.py: sa.Column('parent_relationship', sa.Text),
./master/buildbot/db/model.py: sa.Column('name', sa.Text, nullable=False),
./master/buildbot/db/model.py: sa.Column('comments', sa.Text, nullable=False),
./master/buildbot/db/model.py: sa.Column('patch_base64', sa.Text, nullable=False),
./master/buildbot/db/model.py: sa.Column('patch_author', sa.Text, nullable=False),
./master/buildbot/db/model.py: sa.Column('patch_comment', sa.Text, nullable=False),
./master/buildbot/db/model.py: sa.Column('subdir', sa.Text),
./master/buildbot/db/model.py: sa.Column('name', sa.Text, nullable=False),
./master/buildbot/db/model.py: sa.Column('name', sa.Text, nullable=False),
./master/buildbot/db/model.py: sa.Column('description', sa.Text, nullable=True),
./master/buildbot/db/model.py: sa.Column('name', sa.Text, nullable=False),
./master/buildbot/db/model.py: sa.Column("value_json", sa.Text, nullable=False),
./master/buildbot/db/model.py: sa.Column('name', sa.Text, nullable=False),

tardyp

If mysql as limitation, we should document that limitation, but we shouldn't workaround like this.
what if pg users want bigger properties?

tardyp · 2017-01-16T17:10:27Z

master/buildbot/db/model.py

@@ -314,7 +314,7 @@ class Model(base.DBConnectorComponent):
                  nullable=False),
        sa.Column('property_name', sa.String(256), nullable=False),
        # JSON-encoded tuple of (value, source)
-        sa.Column('property_value', sa.String(1024), nullable=False),
+        sa.Column('property_value', sa.String(14336), nullable=False),


I dont think it is a good idea. We should keep Text, and be coherent with the other property table.

Alters change_properties.propery_value from String(1024) to Text http://trac.buildbot.net/ticket/3197

nand0p · 2017-01-18T16:39:36Z

@tardyp ready_for_review

tardyp · 2017-01-18T17:05:59Z

I dont see the documentation about mysql limitation.

Can you document it here (with reference on mysql documentation on why it truncates the text rows)
https://github.com/tardyp/buildbot/blob/master/master/docs/manual/cfg-global.rst#L66

nand0p · 2017-01-18T17:40:34Z

@tardyp The mysql limitation only applies to varchar, not text, so it is not relevant anymore now that the table column is altered to text.

The truncation I was seeing apparently was a result of evaluating the unicode result array, instead of string. this is fixed and test works as expected.

example unicode query result array i was previously erroneously evaluating:

(u'szejzjapotmsjlmyeeixifhpnrwwavtqkvycywhaujatezhrfgzzoiuwocbfzyqydxcgnmcbpczhoabhqkwejwfksgqondbuihfeqjslrixrshhgsgxunfjticykltbuguxwtegxxfbodtusabqe ... (65238 characters truncated) ... wcppgackzpxvasrfjdiazbxhvtjzqggzlueeoiwntoeiovvhorgbtvuhneojtkkhvioadgcwrozkfqairporuxmybewweigaapzyhlhcuthearlhwotqpzidulftveztyuhldahokuhaxrhjxysaq',)

tardyp · 2017-01-19T20:55:55Z

thanks!

nand0p force-pushed the 3197_change_properties_to_text branch 2 times, most recently from 43cb8c0 to 041303b Compare January 4, 2017 20:51

tardyp added needs relnotes needs tests labels Jan 5, 2017

nand0p force-pushed the 3197_change_properties_to_text branch 3 times, most recently from 55101c8 to a4e0326 Compare January 5, 2017 20:18

tardyp reviewed Jan 9, 2017

View reviewed changes

nand0p force-pushed the 3197_change_properties_to_text branch from a4e0326 to f685fc4 Compare January 9, 2017 22:40

tardyp reviewed Jan 10, 2017

View reviewed changes

tardyp requested changes Jan 10, 2017

View reviewed changes

nand0p force-pushed the 3197_change_properties_to_text branch 8 times, most recently from c3ff87a to a0a6a28 Compare January 11, 2017 15:10

nand0p force-pushed the 3197_change_properties_to_text branch 2 times, most recently from 75a7741 to c83d2c9 Compare January 11, 2017 21:04

tardyp reviewed Jan 11, 2017

View reviewed changes

nand0p force-pushed the 3197_change_properties_to_text branch from c83d2c9 to 05f9e04 Compare January 13, 2017 22:34

nand0p force-pushed the 3197_change_properties_to_text branch 3 times, most recently from 395bcee to c0c360e Compare January 13, 2017 23:01

tardyp requested changes Jan 16, 2017

View reviewed changes

nand0p force-pushed the 3197_change_properties_to_text branch 4 times, most recently from 9da4377 to 3e23807 Compare January 17, 2017 21:23

3197 DB schema: change_properties to text

d614f88

Alters change_properties.propery_value from String(1024) to Text http://trac.buildbot.net/ticket/3197

nand0p force-pushed the 3197_change_properties_to_text branch from 3e23807 to d614f88 Compare January 17, 2017 21:32

tardyp merged commit 0fde518 into buildbot:master Jan 19, 2017

rodrigc mentioned this pull request Jan 20, 2017

Fix whitespace to appease 'make isort'. #2636

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3197 DB schema: change_properties to text #2561

3197 DB schema: change_properties to text #2561

nand0p commented Jan 4, 2017

mention-bot commented Jan 4, 2017

tardyp commented Jan 5, 2017

nand0p commented Jan 6, 2017 •

edited

nand0p commented Jan 9, 2017

tardyp commented Jan 9, 2017

tardyp Jan 9, 2017

nand0p Jan 9, 2017

tardyp Jan 10, 2017

tardyp Jan 10, 2017

tardyp left a comment

nand0p commented Jan 11, 2017 •

edited

nand0p commented Jan 11, 2017

tardyp Jan 11, 2017

tardyp Jan 11, 2017

nand0p commented Jan 13, 2017

tardyp left a comment

tardyp Jan 16, 2017

nand0p commented Jan 18, 2017

tardyp commented Jan 18, 2017

nand0p commented Jan 18, 2017 •

edited

tardyp commented Jan 19, 2017

		@@ -0,0 +1 @@
		Alters change properties from String to Text. Requires upgrade master. (:bug:`3197`)

3197 DB schema: change_properties to text #2561

3197 DB schema: change_properties to text #2561

Conversation

nand0p commented Jan 4, 2017

mention-bot commented Jan 4, 2017

tardyp commented Jan 5, 2017

nand0p commented Jan 6, 2017 • edited

nand0p commented Jan 9, 2017

tardyp commented Jan 9, 2017

tardyp Jan 9, 2017

Choose a reason for hiding this comment

nand0p Jan 9, 2017

Choose a reason for hiding this comment

tardyp Jan 10, 2017

Choose a reason for hiding this comment

tardyp Jan 10, 2017

Choose a reason for hiding this comment

tardyp left a comment

Choose a reason for hiding this comment

nand0p commented Jan 11, 2017 • edited

nand0p commented Jan 11, 2017

tardyp Jan 11, 2017

Choose a reason for hiding this comment

tardyp Jan 11, 2017

Choose a reason for hiding this comment

nand0p commented Jan 13, 2017

tardyp left a comment

Choose a reason for hiding this comment

tardyp Jan 16, 2017

Choose a reason for hiding this comment

nand0p commented Jan 18, 2017

tardyp commented Jan 18, 2017

nand0p commented Jan 18, 2017 • edited

tardyp commented Jan 19, 2017

nand0p commented Jan 6, 2017 •

edited

nand0p commented Jan 11, 2017 •

edited

nand0p commented Jan 18, 2017 •

edited