Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Partitions editor: Allow to edit partition names; Add 'COLUMNS Partitioning' support. #12430

Merged
merged 1 commit into from
Aug 17, 2016

Conversation

rpv-tomsk
Copy link
Contributor

Fixed issues:

  • Table partitions editor does not support 'COLUMNS Partitioning' ('RANGE COLUMNS', 'LIST COLUMNS'). Added in MySQL 5.5 (http://dev.mysql.com/doc/refman/5.5/en/partitioning-columns.html).
  • Table partitions editor ignores exising partitions names and overwrites them to 'pXX' on saving.
  • The 'Partition maintenance' snippet on 'Operations' tab has duplicate partition names in list when partitioning has subpartitions.
  • Partition comment textarea adds extra spaces to value. Caused by word wrap/formatting.

Optimization:

  • Optimize Partition::getPartitionMethod() - need only one row, so use 'LIMIT 1' in query to avoid much data transfer from database when table has many partitions/subpartitions.

@codecov-io
Copy link

codecov-io commented Aug 2, 2016

Current coverage is 50.07% (diff: 22.22%)

No coverage report found for QA_4_6 at 16c9eee.

Powered by Codecov. Last update 16c9eee...5a5ae58

@rpv-tomsk
Copy link
Contributor Author

Possible TODO/future improvement: Add columns hint/selectbox for 'COLUMNS Partitioning'.

@nijel
Copy link
Contributor

nijel commented Aug 3, 2016

I think this is something what is out of scope for maintenance branch (4.6), please target such changes to master branch.

@rpv-tomsk
Copy link
Contributor Author

rpv-tomsk commented Aug 3, 2016

I think this is something what is out of scope for maintenance branch (4.6), please target such changes to master branch.

Are you speaking about whole PR or about 'hint/selectbox'?

If about whole PR, when what do you think about this part:

  • The 'Partition maintenance' snippet on 'Operations' tab has duplicate partition names in list when partitioning has subpartitions.
  • Partition comment textarea adds extra spaces to value. Caused by word wrap/formatting.

?

@rpv-tomsk
Copy link
Contributor Author

I think this is something what is out of scope for maintenance branch (4.6),

As for me, I have put this to our production server.
I tried to make things backward-compatible, how it possible, so I see no much changes to forbid merging to 4.6. But last decision is on team, of course.

<?= htmlspecialchars($partition['name']); ?>
</th>
<td rowspan="<?= $rowspan; ?>">
<input type="text" name="<?= $partition['prefix']; ?>[name]"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

htmlspecialchars probably missing here.

@nijel
Copy link
Contributor

nijel commented Aug 4, 2016

I don't think this qualifies as bug fix, which is only thing which should be going into 4.6 branches.

@rpv-tomsk
Copy link
Contributor Author

I don't think this qualifies as bug fix, which is only thing which should be going into 4.6 branches.

How I found this issue:

We have some table with partitions named 'year_2015', 'year_2016', 'year_max' and so on.
Our queries use partition names (https://dev.mysql.com/doc/refman/5.6/en/partitioning-selection.html)

After editing partitions all these names which are referenced in queries was lost. Is this not a bug?

@rpv-tomsk
Copy link
Contributor Author

htmlspecialchars probably missing here.

You are likely to be right. I will continue the work on this patch on this week.

Thanks for code review.

@madhuracj
Copy link
Contributor

With @rpv-tomsk's explanation, I tend to think this qualifies as a bug fix.

@rpv-tomsk rpv-tomsk force-pushed the improve-partitions-support branch 2 times, most recently from 4ed1d53 to 4f7b205 Compare August 14, 2016 09:48
@rpv-tomsk
Copy link
Contributor Author

rpv-tomsk commented Aug 14, 2016

As per libraries/tbl_partition_definition.inc.php and libraries/controllers/table/TableStructureController.php code, partition['prefix'] value is built by code, in form of partitions[X][subpartitions][Y] where X and Y are integers. So, no htmlspecialchars() is required on partition['prefix'] value. But if htmlspecialchars() is required by codestyle, I will update the patch.

While digging, found missing htmlspecialchars() on $subParition->getComment() in templates/table/structure/display_partitions.phtml. Patch updated.

Also, please look into templates/columns_definitions/partitions.phtml:111, there is unescaped $partition['value']; :

<input type="text" class="partition_value"
                               name="<?= htmlspecialchars($partition['prefix']); ?>[value]"
                               value="<?= $partition['value']; ?>" />

I'm unsure if this variable requires htmlspecialchars().

…ioning' support.

+ Add 'COLUMNS Partitioning' support
+ Allow to edit partition names
* Do not show duplicate partition names in 'Partition maintenance' snippet on 'Operations' tab. Caused by subpartitions presence.
* Optimize Partition::getPartitionMethod() - need only one row, so use 'LIMIT 1' in query to avoid much data transfer from database.
* Change HTML-code of partition comment textarea to avoid extra spaces added to value.
* Added missing htmlspecialchars() on subpartitions comment.

Signed-off-by: Pavel Rochnyack <pavel2000@ngs.ru>
@nijel nijel self-assigned this Aug 17, 2016
@nijel nijel added this to the 4.6.5 milestone Aug 17, 2016
@nijel nijel merged commit ba59178 into phpmyadmin:QA_4_6 Aug 17, 2016
@nijel
Copy link
Contributor

nijel commented Aug 17, 2016

Okay, merged now. Thanks!

nijel added a commit that referenced this pull request Aug 17, 2016
Impove partitioning support

Signed-off-by: Michal Čihař <michal@cihar.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants