Bugzilla

Updated

•

15 years ago

Assignee: zach → installation

Comment 6

•

10 years ago

The way to set ANSI mode is to run:

SET GLOBAL sql_mode='ANSI';

The way to check for it is to run:

SELECT @@global.sql_mode;

If the result is blank, ANSI mode is not set. If the result, split on commas, contains "ANSI", it is.

mysql> SELECT @@global.sql_mode;
+-------------------------------------------------------------+
| @@global.sql_mode                                           |
+-------------------------------------------------------------+
| REAL_AS_FLOAT,PIPES_AS_CONCAT,ANSI_QUOTES,IGNORE_SPACE,ANSI |
+-------------------------------------------------------------+
1 row in set (0.00 sec)

See https://dev.mysql.com/doc/refman/5.1/en/compatibility.html .

Gerv

Whiteboard: [good first bug]

Assignee

Comment 7

•

10 years ago

Attached patch DB.pm.patch (obsolete) — Details — Splinter Review

Attachment #8451345 - Flags: review?(gerv)

Comment 8

•

10 years ago

(In reply to Vishant Gautam from comment #7)
> Created attachment 8451345 [details] [diff] [review]
> DB.pm.patch

Hi Visham,

I'm a bit confused by your patch. It seems to be _removing_ some code related to ANSI mode, but I don't think we have any such code at the moment. Is the patch the wrong way round?

Also, the code seems to check for ANSI mode and tell the user to enable it if it's not enabled. However, this bug is about the fact that Bugzilla doesn't work in ANSI mode, and we want to tell the user to _dis_able it if it's enabled!

Gerv

Comment 9

•

10 years ago

Comment on attachment 8451345 [details] [diff] [review]
DB.pm.patch

Marking as r- to get a resolution to the issues raised in the previous comment.

Gerv

Attachment #8451345 - Flags: review?(gerv) → review-

Assignee

Comment 10

•

10 years ago

Attached patch DB.pm.patch (obsolete) — Details — Splinter Review

Attachment #8454987 - Flags: review?(gerv)

Assignee

Comment 11

•

10 years ago

(In reply to Gervase Markham [:gerv] from comment #9)
> Comment on attachment 8451345 [details] [diff] [review]
> DB.pm.patch
> 
> Marking as r- to get a resolution to the issues raised in the previous
> comment.
> 
> Gerv

Actually yearlier i was working on Bugzilla 4.4.4 but now i have changed to Bugzilla-4.5.4.

Comment 12

•

10 years ago

Comment on attachment 8454987 [details] [diff] [review]
DB.pm.patch

Review of attachment 8454987 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the updated patch :-)

Gerv

::: DB.old.pm
@@ +170,5 @@
>      # And now check the version of the database server itself.
>      my $dbh = _get_no_db_connection();
>      $dbh->bz_check_server_version($db, $output);
> +    
> +    my $rows = $dbh->prepare("select \@\@global.sql_mode;");

Use single quotes to avoid having to escape the @ characters.

Also, no need to prepare and execute - just do fetchrow_array directly.

@@ +171,5 @@
>      my $dbh = _get_no_db_connection();
>      $dbh->bz_check_server_version($db, $output);
> +    
> +    my $rows = $dbh->prepare("select \@\@global.sql_mode;");
> +		my $sth = $rows->execute();

Your indentation is messed up because you are using tabs. Please use only spaces, and use 4-space indentation for Perl code.

@@ +175,5 @@
> +		my $sth = $rows->execute();
> +		my $array = $rows->fetchrow_array;
> +
> +		if ( index($array,"ANSI") > 0 ){
> +				die "Quiting installation\n", "MYSql is in ANSI mode disable it before continuing

Max 80 columns on line.

Please use spacing around operators consistent with the rest of the file.

MySQL is spelt "MySQL". 

The error should simply be: "MySQL is in ANSI mode, which is incompatible with Bugzilla. See bug 252555."

Attachment #8454987 - Flags: review?(gerv) → review-

Assignee

Comment 13

•

10 years ago

Attached patch DB.pm.patch (obsolete) — Details — Splinter Review

I tried to execute fetchrow_array directly but its says that you can't execute it directly.

Attachment #8459271 - Flags: review?(gerv)

Comment 14

•

10 years ago

Comment on attachment 8459271 [details] [diff] [review]
DB.pm.patch

Review of attachment 8459271 [details] [diff] [review]:
-----------------------------------------------------------------

::: DB.old.pm
@@ +172,5 @@
>      $dbh->bz_check_server_version($db, $output);
> +    
> +    my $rows = $dbh->prepare('select @@global.sql_mode;');
> +	my $sth = $rows->execute();
> +	my $array = $rows->fetchrow_array;

I actually meant selectrow_arrayref, of course, which is the shortcut method for these three calls.

https://metacpan.org/pod/DBI#selectrow_arrayref

You still have an indentation problem here. Unless you are entering or leaving a block, the indentation level should not change.

@@ +173,5 @@
> +    
> +    my $rows = $dbh->prepare('select @@global.sql_mode;');
> +	my $sth = $rows->execute();
> +	my $array = $rows->fetchrow_array;
> +	if (index($array, "ANSI") < 0 ) {

You should split on "," and check for array membership, to be safe. (There may be other features with the letters A N S I in.)

@@ +176,5 @@
> +	my $array = $rows->fetchrow_array;
> +	if (index($array, "ANSI") < 0 ) {
> +	    die "Quiting installation\n",
> +"MySQL is in ANSI mode, which is incompatible with Bugzilla.
> +For disabling ANSI mode enter the following SQL commands:

My previous review asked you to change the error message to different text. Please use that text.

Thanks,

Gerv

Attachment #8459271 - Flags: review?(gerv) → review-

Assignee

Comment 15

•

10 years ago

Attached patch DB.pm.patch (obsolete) — Details — Splinter Review

Attachment #8460270 - Flags: review?(gerv)

Comment 16

•

10 years ago

Comment on attachment 8460270 [details] [diff] [review]
DB.pm.patch

Review of attachment 8460270 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch :-)

Gerv

::: DB.old.pm
@@ +170,5 @@
>      # And now check the version of the database server itself.
>      my $dbh = _get_no_db_connection();
>      $dbh->bz_check_server_version($db, $output);
> +    
> +    # And now check whether MySQL is in ANSI mode or not.

As far as I can see, this code executes even if we aren't using MySQL. You need to change that. The code just above should give you a value you can check to see if you are running MySQL.

@@ +172,5 @@
>      $dbh->bz_check_server_version($db, $output);
> +    
> +    # And now check whether MySQL is in ANSI mode or not.
> +    my @row = $dbh->selectrow_array('select @@global.sql_mode;');
> +    my @array = split(/,/, $row[0]);

@array is not the right name for this variable. Perhaps @mode_tokens?

@@ +175,5 @@
> +    my @row = $dbh->selectrow_array('select @@global.sql_mode;');
> +    my @array = split(/,/, $row[0]);
> +    
> +    die "MySQL is in ANSI mode, which is incompatible with Bugzilla.
> +See bug 252555." if 'ANSI' ~~ @array;

Please don't use postfix if. Also, please don't use multiline strings.

Attachment #8460270 - Flags: review?(gerv) → review-

Assignee

Comment 17

•

10 years ago

Attached patch DB.pm.patch (obsolete) — Details — Splinter Review

Attachment #8464758 - Flags: review?(gerv)

Comment 18

•

10 years ago

Comment on attachment 8464758 [details] [diff] [review]
DB.pm.patch

>+    # Now check if database is MySQl.
>+    if ($lc->{db_driver} eq 'mysql') {
>+        # And now check whether MySQL is in ANSI mode or not.
>+        my @row = $dbh->selectrow_array('select @@global.sql_mode;');
>+        my @mode_tokens = split(/,/, $row[0]);
>+        
>+        if ('ANSI' ~~ @mode_tokens) {
>+            die "MySQL is in ANSI mode, which is incompatible with Bugzilla.See bug 252555.";
>+        }
>+    }

This is not the right place to write this code, nor the right approach, IMO. We already have code in Bugzilla/DB/Mysql.pm, in new(), see lines 90-105, which checks what the SQL mode is. As sql_mode can be set dynamically, you should simply remove ANSI as well.

Attachment #8464758 - Flags: review?(gerv) → review-

Comment 19

•

10 years ago

LpSolit is right. Vishant: sorry for not knowing that and telling you earlier. Can you write a patch to match what he says?

Gerv

Flags: needinfo?(LpSolit)

Assignee

Comment 20

•

10 years ago

(In reply to Gervase Markham [:gerv] from comment #19)
> LpSolit is right. Vishant: sorry for not knowing that and telling you
> earlier. Can you write a patch to match what he says?
> 
> Gerv

Yes LpSolit is right. But i think we need to have two separate checks. One for installation and other for
run time.

Comment 21

•

10 years ago

(In reply to Vishant Gautam from comment #20)
> Yes LpSolit is right. But i think we need to have two separate checks. One
> for installation and other for
> run time.

Bugzilla::DB::Mysql::new() is also called at installation time. No need for two separate checks. And no reason for the ANSI mode to be treated differently from other modes.

Flags: needinfo?(LpSolit)

Assignee

Comment 22

•

10 years ago

Attached patch Mysql.pm.patch (obsolete) — Details — Splinter Review

Attachment #8451345 - Attachment is obsolete: true

Attachment #8454987 - Attachment is obsolete: true

Attachment #8459271 - Attachment is obsolete: true

Attachment #8460270 - Attachment is obsolete: true

Attachment #8464758 - Attachment is obsolete: true

Attachment #8471586 - Flags: review?(gerv)

Attachment #8471586 - Flags: review?(LpSolit)

Comment 23

•

10 years ago

Comment on attachment 8471586 [details] [diff] [review]
Mysql.pm.patch

As I said in my previous comment, there is no reason to treat the ANSI mode differently from other modes. As this mode can be modified dynamically, we should do it instead of dying.

Attachment #8471586 - Flags: review?(gerv)

Attachment #8471586 - Flags: review?(LpSolit)

Attachment #8471586 - Flags: review-

Assignee

Comment 24

•

10 years ago

(In reply to Frédéric Buclin from comment #23)
> Comment on attachment 8471586 [details] [diff] [review]
> Mysql.pm.patch
> 
> As I said in my previous comment, there is no reason to treat the ANSI mode
> differently from other modes. As this mode can be modified dynamically, we
> should do it instead of dying.

For dynamically setting MySQL in ANSI mode the MySQL user for BugZilla must 
have root privileges which can be a problem.

Comment 25

•

10 years ago

(In reply to Vishant Gautam from comment #24)
> For dynamically setting MySQL in ANSI mode the MySQL user for BugZilla must 
> have root privileges which can be a problem.

No need, see http://dev.mysql.com/doc/refman/5.5/en/sql-mode.html#sql-mode-setting:

"Setting the GLOBAL variable requires the SUPER privilege and affects the operation of all clients that connect from that time on. Setting the SESSION variable affects only the current client. Each client can change its session sql_mode value at any time."

We want to alter the session variable, not the global one. So you don't need any special privilege.

Assignee

Comment 26

•

10 years ago

Attached patch Mysql.pm.patch (obsolete) — Details — Splinter Review

Attachment #8489034 - Flags: review?(gerv)

Attachment #8489034 - Flags: review?(LpSolit)

Attachment #8489034 - Flags: feedback?(LpSolit)

Comment 27

•

10 years ago

Comment on attachment 8489034 [details] [diff] [review]
Mysql.pm.patch

>-    # Bug 252555 - disable MySQL ANSI mode, if set
>-    my @row = $self->selectrow_array('select @@global.sql_mode;');
>-    my @mode_tokens = split(/,/, $row[0]);

Your patch is reversed. It *removes* non-existent code. Also, we already have a SQL query to get sql_mode:


>     my ($var, $sql_mode) = $self->selectrow_array(
>         "SHOW VARIABLES LIKE 'sql\\_mode'");

Your code must be there. No need for a separate query.

Attachment #8489034 - Flags: review?(gerv)

Attachment #8489034 - Flags: review?(LpSolit)

Attachment #8489034 - Flags: review-

Attachment #8489034 - Flags: feedback?(LpSolit)

Assignee

Comment 28

•

10 years ago

Attached patch Mysql.pm.patch (obsolete) — Details — Splinter Review

Attachment #8489099 - Flags: review?(gerv)

Attachment #8489099 - Flags: review?(LpSolit)

Attachment #8489099 - Flags: feedback?

Comment 29

•

10 years ago

Comment on attachment 8489099 [details] [diff] [review]
Mysql.pm.patch

Please expand the existing regexp to exclude ANSI. Your patch really must be a one-liner; you don't need all this code. Also, I'm not sure REAL_AS_FLOAT is really a problem if set, so I don't think we need to take any special action against it.

Attachment #8489099 - Flags: review?(gerv)

Attachment #8489099 - Flags: review?(LpSolit)

Attachment #8489099 - Flags: review-

Attachment #8489099 - Flags: feedback?

Assignee

Comment 30

•

10 years ago

Expanding existing regexp means to expand the regexp for Bug 321645 
"grep {$_ !~ /^STRICT_(?:TRANS|ALL)_TABLES|TRADITIONAL$/}" to exclude ANSI.

Flags: needinfo?(LpSolit)

Comment 31

•

10 years ago

right!

Flags: needinfo?(LpSolit)

Assignee

Comment 32

•

10 years ago

Attached patch Mysql.pm.patch (obsolete) — Details — Splinter Review

Attachment #8489123 - Flags: review?(LpSolit)

Comment 33

•

10 years ago

Comment on attachment 8489123 [details] [diff] [review]
Mysql.pm.patch

>+    # Bug 252555 - disable MySQL ANSI mode, if set
>+    # Bug 321645 - disable MySQL strict mode, if set

As we have commit history, we don't need to list bug IDs in comments. I know 321645 was already mentioned, but this was old style. Both lines should go away and be replaced by something like: "Disable ANSI and strict modes, else Bugzilla will crash.".


>+        # ANSI is one of the combination modes of SQL. It is shorthand for 
>+        # combination of modes. ANSI mode is equivalent to REAL_AS_FLOAT, 
>+        # PIPES_AS_CONCAT, ANSI_QUOTES, IGNORE_SPACE.

This comment doesn't bring any value to why we are excluding the ANSI mode. This comment should go away.


>-            join(",", grep {$_ !~ /^STRICT_(?:TRANS|ALL)_TABLES|TRADITIONAL$/}
>+            join(",", grep {$_ !~ /^ANSI|STRICT_(?:TRANS|ALL)_TABLES|TRADITIONAL$/}
>                             split(/,/, $sql_mode));

On checkin, the regexp must be fixed to be enclosed within (?: ). This is not your fault, the mistake was already there, but:

  /^FOO|BAR$/ means "either starts with FOO, or ends with BAR", which is not what we want ("FOOXBAR" would match). We really want:

  /^(?:FOO|BAR)$/ which means "either is FOO or BAR".


You also have some trailing whitespaces here and there which should be removed.


r=LpSolit with all these comments addressed on checkin.

Attachment #8489123 - Flags: review?(LpSolit) → review+

Updated

•

10 years ago

Assignee: installation → gautamvishant

Status: NEW → ASSIGNED

Flags: approval?

Flags: approval4.4?

OS: Linux → All

Hardware: Other → All

Summary: Add check for mysql in ansi mode to checksetup → Remove the ANSI mode when running MySQL

Whiteboard: [good first bug]

Target Milestone: --- → Bugzilla 4.4

Updated

•

10 years ago

Attachment #8471586 - Attachment is obsolete: true

Updated

•

10 years ago

Attachment #8489034 - Attachment is obsolete: true

Updated

•

10 years ago

Attachment #8489099 - Attachment is obsolete: true

Simon Green

Updated

•

10 years ago

Flags: approval?

Flags: approval4.4?

Flags: approval4.4+

Flags: approval+

Comment 34

•

10 years ago

Attached patch checked in patch — Details — Splinter Review

Here is the patch I'm going to commit, including all my review comments.

Attachment #8489123 - Attachment is obsolete: true