Skip to content

Conversation

@csthomas
Copy link
Contributor

@csthomas csthomas commented Feb 16, 2018

Pull Request for Issue # #14331

Summary of Changes

This PR is based on #17351, #18483 and is a replacement for #18483.

After PR, joomla recognises correctly:

  • SET NOT NULL;
  • SET DEFAULT '';
  • DROP DEFAULT; - does not work in [postgresql] - schema change rework #18483
  • DROP NOT NULL;
  • TYPE character varying(127) - new stuff - allow to resize column
  • ... DROP DEFAULT, ... SET DEFAULT 0; - only test the last action after comma

Testing Instructions

[UPDATED]

  1. Install joomla staging and go to backend to extension -> database.
  2. Now you see an issue:
    j1

This issue should not happen because there is a fresh installation.

The column has a definition at

"data" text NOT NULL,

This means that the update query from
ALTER TABLE "#__updates" ALTER COLUMN "data" SET DEFAULT '';

is wrong and should be removed as I did in my PR.

  1. After we click on the database fix, everything should be OK, but the query from:
    ALTER TABLE "#__updates" ALTER COLUMN "data" DROP DEFAULT;

    is not visible by joomla (bug) and the query can not fix our database.

Now our database is different than the fresh from installation but joomla does not see any problems.

  1. Apply this PR by com_patchtester

  2. Go to extension -> database.

  3. Now you see
    j2

  4. After you click on the Fix button, the column data from #__updates will be repaired and will be equal to definition from

    "data" text NOT NULL,

  5. The next tests are to check if each query starts with ALTER TABLE ALTER COLUMN is visible by joomla. I prepared 7 queries:

ALTER TABLE "#__newsfeeds" ALTER COLUMN "modified_by" DROP NOT NULL;
ALTER TABLE "#__newsfeeds" ALTER COLUMN "modified_by" DROP DEFAULT;
ALTER TABLE "#__newsfeeds" ALTER COLUMN "modified_by" TYPE bigint USING "modified_by"::bigint;

ALTER TABLE "#__contact_details" ALTER COLUMN "con_position" SET DEFAULT '';
ALTER TABLE "#__contact_details" ALTER COLUMN "mobile" DROP DEFAULT,
                                 ALTER COLUMN "mobile" SET DEFAULT 'new value';
ALTER TABLE "#__contact_details" ALTER COLUMN "address" SET NOT NULL;
ALTER TABLE "#__contact_details" ALTER COLUMN "alias" TYPE character varying(10) USING substr("alias", 1, 10);

After you put it at the bottom of

DROP INDEX "#__user_keys_series_2";
DROP INDEX "#__user_keys_series_3";

and refresh extension -> database page you should see a 7 issues:
j3

This means that every query is recognised by joomla.

Then click on the Fix button.
Now everything is OK.

  1. To revert all above changes, replace above code by:
ALTER TABLE "#__newsfeeds" ALTER COLUMN "modified_by" SET NOT NULL;
ALTER TABLE "#__newsfeeds" ALTER COLUMN "modified_by" SET DEFAULT 0;
ALTER TABLE "#__newsfeeds" ALTER COLUMN "modified_by" TYPE integer USING "modified_by"::integer;

ALTER TABLE "#__contact_details" ALTER COLUMN "con_position" DROP DEFAULT;
ALTER TABLE "#__contact_details" ALTER COLUMN "mobile" SET DEFAULT '';
ALTER TABLE "#__contact_details" ALTER COLUMN "address" DROP NOT NULL;
ALTER TABLE "#__contact_details" ALTER COLUMN "alias" TYPE character varying(255);

Then refresh extension -> database page we will see 7 issues:

j4

this means that all queries have been recognised.
Now click on the Fix button. All s OK.

  1. Remove all custom changes from administrator/components/com_admin/sql/updates/postgresql/3.8.4-2018-01-16.sql and refresh extension -> database. All is OK.

Expected result

All changes/issues are visible and can be fixed.

Actual result

Joomla does not recognise a few above queries or recognises them incorrectly.

@alikon
Copy link
Contributor

alikon commented Feb 17, 2018

i remember that DROP DEFAULT; was fixed on #18483, but i've no problem on closing that in favour of this one as your are adding new features, and with the hope this one have more luck than my old one 😆

  • trying to truncate a VARCHAR from 255 to 10 doesn't work
    getting the ERROR: value too long for type character varying(10)

ALTER TABLE "#__contact_details" ALTER COLUMN "alias" TYPE character varying(10);

you should issue/manage something like this instead

ALTER TABLE "#__contact_details" ALTER COLUMN "alias" TYPE varchar(10)
USING substr("alias", 1, 10)

@csthomas
Copy link
Contributor Author

csthomas commented Feb 19, 2018

Thanks,

ALTER TABLE "#__contact_details" ALTER COLUMN "alias" TYPE varchar(10) USING substr("alias", 1, 10)

I fixed it and now you can use function with parameters after word USING

@alikon
Copy link
Contributor

alikon commented Feb 21, 2018

after the first fix, still get database issues

screenshot from 2018-02-21 08-05-42

@csthomas
Copy link
Contributor Author

after the first fix, still get database issues

I have not explained that before.
This is an expected behaviour (for above columns) because of earlier sql queries.

This was a reason I had to comment line in administrator/components/com_admin/sql/updates/postgresql/3.1.0.sql

because it is conflicted with query from
administrator/components/com_admin/sql/updates/postgresql/3.7.0-2017-03-03.sql

Example:
The first update file contains "... column X SET DEFAULT ''"
The second update file redefines column X and change default value to NULL.
This two changes has a conflict and joomla show first or second issue.

The result is, the problem won't be fixed because the database table won't match to the query from the first file or to the query from the second file.

In my example, which you tested, I redefined a few columns and then joomla show changed column that does not match to queries from previous update files.

If this is a problem I can find a better columns (which does not exists in update files before).

@alikon
Copy link
Contributor

alikon commented Feb 21, 2018

i was 😪
😄

@alikon
Copy link
Contributor

alikon commented Feb 21, 2018

I have tested this item ✅ successfully on 7001e67


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19707.

@brianteeman
Copy link
Contributor

I finally got pgsql installed on my windows machine

Not sure if my test is successful or not as I still have a few issues to fix after applying the patch


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19707.

@csthomas
Copy link
Contributor Author

csthomas commented Feb 23, 2018

Nice,

after you applied the PR, joomla starts to show you a database problems (old problems that have never been recognized before).

Before you start to use my above test examples you can click on fix button and all issues should disappear.
Today or tomorrow I will try to add easier text instructions.

@csthomas
Copy link
Contributor Author

I updated the test instructions.

@csthomas
Copy link
Contributor Author

csthomas commented Mar 1, 2018

As PR #17351 for the mysql version has been merged it would be good to take a look at this version for postgreSQL.

@csthomas
Copy link
Contributor Author

Can we merge this after only one success test?

/* Changes to tables where data type conflicts exist with MySQL (mainly dealing with null values */
ALTER TABLE "#__modules" ALTER COLUMN "content" SET DEFAULT '';
ALTER TABLE "#__updates" ALTER COLUMN "data" SET DEFAULT '';
--ALTER TABLE "#__updates" ALTER COLUMN "data" SET DEFAULT '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should't we remove this line? Maybe I just don't get it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

can we add such a comment than too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks 👍

@csthomas csthomas force-pushed the db_fix_postgresql_button branch from 7001e67 to daed6e3 Compare May 17, 2018 20:51
@csthomas
Copy link
Contributor Author

I rebased it and I added a comment only.

@wilsonge
Copy link
Contributor

Given the low number of postgres testers I'm going to merge this with the one good test

@wilsonge wilsonge merged commit bf199b0 into joomla:staging Aug 10, 2018
@wilsonge wilsonge added this to the Joomla 3.8.12 milestone Aug 10, 2018
@csthomas csthomas deleted the db_fix_postgresql_button branch August 10, 2018 17:28
richard67 added a commit to richard67/joomla-cms that referenced this pull request Dec 30, 2021
Migrate the change from PR joomla#19707 for ALTER TABLE statements with multiple actions to check the last change instead of failing from PostgreSQL to MySQL.
richard67 added a commit to richard67/joomla-cms that referenced this pull request Dec 31, 2021
Migrate the change from PR joomla#19707 for ALTER TABLE statements with multiple actions to check the last change instead of failing from PostgreSQL to MySQL.
richard67 added a commit to richard67/joomla-cms that referenced this pull request Jan 3, 2022
Migrate the change from PR joomla#19707 for ALTER TABLE statements with multiple actions to check the last change instead of failing from PostgreSQL to MySQL.
richard67 added a commit to richard67/joomla-cms that referenced this pull request Jan 22, 2022
Migrate the change from PR joomla#19707 for ALTER TABLE statements with multiple actions to check the last change instead of failing from PostgreSQL to MySQL.
richard67 added a commit to richard67/joomla-cms that referenced this pull request Feb 28, 2022
Migrate the change from PR joomla#19707 for ALTER TABLE statements with multiple actions to check the last change instead of failing from PostgreSQL to MySQL.
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.

7 participants