Opened 4 years ago

Closed 4 years ago

#4237 closed defect (fixed)

MySQL SQL statments can fail due to inconsistent use of engine settings when creating tables

Reported by: tmark Owned by: tmark
Priority: medium Milestone: Kea1.0
Component: database-all Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Our MySQL SQL scripts do not consistently use "ENGINE = INNODB" with CREATE TABLE statements. Depending on the MySQL installation and configuration settings, the default engine may not be INNODB. This results in different ENGINE tyes for different tables and attemting to
create foreign keys between them susequently fail.

All MySQL SQL text needs shold explicitly set the ENGINE type to INNODB. This includes:

src/bin/admin/scripts/mysql

dhcpdb_create.mysql
upgrade_1.0_to_2.0.sh.in
upgrade_2.0_to_3.0.sh.in
upgrade_3.0_to_4.0.sh.in

src/bin/admin/tests/dhcpdb_create_1.0.mysql

Subtickets

Change History (8)

comment:1 Changed 4 years ago by tmark

  • Owner set to tmark
  • Status changed from new to assigned

comment:2 Changed 4 years ago by tmark

  • Owner changed from tmark to Unassigned
  • Status changed from assigned to reviewing

This was originally reported as a pull request:

https://github.com/isc-projects/kea/pull/16

However the pull only addressed the create script, mysql/dhcp_create.mysql.
This left out the upgrade scripts and unit test schema create text.

All of the above were updated.

I removed the explicit delete of the ipv6_reserverations table from
src/bin/admin/tests/mysql_tests.sh.in:mysql_wipe() function as this
step is not necessary. The fucntion turns off referential integrity
checks, so the tables can be deleted in any order.

Similarly, I added a the statement to turn off referential intergrity
checks to src/lib/dhcpsrv/tests/schema_mysql_copy.h:destroy_statement.

I suggest the following ChangeLog:

1xxx.   [bug]       tmark
    Added explicit use of INNODB engine to all MySQL table create statements.
    This corrects an issue that caused MySQL database creation or upgrade to
    fail in environments where INNODB is not the default engine.
    (Trac #4237, git TDB)

comment:3 Changed 4 years ago by tomek

  • Owner changed from Unassigned to tomek

comment:4 Changed 4 years ago by tomek

  • Owner changed from tomek to tmark

As discussed on jabber, this sort of change warrants update of the minor DB version. There's other minor change in another ticket, so the decision was to bundle both of them together.

src/bin/admin/scripts/tests/data/mysql.lease6_dump_test.reference.csv
The change in caused the unit-test to fail on Ubuntu 15.10 x64. Here's the failure and content of related files: http://pastebin.com/tzTk94Q2.

After a brief discussion, we came to a conclusion that the best way forward will be to update the test with ORDER BY to ensure that the order is predictable.

comment:5 Changed 4 years ago by tmark

  • Owner changed from tmark to tomek

I modified the dump functions to order by lease address.

I have bumped the MySQL version number from 4.0 to 4.1

Per our jabber discussion I have addressed #4238 with this as it too requires
a bump in the schema version. This way we only bump it once.

The ChangeLog entry should probably now read this:

1xxx.   [bug]       tmark
    This change bumps the MySQL schema version from 4.0 to 4.1 and includes
    the following changes: added explicit use of INNODB engine to all MySQL 
    table create statements,  MySQL lease dump output is now sorted by lease 
    address in ascending order, and the MySQL lease_hwaddr_source table now 
    contains an entry for HWADDR_SOURCE_UNKNOWN (i.e. source = 0).
    (Trac #4237, #4238, git TDB)

I ran the unit tests under both my Centos VM which is running MySQL 5.1 and my OS-X which is running MySQL 5.6.

comment:6 Changed 4 years ago by tomek

Updated code compiles on ubuntu 15.10 x64 and unit-tests pass. I did review the changes and they look good. Thanks for taking care of this.

Except peculiar choice of capitalization in FROm word in
dhcpdb-create.mysql, the changes look good and seem to be working ok.

I think you should credit somehow the guy who reported this and sent a patch. It may be a minor patch, but it encourages people to keep contributing. Also, getting longer list of contributors encourages others to join the community.

Putting something in ChangeLog? is sufficient. If you think it's appropriate to credit this guy, you may also mention him in contributions section in AUTHORS file. I tend to do it. It's useful if we ever get more contributions from a given person and we can get back to him "we still remember the patch for X you sent us years ago. thanks!".

The ChangeLog? entry should also mention github pull request #. There were couple other pull requests accepted before. We used Github #X naming convention.

With those three minor things, the change is ready to go. Make sure you close both trac tickets and appropriate pull request on github. If you don't have sufficient credentials for that, ping me and I'll set you up.

comment:7 Changed 4 years ago by tomek

  • Owner changed from tomek to tmark

comment:8 Changed 4 years ago by tmark

  • Resolution set to fixed
  • Status changed from reviewing to closed

I fixe the "FROm" and updated the AUTHORS file.

Changes merged with git #f0fb9f35a394785215573a591c2bcc68ab481436

Added ChangeLog? entry #1073. It includes the pull request and submitter thank you.

I thanked the submitter via the pull request which I will now close

I will also close 4238.

Note: See TracTickets for help on using tickets.