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

Moving Columns with DEFAULT NULL doesn't work on MariaDB 10.2+ #14951

Closed
adwiv opened this issue Feb 12, 2019 · 22 comments
Closed

Moving Columns with DEFAULT NULL doesn't work on MariaDB 10.2+ #14951

adwiv opened this issue Feb 12, 2019 · 22 comments
Assignees
Labels
Bug A problem or regression with an existing feature
Projects
Milestone

Comments

@adwiv
Copy link

adwiv commented Feb 12, 2019

Describe the bug

When a column is nullable with default value NULL, trying to move it using the "Move Columns" popup either does not work or works incorrectly with disastrous side effects.

It appears that the SQL generated for moving the column converts the NULL keyword value into 'NULL' string value. If the column is a text column, the default value is changed to 'NULL' String instead of the NULL VALUE. If the column is some other type (eg int), the column moving fails with an error since the string 'NULL' is not a valid integer value.

To Reproduce

Create a simple table

CREATE TABLE test (
 col1 int(11) DEFAULT NULL,
 col2 varchar(20) DEFAULT NULL
)

Click on the Structure tab and select "Move Columns". Try to move col2 to top of table. The SQL generated is:

ALTER TABLE `test` CHANGE `col2` `col2` VARCHAR(20) NULL DEFAULT 'NULL'  FIRST

Notice the 'NULL' in quotes which changes the default value of the column.

Now try moving column col1 to top of table again. This generates the following SQL

ALTER TABLE `test` CHANGE `col1` `col1` INT(11) NULL DEFAULT 'NULL' FIRST

This time running it fails with error #1366 - Incorrect integer value: 'NULL' for column 'col1' at row 1

Expected behavior

Columns should have moved without any side effects. The default value should have remained NULL, and not converted to string 'NULL'

Server configuration

  • MAC OS X 10.14.2
  • Web server: Apache/2.4.35 (Unix)
  • Database version: mysqlnd 5.0.12
  • PHP version: 7.3.1
  • phpMyAdmin version: 4.8.5 and 5.0+snapshot
@williamdes
Copy link
Member

Tested on demo servers (root, no password)

QA_4_8

On QA_4_8 there is a modal that is broken.
Can not test more.

@williamdes williamdes added the Bug A problem or regression with an existing feature label Feb 12, 2019
@adwiv
Copy link
Author

adwiv commented Feb 13, 2019

On further testing I can confirm that the bug is somehow dependent on Server version.

The latest snapshot works fine with MariaDB 10.1.38, but fails with newer MariaDB 10.2.22 and 10.3.12, whereas MySQL 5.7/8.0 do not seem to be affected.

Tested using docker images on Ubuntu 18.04 with apache and phpMyAdmin-5.0+snapshot-all-languages.tar.gz

@kartik1000
Copy link
Contributor

@williamdes, Hey it's working fine for me both on mariadb and mySql Server on demo server. I'll share the screenshot with you and in preview Sql mode also its working fine for me
Screenshot 2019-03-10 at 1 09 50 AM

@williamdes
Copy link
Member

@kartik1000 Can you please try on wdes's demo servers ? (we have enough mariadb and mysql servers)

https://phpmyadmin-pr-6.wdes.eu.org and https://phpmyadmin-pr-7.wdes.eu.org

public/public

@kartik1000
Copy link
Contributor

@williamdes, Hey Yes You are right It ain't working on these servers switching col 1 to top does give back an error and the error is same as stated by @adwiv , and moreover https://phpmyadmin-pr-7.wdes.eu.org does not show sql preview option whereas the other one does.

@williamdes
Copy link
Member

Thank you @kartik1000 :)

@williamdes williamdes added this to Medium priority in issues Apr 29, 2019
@williamdes
Copy link
Member

I could not reproduce this issue on demo servers (root, no password)

@williamdes williamdes moved this from Medium priority to Not reproduced in issues Aug 11, 2019
@adwiv
Copy link
Author

adwiv commented Aug 11, 2019

@williamdes The demo servers referred by you use MariaDB 10.1.26 whereas the errors happen with newer 10.2 or 10.3.

@williamdes
Copy link
Member

williamdes commented Aug 11, 2019

On my demo servers https://phpmyadmin-pr-7.wdes.eu.org/ (QA_4_9)

The servers are MariaDB 1 > 10.3 MariaDB 2 > 10.2 MariaDB 3 > 10.3

User root or public and password public

[EDIT]

Reproduced: you need to move col2 to first position and then try to move it back !

@williamdes
Copy link
Member

image

#1366 - Incorrect integer value: 'NULL' for column ``.``.`col1` at row 1

@williamdes williamdes moved this from Not reproduced to Medium priority in issues Aug 11, 2019
@adwiv
Copy link
Author

adwiv commented Aug 11, 2019

Reproduced: you need to move col2 to first position and then try to move it back !

Yes, if the column type is VARCHAR, the move changes NULL default value to string "NULL".
Second time, the error is shown because INT column is being moved and string "NULL" cant be converted to int.

If you use the following schema, it will show the error everytime.

CREATE TABLE test (
 col1 int(11) DEFAULT NULL,
 col2 varchar(20) DEFAULT NULL
)

In short, the code is somehow interpreting NULL default value as string "NULL". In case of any non-text column, it will show error. And in case of text column, it won't show error but will corrupt the schema.

@adwiv
Copy link
Author

adwiv commented Oct 19, 2019

Okay. I figured out the cause of issue.

Starting with MariaDB 10.2, they have made some changes to the Information Scheme Table which deviates from MySQL. ChangeLog Here

Changes to the Information Schema COLUMNS table. Literals are now quoted in the COLUMN_DEFAULT column to distinguish them from expressions (MDEV-13132), and two new columns added providing information about generated (virtual, or computed) columns (MDEV-9255).

So, when we have a nullable column with NULL default in MariaDb, instead of getting a null value from the information_schema table for COLUMN_DEFAULT, we get a literal string with value NULL.

@kartik1000
Copy link
Contributor

@adwiv, you must be right because I checked the changes made in the codebase when moving columns, and checked if what change possibly triggered this issue as it was working fine earlier but couldn't find any. @williamdes what can we now do possibly to fix the issue?

@williamdes williamdes self-assigned this Oct 19, 2019
@williamdes
Copy link
Member

@kartik1000 I would add an override to the existing code

@kartik1000
Copy link
Contributor

@kartik1000 It will implement the desired code after checking the MariaDB version right?

@williamdes
Copy link
Member

@kartik1000 Yes, perfect :)

@adwiv
Copy link
Author

adwiv commented Oct 20, 2019

@kartik1000 @williamdes There is another problem introduced due to a recent commit 98a7479 which results in column always becoming NOT NULL when moved. This commit changed the expected value of $null parameter in function Table::generateFieldSpec from NULL/NOT NULL to YES/NO.

if ($null == 'YES') {
$query .= ' NULL';
} else {
$query .= ' NOT NULL';
}

However, the expected value was not not updated in TableStructure::moveColumns function which is still passing NULL/NOT NULL as parameters.

$data['Null'] === 'YES' ? 'NULL' : 'NOT NULL',

So, in case of moving a nullable field, Table::generateFieldSpec receives parameter $null as 'NULL' but is expecting 'YES', so it outputs 'NOT NULL' as field definition. The resolution is to simply change the above line to send parameters YES/NO instead of NULL/NOT NULL.

Thanks.

@williamdes
Copy link
Member

@adwiv Thank you so much for your comment!
Making the fix soon

@williamdes williamdes changed the title Moving Columns with DEFAULT NULL doesn't work Moving Columns with DEFAULT NULL doesn't work on MariaDB 10.2+ Nov 9, 2019
williamdes added a commit to williamdes/phpmyadmintest that referenced this issue Nov 9, 2019
…on MariaDB 10.2+

Ref: 98a7479
Signed-off-by: William Desportes <williamdes@wdes.fr>
williamdes added a commit that referenced this issue Nov 9, 2019
Signed-off-by: William Desportes <williamdes@wdes.fr>
@williamdes
Copy link
Member

Hi @adwiv thanks for your patience !
can you try to apply 8aad190 ?

You can also add e9ad2bc

@williamdes williamdes added this to the 4.9.2 milestone Nov 9, 2019
issues automation moved this from Medium priority to Closed Nov 10, 2019
@etkaar
Copy link

etkaar commented Dec 1, 2019

Good evening! Was this problem fixed? Because it still concerns me when I try to move columns:

#1366 - Incorrect integer value: 'NULL' for column ..last_login_time at row 1

Apache/2.4.38
PHP 7.3.11-1~deb10u1
10.3.18-MariaDB-0+deb10u1
phpMyAdmin 4.6.6deb4

@williamdes
Copy link
Member

@Kaulkwappe your phpmyadmin version is very out to date
As you can see in #15236 phpMyAdmin 4.9 will be shipped in the next debian version and for now you can use our Ubuntu ppa #15515

The issue seems is fixed in latest versions

@etkaar
Copy link

etkaar commented Dec 1, 2019

I see, thanks for the fast response. Seems I have to wait since 4.6.6deb4 is the most recent available version in Debian Buster 10.2.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A problem or regression with an existing feature
Projects
issues
  
Closed
Development

No branches or pull requests

4 participants