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

Fulltext search edit - fixes #4387 #4388

Merged
merged 2 commits into from Dec 7, 2017

Conversation

Projects
None yet
4 participants
@sbulen
Contributor

sbulen commented Nov 18, 2017

There was a problem with the version check that prevented this from working properly.

Fixes #4387

Tested by creating fulltext indexes and performing searches on MyISAM and InnoDB. Also confirmed edit works properly by preventing creation of a fulltext index on MySQL 5.5 for InnoDB.

sbulen added some commits Nov 18, 2017

Fix innodb fulltext version check
Signed by Shawn Bulen, bulens@pacbell.net
Clarify fulltext message
Signed by Shawn Bulen, bulens@pacbell.net
@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Nov 19, 2017

Collaborator

Since many distro provide mariadb instead of mysql,
you should maybe extend the check also for mariadb (i know that smf didn't support mariadb),
would safe many support tickets...

Collaborator

albertlast commented Nov 19, 2017

Since many distro provide mariadb instead of mysql,
you should maybe extend the check also for mariadb (i know that smf didn't support mariadb),
would safe many support tickets...

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Nov 19, 2017

Contributor

Since MariaDB supports the same engines (InnoDB and MyISAM), I don't think anything needs to change here to provide MariaDB support.

In theory, it is "drop-in" compatible with MySQL. Like Percona.

MariaDB offers several additional engines (Aria), but a lot more work would need to be done to support those. I don't think we want to go there.

Contributor

sbulen commented Nov 19, 2017

Since MariaDB supports the same engines (InnoDB and MyISAM), I don't think anything needs to change here to provide MariaDB support.

In theory, it is "drop-in" compatible with MySQL. Like Percona.

MariaDB offers several additional engines (Aria), but a lot more work would need to be done to support those. I don't think we want to go there.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Nov 19, 2017

Collaborator

The possible issue is that db_get_version returns 10.1.21-MariaDB instead of 5.7.0,
but your are lucky because mariadb 10 ~ mysql 5.6.
When the feature exists since 5.7 then you had to beware of which db you running,
because of mysql it would be 5.7 and in maria it would be 10.1 (10 wouldn't support this).

Collaborator

albertlast commented Nov 19, 2017

The possible issue is that db_get_version returns 10.1.21-MariaDB instead of 5.7.0,
but your are lucky because mariadb 10 ~ mysql 5.6.
When the feature exists since 5.7 then you had to beware of which db you running,
because of mysql it would be 5.7 and in maria it would be 10.1 (10 wouldn't support this).

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Nov 19, 2017

Contributor

Yikes... Their numbering strayed.

So much for "drop in" compatibility. That is now out the door.

Looks like innodb supports fulltext from 10.0.5 on:
https://mariadb.com/kb/en/library/full-text-index-overview/

I think the MariaDB folks shot themselves in the foot.

I'm not so sure we can maintain a mapping of versions - it would have to be feature by feature, since they have abandoned MySQL version numbering. That is not a reasonable expectation. Over time, our code would have to be peppered with individual checks.

I will leave this as-is.

Contributor

sbulen commented Nov 19, 2017

Yikes... Their numbering strayed.

So much for "drop in" compatibility. That is now out the door.

Looks like innodb supports fulltext from 10.0.5 on:
https://mariadb.com/kb/en/library/full-text-index-overview/

I think the MariaDB folks shot themselves in the foot.

I'm not so sure we can maintain a mapping of versions - it would have to be feature by feature, since they have abandoned MySQL version numbering. That is not a reasonable expectation. Over time, our code would have to be peppered with individual checks.

I will leave this as-is.

@colinschoen

LGTM approval

@colinschoen colinschoen merged commit e39a068 into SimpleMachines:release-2.1 Dec 7, 2017

2 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sbulen sbulen deleted the sbulen:fulltext_search_1 branch Dec 8, 2017

@Gwenwyfar Gwenwyfar added the Database label Dec 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment