Enable 'SQL_BIG_SELECT'

stephan-ansems commented 2 years ago

Enabled this to support certain issues (especially when upgrading Joomla) with providers that have this setting disabled by default.

I experienced issues when upgrading a vanilla version of Joomla 4.1.4 to 4.1.5 which is due to the provider having disabled the 'SQL_BIG_SELECT' by default. Because this was disabled an error occurred when attempting to upgrade Joomla to the newer version. Combine this with the insistent upgrade emails from many installations at the same provider and it it gets old real fast....

Enabling this setting resolves the bug.

Summary of Changes

Only one line of code was needed

Testing Instructions

Requires some doing to set things up, but a vanilla upgrade of Joomla CMS 4.1.4 to 4.1.5 on a provider with the setting disabled and this patch not installed should reproduce the issue, pathing it would be the test.

Documentation Changes Required

stephan-ansems commented 2 years ago

Forgot to mention it was needed for mysqli databases only.

richard67 commented 1 year ago

Forgot to mention it was needed for mysqli databases only.

@stephan-ansems You mean it's not needed for MySQL databases or MariaDB databases with the PDO driver https://github.com/joomla-framework/database/blob/2.0-dev/src/Mysql/MysqlDriver.php#L150 ? Or have you just not checked with "MySQL (PDO)"?

richard67 commented 1 year ago

HLeithner commented 1 year ago

I'm not in favor doing any request just because a handful providers are unable to configure there sql server properly.

Each database request is expensive and it should be keep to a minimum.

richard67 commented 1 year ago

Forgot to mention it was needed for mysqli databases only.

@stephan-ansems You mean it's not needed for MySQL databases or MariaDB databases with the PDO driver https://github.com/joomla-framework/database/blob/2.0-dev/src/Mysql/MysqlDriver.php#L150 ? Or have you just not checked with "MySQL (PDO)"?

@stephan-ansems Meanwhile we know from a user who has tested it that it needs the change also for the PDO driver. See https://github.com/joomla/joomla-cms/issues/41156#issuecomment-1640736182 . Could you do that, too? Thanks in advance.

richard67 commented 1 year ago

I'm not in favor doing any request just because a handful providers are unable to configure there sql server properly.

Each database request is expensive and it should be keep to a minimum.

@HLeithner We already set the SQL mode with every connect, so one more SET statement is not really a performance problem. Unfortunately not all people have a hosting where they can control the database server configuration, and it seems that for some versions of MariaDB the default value of SQL_BIG_SELECTS is off (0). As far as I could see on a quick view (not verified yet), Drupal uses a similar approach to handle that problem.

HLeithner commented 1 year ago

I'm not in favor doing any request just because a handful providers are unable to configure there sql server properly. Each database request is expensive and it should be keep to a minimum.

@HLeithner We already set the SQL mode with every connect, so one more SET statement is not really a performance problem. Unfortunately not all people have a hosting where they can control the database server configuration, and it seems that for some versions of MariaDB the default value of SQL_BIG_SELECTS is off (0). As far as I could see on a quick view (not verified yet), Drupal uses a similar approach to handle that problem.

every request is a full roundtrip (actually multiple), where do we set the SQL mode? the code looks like it's only set if explicited wished by the application.

if adding this then maybe as option in the application (cms) for broken server but in the end we shouldn't support this

richard67 commented 1 year ago

where do we set the SQL mode?

@HLeithner For MySQLi we set the option here in the constructor: https://github.com/joomla-framework/database/blob/2.0-dev/src/Mysqli/MysqliDriver.php#L126 and then https://github.com/joomla-framework/database/blob/2.0-dev/src/Mysqli/MysqliDriver.php#L110-L114 . And later we set the mode in the session in the connect method here: https://github.com/joomla-framework/database/blob/2.0-dev/src/Mysqli/MysqliDriver.php#L327-L331

For MySQL (PDO) we set it here: https://github.com/joomla-framework/database/blob/2.0-dev/src/Mysql/MysqlDriver.php#L118-L122 and then https://github.com/joomla-framework/database/blob/2.0-dev/src/Mysql/MysqlDriver.php#L127 and later set it in the session here: https://github.com/joomla-framework/database/blob/2.0-dev/src/Mysql/MysqlDriver.php#L231-L235 .

the code looks like it's only set if explicited wished by the application.

As you can see by the previously linked code, that is not true. We set the SQL mode to the predefined values in the constructor if there are not explicitly other values defined in the options parameter.

HLeithner commented 1 year ago

As you can see by the previously linked code, that is not true. We set the SQL mode to the predefined values in the constructor if there are not explicitly other values defined in the options parameter.

I didn't say that this is the case, what I say is that the framework package should be application agnostic, and the application is responsible for the concrete parameters it needs to work with the database.

richard67 commented 1 year ago

@HLeithner I've prepared 2 alternative PR's, #284 for 2.0-dev and #285 for 3.x-dev. Would that be better and should we do it in both branches or only in 3.x-dev for Joomla 5? I think we should also fix it for Joomla 4.4 and so in the 2.0-dev branch here.

HLeithner commented 1 year ago

thanks, looking at the reason for the sqlMode query I found this commit, do we still need this with mysql 8? https://github.com/joomla-framework/database/commit/11c95bf19d30e7d86726b20e702d328ba9addd12

richard67 commented 1 year ago

thanks, looking at the reason for the sqlMode query I found this commit, do we still need this with mysql 8? 11c95bf

@HLeithner Well, the comment says mysql > 5.7.+, and 8 fulfils this condition, so I would assume: yes.

HLeithner commented 1 year ago

the question is, is it still relevant for us, since in all supported version NULL dates are supported and 0000-.... is deprecated and not used in the cms anymore, not sure if this is relevant for 3rd party

richard67 commented 1 year ago

the question is, is it still relevant for us, since in all supported version NULL dates are supported and 0000-.... is deprecated and not used in the cms anymore, not sure if this is relevant for 3rd party

@HLeithner What does the $this->options['sqlModes'] = explode(',', $this->setQuery('SELECT @@SESSION.sql_mode;')->loadResult()); have to do with the null date thing? It's in the same commit, but I don't think it's related to the other change. It's just reading back the result in case if some of the modes were implicitly changed.

HLeithner commented 1 year ago

There reason for the sqlModes select query is only because of this nullDate check.

richard67 commented 1 year ago

There reason for the sqlModes select query is only because of this nullDate check.

@HLeithner Possibly, but it's still not related to the issue handled here or in my draft PR's.

P.S.: And as long as the framework minimum version is not adjusted we still need it as it should work with 5.6. See here: https://github.com/joomla-framework/database/blob/2.0-dev/src/Mysqli/MysqliDriver.php#L86

HLeithner commented 1 year ago

The reason I'm bringing it up here is to think about a conceptional problem. We just fix a problem of a group of people, with patchwork. But we don't have a framework which allows us or the user to set everything in easily or completely ignore the case and set it in the relevant application directly (if needed).

richard67 commented 10 months ago

@HLeithner Could you check again my 2 alternative PR's, #284 for 2.0-dev and #285 for 3.x-dev? I have modified and extended them so they handle your concerns about unnecessary SQL statements - they only do something if necessary, i.e. explicitly required by the parameters. I've also added methods to get the parameters e.g. for display in the system information in the CMS backend. The PR's are still draft because I have to provide some testing instructions (or some PR for the CMS using the stuff added here with my PR's).

richard67 commented 9 months ago

@HLeithner I've decided to split up the changes from this PR here to

PR #292 for the 2.0-dev branch for just setting the sql_big_select variable if that is explicitly enforced in the driver's options so that can be used by a bug fix PR for the CMS,
PR #293 for the 3.x-dev branch with the same thing, just in case if the one for 2.0-dev will not be accepted .e.g. because no new features, and
PR #294 for the 3.x-dev branch with new, more generalized methods to get the values of session variables e.g. for display in the system information of the CMS, which then can be used for a feature PR by the CMS.

I'd be happy if you could find the time and have a look on them. The same applies of course to all other readers here.

richard67 commented 9 months ago

Meanwhile I was able to identify the critical SQL query in the CMS core, and a pull request is ready for being merged: https://github.com/joomla/joomla-cms/pull/42576 . So for the CMS core the issue will be solved, and it will not need the changes from this PR here or my alternative PR's which I had mentioned above. Therefore I've closed my PR's.

joomla-framework / database