? Pending

User tests: Successful: Unsuccessful:

avatar richard67
richard67
24 Sep 2019

Pull Request for Issue #26351 (comment) .

Summary of Changes

This Pull Request (PR) does the same change as PR #26351 in postgresql/joomla.sql also for the update sql script "4.0.0-2018-07-29.sql" so there is no database error shown about missing index after a new installation of J4 on a PostgreSQL database.

Furthermore, the index name of the same index for MySQL is changed so it also fits to our naming scheme and is consistent with PostgreSQL (except that the latter has the table name as beginning of the index name), i.e. if someone searches for "idx_term_language" in sql files, he/she will find it in both MySQL and PostgreSQL joomla.sql and 4.0.0-2018-07-29.sql files.

Note on updating old 4.0 sql update scripts

Since we are not in Beta phase yet and so don't have to support updates from 4.0-Alpha-x to 4.0-Alpha-y or between nightly builds, we can change the existing 4.0 update scripts (but of course not pre-4.0 scripts). Later when in beta this will not be allowed anymore because we have to support updating from Beta-x to Beta-y (with y > x of course), so now is a good time to fix them.

Testing Instructions

For PostgreSQL

  1. Make a new installation of current 4.0-dev on a PostgreSQL database.
  2. After installation has finished, go to Extensions -> Manage -> Database and check if there are database errors shown.
  3. Verify that the change in file 4.0.0-2018-07-29.sql for PostgreSQL is the same change as PR #26351 has made in joomla.sql for this database type.

For MySQL

  1. Make a new installation of current 4.0-dev on a MySQL database.
  2. After installation has finished, go to Extensions -> Manage -> Database and check if there are database errors shown.
  3. Check e.g. in PhpMyAdmin the name of the index on columns term and language of the #__finder_terms_common table.

Expected result

On PostgreSQL

No database errors shown.

On MySQL

No database errors shown.

Index name is consistent with PostgreSQL (except that the latter has the table name as beginning of the index name).

Actual result

On PostgreSQL

Database error shown, see #26351 (comment) .

On MySQL

No database errors shown.

Index name is not consistent with PostgreSQL and our naming scheme "idx_column1_column2" for an index over 2 columns.

Documentation Changes Required

None.

avatar richard67 richard67 - open - 24 Sep 2019
avatar richard67 richard67 - change - 24 Sep 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 24 Sep 2019
Category SQL Administration com_admin Postgresql Installation
avatar richard67
richard67 - comment - 24 Sep 2019

@alikon Please test. @wilsonge Please test and/or review.

avatar brianteeman
brianteeman - comment - 24 Sep 2019

I will repeat my objection to changing sql update files. They should be immutable no matter what stage of release we are in

avatar richard67
richard67 - comment - 24 Sep 2019

@brianteeman That's your opinion, and you have been told several times that you are not right.

avatar brianteeman
brianteeman - comment - 24 Sep 2019

If they are not immutable at this stage then there was no point at all in creating multiple update sql and everything should have just gone in one file

avatar richard67
richard67 - comment - 24 Sep 2019

Well, that could have been done, but there are other good reasons to have separate files for particular changes, last but not least execution times.

The thing is that the database schema checker can't handle when you e.g. change data type of a column in 1 sql update script, and then later change it again in a later update script. Sometimes such things happen. When the 1st sql is already in the wild, e.g. with beta, you have of course to make a 2nd script with a later date in the file name, which again modifies that column to another data type or varchar length or default value or whatever changes.

But this will not be enough, because when the column is fixed by the 2nd script and now you go again to the database checker, it shows an error that the column does not fit to what is in the 1st script. So you fix again, but then it will show that it does not fit to the 2nd script again, and so you end in a ping pong. We had such things in past, and the solution was that we commented out the change in the 1st script, leaving a ref to the corresponding PR in a comment. That was the only way to solve such stuff.

So immutable they were never. In some cases they had to be changed.

Now we are in the lucky situation that we don't have to support updates yet, so the period in which we can edit existing scripts is relative long. Normally we can edit an existing script only when it has not been released yes, e.g. I add a new script today, tomorrow someone finds mistake, and day after tomorrow is a release, then is is ok that it was touched.

This is why I think we should use this time now. If we would now act as if we would support updates from alpha x to alpha y, and we wanna change data structures, do things like nullable datetime columns and have the not nullable datetime columns valid default values, so we don't have any invalid datetime pronlemanymore which keeps us from MySQL strict mode, then we would have some 100 update scripts more, andf the first 50 would consist of comments that the statement has been moved to a later script due to this and that PR.

Sorry for my long text. I really don't want to argue. I only want to try again to explain you why I think we can do this now and why it is better. As soon as Beta is out, all will be like you and me are used to, the script will be nearly immutable except there is serious reason.

avatar richard67
richard67 - comment - 24 Sep 2019

P.S.: The database schema checker is the only reason why we sometimes have to touch old update sql scripts, and if we did not have this checker, we could exactly proceed like Brian sais, once an update sql is created it should never be touched again. They could all run in a sequence from old to young, with all intermediate states, so if something to be changed again, just add a new file with younger date at the end. That would be perfect and maximum safe. I wish it would be like that.

Only because we have the checker we can't do that, and for the same reason I try to do as much as I can in the old files as long as possible, because otherwise the sequence of changes over the multiple files would be again not checker-compatible.

avatar infograf768
infograf768 - comment - 25 Sep 2019

Restarted drone

avatar infograf768 infograf768 - change - 25 Sep 2019
Labels Added: ?
avatar wilsonge
wilsonge - comment - 25 Sep 2019

Agree with @richard67 - update files are never going to be totally immutable (for the reasons he stated - the upgrade checker isn't smart enough to understand - although this is edge cases) - but in my opinion there is no point in having bad data in the update files. For example here adding the bad index then removing it won't work with the schema checker - it will look for the index to exist and not exist at the same time (we've previously commented the files out for stable things)

Of course these are things that should be improved in the schema checker too so we can end up with immutable files - but we're not there yet. And to be honest for an alpha even if we get there i'd definitely think twice about whether it was needed to start creating the extra files

avatar wilsonge wilsonge - change - 25 Sep 2019
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2019-09-25 09:30:50
Closed_By wilsonge
avatar wilsonge wilsonge - close - 25 Sep 2019
avatar wilsonge wilsonge - merge - 25 Sep 2019

Add a Comment

Login with GitHub to post a comment