If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[MS-SQL] Support MS SQL Server: Bugzilla::DB::Schema::Mssql

ASSIGNED

Status

()

Bugzilla
Database
--
enhancement
ASSIGNED
9 years ago
5 years ago

People

(Reporter: Michael Thomas (Mockodin), Assigned: Michael Thomas (Mockodin))

Tracking

(Blocks: 1 bug)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.0.8) Gecko/2009032609 Firefox/3.0.8
Build Identifier: Bugzilla CVS Head

Review ticket for adding Bugzilla::DB::Schema::Mssql for MSSQL support

Reproducible: Always
(Assignee)

Updated

9 years ago
Blocks: 285122
(Assignee)

Updated

9 years ago
No longer blocks: 285122

Updated

9 years ago
Blocks: 285122

Updated

9 years ago
Assignee: database → m.thomas
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows NT → All
Hardware: x86_64 → All
Target Milestone: --- → Bugzilla 4.0
(Assignee)

Comment 1

9 years ago
Created attachment 372705 [details] [diff] [review]
Initial Patch for Bugzilla::DB::Schema::Mssql
(Assignee)

Updated

9 years ago
Attachment #372705 - Flags: review?(mkanat)
(Assignee)

Comment 2

9 years ago
Created attachment 372955 [details] [diff] [review]
Patch for Bugzilla::DB::Schema::Mssql

Cleaned up Un-Needed Functions.
Attachment #372705 - Attachment is obsolete: true
Attachment #372955 - Flags: review?(mkanat)
Attachment #372705 - Flags: review?(mkanat)
(Assignee)

Comment 3

9 years ago
Found a bug creating the triggers and detecting existence of when dropping them.
Will post correction tonight.
(Assignee)

Comment 4

9 years ago
Created attachment 373700 [details] [diff] [review]
Patch Update - Corrects Trigger and FK creation for MSSQL
Attachment #372955 - Attachment is obsolete: true
Attachment #373700 - Flags: review?(mkanat)
Attachment #372955 - Flags: review?(mkanat)
(Assignee)

Comment 5

8 years ago
Created attachment 390547 [details] [diff] [review]
Patch v4

Corrected the creation logic on triggers to create single Update and delete trigger instead of one for one when using a trigger instead of a FKey. This required refactoring of the schema where references where concerned.

In the cases where a FKey is still used added logic to determine if index can be filtered (ignore nulls) or if a normal index should be used

A result of the mix triggers and fkeys is that schema changes will trigger them to be completely refreshed. By that I mean dropped and recreated.

This includes when creating custom multi select fields. Running checksetup against mssql takes this into account and rebuilds the custom field table fkeys/triggers in addition to the default bugzilla tables fkeys/triggers.

I need to run a few more tests against a loaded data set, about 200k bugs, 1.5M comments. And see if I need to change this a bit, I probably need to make this only drop and recreate if something changed for that specific FKey or associated tables.

A fair number of other adjustments, overall performance under mssql improved greatly. the biggest limitation at this point I think is the speed of TT2 under windows. Pages like the admin edit user screen are slow while it parses the product responsibilities (default assignee or QA contact etc..) I'm guessing these are a bit sluggish, comparatively to other pages,  under any OS and Web server however.
Attachment #373700 - Attachment is obsolete: true
Attachment #390547 - Flags: review?(mkanat)
Attachment #373700 - Flags: review?(mkanat)
(Assignee)

Updated

8 years ago
Attachment #390547 - Flags: review?(mkanat)
(Assignee)

Comment 6

8 years ago
Created attachment 429019 [details] [diff] [review]
v5
Attachment #390547 - Attachment is obsolete: true
(Assignee)

Comment 7

8 years ago
Pretty much ignore comment 5.

I moved these bad to one for one and all FKeys are now triggers. Why because mssql has issues with multipath cascading fkeys. The short answer is it doesn't work. So we have to use triggers instead. Currently all of the fkeys in bugzilla are cascade. If a restrict is added then we will need to alter the code to takes this in to account, ie tell the trigger to issue a roll back on a no action/restrict. Perhaps we should do it now.. but..

Comment 5 worked fine, until you upgraded to a newer version of Bugzilla and needed to selectively drop and recreate a trigger, Perhaps it can be done, I'm sure it can but for the purposes of simplicity and need it doesn't makes sense.
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Attachment #429019 - Flags: review?(mkanat)

Updated

8 years ago
Target Milestone: Bugzilla 4.0 → Bugzilla 3.8

Comment 8

8 years ago
(In reply to comment #7)
> Why because mssql has issues with multipath cascading fkeys.

  What do you mean by "multipath"?

> The short answer is it doesn't
> work. So we have to use triggers instead. Currently all of the fkeys in
> bugzilla are cascade. If a restrict is added then we will need to alter the
> code to takes this in to account, ie tell the trigger to issue a roll back on a
> no action/restrict. Perhaps we should do it now.. but..

  They're not all CASCADE. The ones that don't have anything specified have specified defaults in Schema.pm, which are RESTRICT for DELETE and CASCADE for UPDATE. There are definitely extant RESTRICT FKs.

Comment 9

8 years ago
Comment on attachment 429019 [details] [diff] [review]
v5

>+# The Initial Developer of the Original Code is Netscape Communications
>+# Corporation. Portions created by Netscape are
>+# Copyright (C) 1998 Netscape Communications Corporation. All
>+# Rights Reserved.

  Same note as the DB module.

>+# Many of the functions in this module were copied from Mysql.pm and Oracle.pm then altered for MSSQL

  Nit: Line too long.

>+package Bugzilla::DB::Schema::Mssql;
>+
>+###############################################################################
>+#
>+# DB::Schema implementation for MySQL
>+#
>+###############################################################################

  Not necessary and incorrect. :-)

>+use base qw(Bugzilla::DB::Schema);

  Usually we put this right under "use strict" or right under the package declaration.


>+use constant BOOLEAN_MAP => {

  You don't need this.

>+sub _initialize {
>+        BOOLEAN =>      'SMALLINT',

  Any reason to not use BIT? Or tinyint?

>+        INT1 =>         'int',
>+        INT2 =>         'int',
>+        INT3 =>         'int',
>+        INT4 =>         'int',

  Any reason to make these lowercase and other types uppercase? I think we generally make them lowercase here in the other drivers.

>+        TINYTEXT =>     'nvarchar(255)',
>+        MEDIUMTEXT =>   'nvarchar(4000)',
>+        LONGTEXT =>     'nvarchar(max)',
>+
>+        LONGBLOB =>     'varbinary(max)',
>+
>+        DATETIME =>     'smalldatetime',

  The MS-SQL docs seem to say that smalldatetime is only accurate to the...minute? That's not enough for Bugzilla.

>+	varchar =>      'nvarchar',

  Tab.

>+sub _adjust_schema {

>+        unless ($has_pk){
>+	    push @{ $self->{schema}{$table}{FIELDS} }, "$table\_id" => { TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1, };

  Instead of this, I think you should add these in bz_setup_database.

> [snip]
>+            $field_def->{TYPE} =~ m/^(\w*)/;
>+            my $datatype = $1 if $1;
> [snip]

  Why is all this this in here instead of in Schema.pm?

  If you're sharing this code with Oracle.pm, this code should be brought up to the superclass.

>+sub _get_add_indexes_ddl {
>+    my($self, @indexes) = @_;
>+
>+    
>+}

  Dead code.

>+sub get_add_fk_sql {
>+    my ($self, $table, $column, $def) = @_;
>+
>+    my $fk_string = $self->get_fk_ddl($table, $column, $def);
>+    my @sql = $self->create_trigger_ddl($table, $column, $def);
>+    push @sql, "ALTER TABLE $table ADD $fk_string" if $fk_string;
>+    return join("\n", @sql);

  No, you should be returning @sql, not a single string.

>+sub create_trigger_ddl {

>+       next unless $references->{$ACTION};

  As I mentioned in the comment above, this is probably wrong.

>+       my $name = $self->_get_trigger_name($ACTION,$references->{$ACTION},$ref_table,$ref_column,$table,$column);

  Nit: Long line and there should be spaces after the commas.

>+           my $sql .= "CREATE TRIGGER [$name] ON [$ref_table]\n";

  This is a great opportunity for a heredoc.

>+       } elsif ($ACTION eq 'DELETE' && $references->{$ACTION} eq 'SET NULL'){

  Nit: Long line, and put a space between ) and {.

>+=item C<get_table_ddl>

  You don't need to copy the POD.

>+    my($self, $table) = @_;

  Nit: Space after my.

>+    my @ddl = ();

  Nit: The = () isn't necessary.

>+    die "Table $table does not exist in the database schema."
>+        unless (ref($self->{schema}{$table}));
>+
>+    my $create_table = $self->_get_create_table_ddl($table);
>+    push(@ddl, $create_table) if $create_table;

  I don't think you should have to copy this whole method--could you find some way to avoid that? Refactoring existing code is acceptable.

>+sub get_add_fulltext {

  Maybe we should add in fulltext later, after the initial stuff. That way I can pay more attention to reviewing fulltext.

>+sub _get_create_table_ddl {
> [snip]

  This looks like a duplicate of the superclass's code...? Oh, except that you have brackets. Why do you have brackets? Are we using column names that are reserved words in MS-SQL?

>+sub _get_create_index_ddl {
>+    # If all fields are NOT NULL then the index should be a standard index
>+    # If it is mixed then ignore nulls

  What if the NULLness of a column changes after the index is created?

>+    my $allownull;
>+    foreach my $field (@$index_fields) {
>+        my %col_def = %{ $self->get_column_abstract($table_name, $field)};

  Nit: Space before }.

  It is generally not safe to do this, particularly since I think you're calling this in some places before the table has been inserted into bz_real_schema.

>+	if ( !defined $col_def{NOTNULL} || defined $col_def{NOTNULL} && $col_def{NOTNULL} == 0){

  We never set NOTNULL => 0. Just check $col_def{NOTNULL}.

>+    # Make it a filtered index IF we allow nulls certain conditions
>+    # In the case of multiple fields NOT NULL only allies when all fields are null else standard rules apply.
>+    $sql .= "WHERE [".join('] IS NOT NULL AND [', @$index_fields)."] IS NOT NULL\n" if $allownull;

  So IS NULL and IS NOT NULL won't use indexes?

>+sub get_create_database_sql {
>+    my ($self, $name) = @_;
>+
>+    return ("CREATE DATABASE [$name] ");
>+}

  If you need an identifier quoted, we should probably modify the superclass to do $dbh->quote_identifier.

>+sub get_set_serial_sql {
>+    my ($self, $table, $column, $value) = @_;
>+    return ("DBCC CHECKIDENT ($table, RESEED, $value)");
>+}

  This is used by Bugzilla::Migrate's --dry-run switch. Did you test it?

>+sub get_alter_column_ddl {
>+    if ($old_def->{PRIMARYKEY} && $new_def->{PRIMARYKEY}) {
>+        delete $new_def_copy{PRIMARYKEY};
>+    }
>+
>+    my $new_ddl = $self->get_alter_type_ddl(\%new_def_copy);

  I think you mean get_alter_type_sql.

>+    push(@statements, "UPDATE [$table] SET [$column] = $set_nulls_to
>+                        WHERE [$column] IS NULL") if defined $set_nulls_to;

  The superclass is already doing that, isn't it?

  Are you sure that the superclass implementation doesn't work all by itself? Or perhaps you could just do some modifications on the PK stuff before calling the superclass method.

>+sub get_alter_type_ddl {

  This should be get_alter_type_sql. It's confusing, I know. We should be more consistent about when we say DDL and when we say SQL.

>+sub get_rename_column_ddl {
>+    my ($self, $table, $old_name, $new_name) = @_;
>+    return ("EXEC sp_rename '$table.$old_name' '$new_name' 'COLUMN'");

  Is that a built-in or did you add it?

>+sub get_drop_column_ddl {
>+    my ($self, $table, $column) = @_;


>+    my $sql = "DECLARE \@default NVARCHAR(255)
>+    SELECT \@default = d.name FROM sys.default_constraints d
>+    INNER JOIN sys.columns c
>+    ON d.parent_column_id = c.column_id
>+    WHERE d.parent_object_id = OBJECT_ID(N'dbo.$table', N'U')
>+    AND c.name = '$column'
>+    IF (\@default is not null) BEGIN
>+        EXEC('ALTER TABLE [$table] DROP CONSTRAINT ['+\@default+']')
>+    END
>+    ALTER TABLE [$table] DROP COLUMN [$column]\n";

  Could you reformat that SQL according to the Developers Guide so that it's more readable?

>+    my $name = "fk_${to_table}_${to_column}_${table}_${column}";
>+    if (length($name) > $self->MAX_IDENTIFIER_LEN) {
>+        $name = 'fk_' . $self->_hash_identifier($name);
>+    }

  I think we should just move this into the superclass.

  Oh wait, it is in the superclass. You don't need to override this method.

>+sub _hash_identifier {

  This *is* in the superclass.

>+sub get_fk_ddl {
>+    my $sql = "\n     CONSTRAINT [$fk_name] FOREIGN KEY ($column)\n"
>+            . "      REFERENCES $ref_table($ref_column)\n";
>+       $sql .="      ON UPDATE $update" if $update && $update ne 'CASCADE';
>+       $sql .="      ON DELETE $delete" if $delete && $delete ne 'CASCADE' && $delete ne 'SET NULL';

  This looks exactly like the superclass implementation, except that you did [$fk_name] instead of just $fk_name.

>+sub get_add_column_ddl {

  This is also a duplicate of the superclass's code.
Attachment #429019 - Flags: review?(mkanat) → review-

Updated

7 years ago
Target Milestone: Bugzilla 4.0 → Bugzilla 5.0

Comment 10

6 years ago
Created attachment 562291 [details] [diff] [review]
v6
Attachment #562291 - Flags: review?(mkanat)

Comment 11

6 years ago
(In reply to Max Kanat-Alexander from comment #8)
>   They're not all CASCADE. The ones that don't have anything specified have
> specified defaults in Schema.pm, which are RESTRICT for DELETE and CASCADE
> for UPDATE. There are definitely extant RESTRICT FKs.

This has been taken care of. Now I am creating triggers even for ones that don't have them specified including rollback on RESTRICT.

(In reply to Max Kanat-Alexander from comment #9)
> Comment on attachment 429019 [details] [diff] [review] [diff] [details] [review]
> v5
> 
> >+# The Initial Developer of the Original Code is Netscape Communications
> >+# Corporation. Portions created by Netscape are
> >+# Copyright (C) 1998 Netscape Communications Corporation. All
> >+# Rights Reserved.
> 
>   Same note as the DB module.

Fixed.

> >+# Many of the functions in this module were copied from Mysql.pm and Oracle.pm then altered for MSSQL
> 
>   Nit: Line too long.

Fixed.

> >+package Bugzilla::DB::Schema::Mssql;
> >+
> >+###############################################################################
> >+#
> >+# DB::Schema implementation for MySQL
> >+#
> >+###############################################################################
> 
>   Not necessary and incorrect. :-)

Fixed.

> >+use base qw(Bugzilla::DB::Schema);
> 
>   Usually we put this right under "use strict" or right under the package
> declaration.

Done.

> 
> >+use constant BOOLEAN_MAP => {
> 
>   You don't need this.

Removed.
 
> >+sub _initialize {
> >+        BOOLEAN =>      'SMALLINT',
> 
>   Any reason to not use BIT? Or tinyint?

No reason I could see. Changed to bit.

> >+        INT1 =>         'int',
> >+        INT2 =>         'int',
> >+        INT3 =>         'int',
> >+        INT4 =>         'int',
> 
>   Any reason to make these lowercase and other types uppercase? I think we
> generally make them lowercase here in the other drivers.

Changed all to lowercase.

> >+        TINYTEXT =>     'nvarchar(255)',
> >+        MEDIUMTEXT =>   'nvarchar(4000)',
> >+        LONGTEXT =>     'nvarchar(max)',
> >+
> >+        LONGBLOB =>     'varbinary(max)',
> >+
> >+        DATETIME =>     'smalldatetime',
> 
>   The MS-SQL docs seem to say that smalldatetime is only accurate to
> the...minute? That's not enough for Bugzilla.

Changed to datetime.

> >+	varchar =>      'nvarchar',
> 
>   Tab.

Fixed.

> >+sub _adjust_schema {
> 
> >+        unless ($has_pk){
> >+	    push @{ $self->{schema}{$table}{FIELDS} }, "$table\_id" => { TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1, };
> 
>   Instead of this, I think you should add these in bz_setup_database.

Done.

> > [snip]
> >+            $field_def->{TYPE} =~ m/^(\w*)/;
> >+            my $datatype = $1 if $1;
> > [snip]
> 
>   Why is all this this in here instead of in Schema.pm?

I've removed this from here now and added it into the Schema.pm. This will be handled in Bug 505362.
 
> >+sub _get_add_indexes_ddl {
> >+    my($self, @indexes) = @_;
> >+
> >+    
> >+}
> 
>   Dead code.

Removed.

> 
> >+sub get_add_fk_sql {
> >+    my ($self, $table, $column, $def) = @_;
> >+
> >+    my $fk_string = $self->get_fk_ddl($table, $column, $def);
> >+    my @sql = $self->create_trigger_ddl($table, $column, $def);
> >+    push @sql, "ALTER TABLE $table ADD $fk_string" if $fk_string;
> >+    return join("\n", @sql);
> 
>   No, you should be returning @sql, not a single string.

Fixed.

> >+sub create_trigger_ddl {
> 
> >+       next unless $references->{$ACTION};
> 
>   As I mentioned in the comment above, this is probably wrong.

Fixed. 

> >+       my $name = $self->_get_trigger_name($ACTION,$references->{$ACTION},$ref_table,$ref_column,$table,$column);
> 
>   Nit: Long line and there should be spaces after the commas.
> 

Fixed.

> >+           my $sql .= "CREATE TRIGGER [$name] ON [$ref_table]\n";
> 
>   This is a great opportunity for a heredoc.
> 

Done.

> >+       } elsif ($ACTION eq 'DELETE' && $references->{$ACTION} eq 'SET NULL'){
> 
>   Nit: Long line, and put a space between ) and {.
> 

Done.

> >+=item C<get_table_ddl>
> 
>   You don't need to copy the POD.
> 

Removed.

> >+    my($self, $table) = @_;
> 
>   Nit: Space after my.

Fixed.

> 
> >+    my @ddl = ();
> 
>   Nit: The = () isn't necessary.

Fixed.

> 
> >+    die "Table $table does not exist in the database schema."
> >+        unless (ref($self->{schema}{$table}));
> >+
> >+    my $create_table = $self->_get_create_table_ddl($table);
> >+    push(@ddl, $create_table) if $create_table;
> 
>   I don't think you should have to copy this whole method--could you find
> some way to avoid that? Refactoring existing code is acceptable.

Method looks quite different to the original one. I can't see a good way to avoid doing this.

> >+sub get_add_fulltext {
> 
>   Maybe we should add in fulltext later, after the initial stuff. That way I
> can pay more attention to reviewing fulltext.
> 

Reformatted SQL.

> >+sub _get_create_table_ddl {
> > [snip]
> 
>   This looks like a duplicate of the superclass's code...? Oh, except that
> you have brackets. Why do you have brackets? Are we using column names that
> are reserved words in MS-SQL?

TheSchwartz has a table that uses coalesce for a column name which is a reserved word in Mssql.

> >+sub _get_create_index_ddl {
> >+    # If all fields are NOT NULL then the index should be a standard index
> >+    # If it is mixed then ignore nulls
> 
>   What if the NULLness of a column changes after the index is created?
> 

This won't affect the existing index except that it probably will perform slightly worse than just a standard index would. Maybe we need to drop and recreate indexes at some point? This could be very time consuming on large databases.

> >+    my $allownull;
> >+    foreach my $field (@$index_fields) {
> >+        my %col_def = %{ $self->get_column_abstract($table_name, $field)};
> 
>   Nit: Space before }.
  
Fixed.

>   It is generally not safe to do this, particularly since I think you're
> calling this in some places before the table has been inserted into
> bz_real_schema.

I couldn't see where this is being called before table has been inserted.

> >+	if ( !defined $col_def{NOTNULL} || defined $col_def{NOTNULL} && $col_def{NOTNULL} == 0){
> 
>   We never set NOTNULL => 0. Just check $col_def{NOTNULL}.

Done.

> >+    # Make it a filtered index IF we allow nulls certain conditions
> >+    # In the case of multiple fields NOT NULL only allies when all fields are null else standard rules apply.
> >+    $sql .= "WHERE [".join('] IS NOT NULL AND [', @$index_fields)."] IS NOT NULL\n" if $allownull;
> 
>   So IS NULL and IS NOT NULL won't use indexes?

Indexing will only be used on NOT NULL fields which will reduce the index size and improve performance.

> 
> >+sub get_create_database_sql {
> >+    my ($self, $name) = @_;
> >+
> >+    return ("CREATE DATABASE [$name] ");
> >+}
> 
>   If you need an identifier quoted, we should probably modify the superclass
> to do $dbh->quote_identifier.

Sounds like a good idea, but do we need to do this now?

> >+sub get_set_serial_sql {
> >+    my ($self, $table, $column, $value) = @_;
> >+    return ("DBCC CHECKIDENT ($table, RESEED, $value)");
> >+}
> 
>   This is used by Bugzilla::Migrate's --dry-run switch. Did you test it?

If you can provide me with a sample GNATS database I can give this a try. SQL statement is valid and will reset the identity number as needed.

> >+sub get_alter_column_ddl {
> >+    if ($old_def->{PRIMARYKEY} && $new_def->{PRIMARYKEY}) {
> >+        delete $new_def_copy{PRIMARYKEY};
> >+    }
> >+
> >+    my $new_ddl = $self->get_alter_type_ddl(\%new_def_copy);
> 
>   I think you mean get_alter_type_sql.

Changed to get_alter_type_sql.

> >+    push(@statements, "UPDATE [$table] SET [$column] = $set_nulls_to
> >+                        WHERE [$column] IS NULL") if defined $set_nulls_to;
> 
>   The superclass is already doing that, isn't it?
> 
>   Are you sure that the superclass implementation doesn't work all by
> itself? Or perhaps you could just do some modifications on the PK stuff
> before calling the superclass method.
> 

I think it is at some point, probably not at the same point, as the method again is quite different to superclass and I am not sure of a good way to do this.

> 
> >+sub get_rename_column_ddl {
> >+    my ($self, $table, $old_name, $new_name) = @_;
> >+    return ("EXEC sp_rename '$table.$old_name' '$new_name' 'COLUMN'");
> 
>   Is that a built-in or did you add it?

Built-in.

> 
> >+sub get_drop_column_ddl {
> >+    my ($self, $table, $column) = @_;
> 
> 
> >+    my $sql = "DECLARE \@default NVARCHAR(255)
> >+    SELECT \@default = d.name FROM sys.default_constraints d
> >+    INNER JOIN sys.columns c
> >+    ON d.parent_column_id = c.column_id
> >+    WHERE d.parent_object_id = OBJECT_ID(N'dbo.$table', N'U')
> >+    AND c.name = '$column'
> >+    IF (\@default is not null) BEGIN
> >+        EXEC('ALTER TABLE [$table] DROP CONSTRAINT ['+\@default+']')
> >+    END
> >+    ALTER TABLE [$table] DROP COLUMN [$column]\n";
> 
>   Could you reformat that SQL according to the Developers Guide so that it's
> more readable?

Done.

> 
> >+    my $name = "fk_${to_table}_${to_column}_${table}_${column}";
> >+    if (length($name) > $self->MAX_IDENTIFIER_LEN) {
> >+        $name = 'fk_' . $self->_hash_identifier($name);
> >+    }
> 
>   I think we should just move this into the superclass.
> 
>   Oh wait, it is in the superclass. You don't need to override this method.

Removed method.

> 
> >+sub _hash_identifier {
> 
>   This *is* in the superclass.

Removed method.

> 
> >+sub get_fk_ddl {
> >+    my $sql = "\n     CONSTRAINT [$fk_name] FOREIGN KEY ($column)\n"
> >+            . "      REFERENCES $ref_table($ref_column)\n";
> >+       $sql .="      ON UPDATE $update" if $update && $update ne 'CASCADE';
> >+       $sql .="      ON DELETE $delete" if $delete && $delete ne 'CASCADE' && $delete ne 'SET NULL';
> 
>   This looks exactly like the superclass implementation, except that you did
> [$fk_name] instead of just $fk_name.

Possibly worth discussing later if adding quoted identifier is preferable.

> 
> >+sub get_add_column_ddl {
> 
>   This is also a duplicate of the superclass's code.

Removed.

Comment 12

6 years ago
Comment on attachment 562291 [details] [diff] [review]
v6

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

Hey! I had a few seconds to start looking through this patch, and I'm really happy you're doing the work on it. :-) I'm not able to finish the review right now--something came up and I have to leave--but I thought you might at least like to see my first few comments:

::: Bugzilla/DB/Schema/Mssql.pm
@@ +8,5 @@
> +# Software distributed under the License is distributed on an "AS
> +# IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
> +# implied. See the License for the specific language governing
> +# rights and limitations under the License.
> +#

This needs an Initial Developer block.

@@ +11,5 @@
> +# rights and limitations under the License.
> +#
> +# Contributor(s): 
> +#                 Michael Thomas <mockodin@gmail.com>
> +#                 Jasmin Sehic <jasmins@embed.com.au>

There's too much indentation there.

@@ +19,5 @@
> +###############################################################################
> +#
> +# DB::Schema implementation for MSSQL
> +#
> +###############################################################################

That comment probably isn't necessary.

@@ +90,5 @@
> +            }
> +        }
> +        my $index_sql  = $self->get_add_index_ddl($table, $index_name, $index_info);
> +        push(@ddl, $index_sql) if $index_sql;
> +    }

Everything above this point looks identical to the superclass. As such, it could be factored out into a separate method in the superclass and then called here.

@@ +95,5 @@
> +
> +    if (@ft_columns) {    
> +        my $fulltext_sql = $self->get_add_fulltext($table, @ft_columns);
> +        push(@ddl, $fulltext_sql);
> +    }

We removed fulltext, right? So we shouldn't need this.
Attachment #562291 - Flags: review?(mkanat) → review-

Comment 13

6 years ago
Created attachment 564481 [details] [diff] [review]
v7
Attachment #562291 - Attachment is obsolete: true
Attachment #564481 - Flags: review?(mkanat)

Comment 14

6 years ago
(In reply to Max Kanat-Alexander from comment #12)
> Comment on attachment 562291 [details] [diff] [review] [diff] [details] [review]
> v6
> 
> Review of attachment 562291 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Hey! I had a few seconds to start looking through this patch, and I'm really
> happy you're doing the work on it. :-) I'm not able to finish the review
> right now--something came up and I have to leave--but I thought you might at
> least like to see my first few comments:
> 
> ::: Bugzilla/DB/Schema/Mssql.pm
> @@ +8,5 @@
> > +# Software distributed under the License is distributed on an "AS
> > +# IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
> > +# implied. See the License for the specific language governing
> > +# rights and limitations under the License.
> > +#
> 
> This needs an Initial Developer block.

Added.

> 
> @@ +11,5 @@
> > +# rights and limitations under the License.
> > +#
> > +# Contributor(s): 
> > +#                 Michael Thomas <mockodin@gmail.com>
> > +#                 Jasmin Sehic <jasmins@embed.com.au>
> 
> There's too much indentation there.

Removed.

> 
> @@ +19,5 @@
> > +###############################################################################
> > +#
> > +# DB::Schema implementation for MSSQL
> > +#
> > +###############################################################################
> 
> That comment probably isn't necessary.

Removed.

> 
> @@ +90,5 @@
> > +            }
> > +        }
> > +        my $index_sql  = $self->get_add_index_ddl($table, $index_name, $index_info);
> > +        push(@ddl, $index_sql) if $index_sql;
> > +    }
> 
> Everything above this point looks identical to the superclass. As such, it
> could be factored out into a separate method in the superclass and then
> called here.

After taking out fulltext specifics this functions was removed as it was identical to the superclass.

> 
> @@ +95,5 @@
> > +
> > +    if (@ft_columns) {    
> > +        my $fulltext_sql = $self->get_add_fulltext($table, @ft_columns);
> > +        push(@ddl, $fulltext_sql);
> > +    }
> 
> We removed fulltext, right? So we shouldn't need this.

Removed.

Updated

6 years ago
Attachment #429019 - Attachment is obsolete: true

Comment 15

6 years ago
Comment on attachment 564481 [details] [diff] [review]
v7

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

Thanks for all of your work on this. :-) I have a lot of comments, but don't get discouraged--this can totally get in with some more work.

::: Bugzilla/DB/Schema/Mssql.pm
@@ +52,5 @@
> +        MEDIUMSERIAL => '[int] identity(1,1)',
> +        INTSERIAL =>    '[int] identity(1,1)',
> +
> +        TINYTEXT =>     'nvarchar(255)',
> +        MEDIUMTEXT =>   'nvarchar(4000)',

Is 4000 the limit? Does anything bad happen if we go over 4000? The only reason we list this as being 4000 long is because that was Oracle's limit. I believe the limit in MySQL is more like 65535 characters.

@@ +68,5 @@
> +    return $self;
> +}
> +
> +sub _get_create_table_ddl {
> +    my($self, $table) = @_;

Nit: Space after my.

@@ +88,5 @@
> +    }
> +
> +    $create_table .= "\)";
> +    return($create_table)
> +}

The *only* functional difference that I see here is that you're putting [] around the field names. Why do you need to do that? If we have fields that are actually reserved words, we won't be able to access them in Bugzilla's normal SQL anyway. Don't duplicate this code from the superclass. I know that you said it's not identical, but other than adding the [], it is identical in functionality. You removed the FK_ON_CREATE code, but that code won't trigger for you anyway--FK_ON_CREATE is false!

@@ +91,5 @@
> +    return($create_table)
> +}
> +
> +sub _get_create_index_ddl {
> +    my($self, $table_name, $index_name, $index_fields, $index_type) = @_;

Nit: Space after my.

@@ +108,5 @@
> +
> +    $sql .= "CREATE ";
> +    $sql .= "$index_type " if $index_type eq 'UNIQUE';
> +    $sql .= "INDEX [$index_name] ON [$table_name] 
> +             ([".join("], [", @$index_fields)."])\n";

You should be generating this by calling the superclass method, which already does exactly this (except that it doesn't use the brackets, but are those really necessary? see my earlier comment about reserved words).

@@ +115,5 @@
> +    # In the case of multiple fields NOT NULL only allies when all fields 
> +    # are null else standard rules apply.
> +    if ($allownull) {
> +        $sql .= "WHERE [".join('] IS NOT NULL 
> +                 AND [', @$index_fields)."] IS NOT NULL\n";

If you're going to do this, you also have to handle it in bz_alter_column. What happens if a column is NULL and becomes NOT NULL? What happens if a column is NOT NULL and becomes NULL?

If this is just a speed or space optimization, get rid of it. I'd rather not have the complexity.

@@ +122,5 @@
> +}
> +
> +sub get_create_database_sql {
> +    my ($self, $name) = @_;
> +    return ("CREATE DATABASE [$name] ");

Okay, I think we need to resolve what's up with the brackets. If these really are required, you should make the superclass use $dbh->quote_identifier when necessary. But I can't imagine why they'd be required.

@@ +132,5 @@
> +        # if the only change is a case change, return an empty list, since MSSQL
> +        # is case-insensitive and will return an error about a duplicate name
> +        return ();
> +    }
> +    return ("EXEC sp_rename '$old_name', '$new_name';");

You don't need the semicolon.

@@ +140,5 @@
> +    my ($self, $table, $column, $value) = @_;
> +    return ("DBCC CHECKIDENT ($table, RESEED, $value)");
> +}
> +
> +sub get_alter_column_ddl {

Okay, so, this duplicates a lot of logic from the superclass. Instead of duplicating this logic, what you should do is refactor the superclass so that it calls things like _get_drop_primary_key_sql and _get_add_primary_key_sql where it's adding sql stuff. Then you implement those in your subclass.

@@ +146,5 @@
> +    my $old_def = $self->get_column($table, $column);
> +    my %new_def_copy = %$new_def;
> +    if ($old_def->{PRIMARYKEY} && $new_def->{PRIMARYKEY}) {
> +        delete $new_def_copy{PRIMARYKEY};
> +    }

Why is this code way up here? It should be down with the PK code. Also, please don't modify the input hashes, you could be modifying something that will get saved to the database. Instead, just keep the logic similar to the superclass's logic.

@@ +166,5 @@
> +    # first drops existing default if any
> +    if (defined $default) {
> +        push(@statements, "sp_alter_column_default '$table', '$column', 
> +                           '$default'");
> +    }

What if you're just removing the default, so no default is defined?

@@ +169,5 @@
> +                           '$default'");
> +    }
> +
> +    my $primarykey = $new_def_copy{PRIMARYKEY};
> +    # first drops existing primary key if any

I don't think that comment is necessary, except perhaps inside of the if block?

@@ +172,5 @@
> +    my $primarykey = $new_def_copy{PRIMARYKEY};
> +    # first drops existing primary key if any
> +    if (defined $primarykey) {
> +        push(@statements, "sp_alter_primarykey '$table', '$column'");  
> +    }

What if this table no longer has a primary key? There could be situations where you go from having a PK on this column to not having a PK on this column.

@@ +177,5 @@
> +
> +    return @statements;
> +}
> +
> +sub get_alter_type_sql {

When a function's name ends with _sql, it's supposed to return full SQL statements, not pieces of DDL.

@@ +195,5 @@
> +    my ($table, $column_fks) = @_;
> +    my @sql;
> +    
> +    # rely on triggers instead of foreign keys
> +    push @sql, $self->_create_trigger_ddl($table, $column_fks);

So, it looks like the whole purpose of get_add_fks_sql is to call _create_trigger_ddl. Why don't you just put _create_trigger_ddl's code in here, instead?

@@ +203,5 @@
> +
> +sub _create_trigger_ddl {
> +    my ($self, $table, $column_fks) = @_;  
> +    my $trigger;  
> +    my @sql;

Don't predeclare variables like this. Declare them only before you use them.

@@ +208,5 @@
> +    
> +    foreach my $column (keys %$column_fks) {
> +        my $fk = $column_fks->{$column};
> +        my $to_column = $fk->{COLUMN};
> +        my $to_table  = $fk->{TABLE};

I think it might be nice to factor out the inside of this loop into a separate function, like _get_add_fk_sql that takes $table, $column, and $fk.

@@ +210,5 @@
> +        my $fk = $column_fks->{$column};
> +        my $to_column = $fk->{COLUMN};
> +        my $to_table  = $fk->{TABLE};
> +
> +        foreach my $ACTION (qw{UPDATE DELETE}) {            

Don't use all-caps variable names. Also, usually we do qw(), not qw{}.

@@ +212,5 @@
> +        my $to_table  = $fk->{TABLE};
> +
> +        foreach my $ACTION (qw{UPDATE DELETE}) {            
> +            # set up defaults
> +            unless ($fk->{$ACTION}) {

Usually we avoid "unless". It would be best to do if (! here, instead.

@@ +217,5 @@
> +                if ($ACTION eq 'UPDATE') {
> +                    $fk->{UPDATE} = 'CASCADE';
> +                } elsif ($ACTION eq 'DELETE') {
> +                    $fk->{DELETE} = 'RESTRICT';
> +                }

Don't modify $fk--that modifies the input hash for callers. Instead, you should store the restriction type in some separate variable and set it to its default as necessary. Either that, or create a superclass method called _set_fk_defaults($fk) that returns a copy of $fk with the correct default values set. (I would prefer this second option, and I'd prefer that you made both the superclass and all existing subclasses use it.)

@@ +221,5 @@
> +                }
> +            }            
> +            my $name = $self->_get_trigger_name($ACTION,$fk->{$ACTION},
> +                                                $to_table,$to_column,$table,
> +                                                $column);                                             

Nit: spaces after commas.

@@ +222,5 @@
> +            }            
> +            my $name = $self->_get_trigger_name($ACTION,$fk->{$ACTION},
> +                                                $to_table,$to_column,$table,
> +                                                $column);                                             
> +            if ($fk->{$ACTION} eq 'CASCADE') {

Can we factor out this entire if/else set into a function called _get_create_fk_sql or something? Just for readability.

@@ +225,5 @@
> +                                                $column);                                             
> +            if ($fk->{$ACTION} eq 'CASCADE') {
> +                if ($ACTION eq 'UPDATE') {
> +                     $trigger = <<ENDSQL;                     
> +CREATE TRIGGER [$name] ON [$to_table]

Thanks for the indentation on all of these. :-) They're actually very readable, even though I don't know MS-SQL's stored query language.

@@ +226,5 @@
> +            if ($fk->{$ACTION} eq 'CASCADE') {
> +                if ($ACTION eq 'UPDATE') {
> +                     $trigger = <<ENDSQL;                     
> +CREATE TRIGGER [$name] ON [$to_table]
> +           FOR $ACTION

You know what $ACTION is for all of these SQL statements, you might as well write it out instead of writing in $ACTION. Either that, or you should store the top of the CREATE TRIGGER statement in a variable somewhere and re-use it. I'm happy either way, as long as things stay readable.

@@ +239,5 @@
> +                 UPDATE [$table] SET [$column] = \@NEW WHERE $column = \@OLD
> +               END
> +           END               
> +ENDSQL
> +                    push(@sql, $trigger);

You're only setting $trigger once, right? So just push it once, at the end.

@@ +269,5 @@
> +               SELECT \@OLD = [$to_column] FROM DELETED
> +               IF (\@NEW <> \@OLD) BEGIN
> +                   IF EXISTS (SELECT [$column] FROM [$table] 
> +                              WHERE [$column] = \@OLD) BEGIN
> +                       RAISERROR('UPDATE not allowed due to RESTRICT', 16, 1)

What does 16, 1 mean, there? It would be nice to have a comment explaining it.

@@ +277,5 @@
> +               END               
> +           END               
> +ENDSQL
> +                    push(@sql, $trigger);
> +                } elsif ($ACTION eq 'DELETE') {

Perl style is that the elsif comes on the next line, aligned with the closing curly-brace. Like:

if (condition) {
  blah
}
else {
  blah
}

@@ +291,5 @@
> +                          WHERE [$column] = \@OLD) BEGIN
> +                   RAISERROR('DELETE not allowed due to RESTRICT', 16, 1)
> +                   ROLLBACK TRAN
> +                   RETURN
> +               END 

It looks like this IF EXISTS block is re-used, perhaps it should be stored in a variable?

@@ +296,5 @@
> +           END
> +ENDSQL
> +                    push(@sql, $trigger);
> +                }
> +            } elsif ($ACTION eq 'DELETE' && $fk->{$ACTION} eq 'SET NULL') {

You're never going to hit this--you already checked if $ACTION eq 'DELETE' above.

@@ +317,5 @@
> +}
> +
> +sub get_drop_fk_sql {
> +    my ($self, $table, $column, $references) = @_;
> +    my $sql = '';

Set this right before you need it, not at the top of the method.

@@ +320,5 @@
> +    my ($self, $table, $column, $references) = @_;
> +    my $sql = '';
> +
> +    $references->{UPDATE} ||= 'RESTRICT';
> +    $references->{DELETE} ||= 'RESTRICT';

Don't modify the input hash. Instead, just do what the superclass does:

my $update = $references->{UPDATE} || 'CASCADE';
my $delete = $references->{DELETE} || 'RESTRICT';

Also, note that your default action for UPDATE is incorrect.

@@ +323,5 @@
> +    $references->{UPDATE} ||= 'RESTRICT';
> +    $references->{DELETE} ||= 'RESTRICT';
> +
> +    if ($references->{UPDATE} eq 'RESTRICT' || 
> +        $references->{DELETE} eq 'RESTRICT' ) {

When you have a multi-line if, line up the ending curly-brace with the "i" in "if", on the next line.

@@ +325,5 @@
> +
> +    if ($references->{UPDATE} eq 'RESTRICT' || 
> +        $references->{DELETE} eq 'RESTRICT' ) {
> +        my $fk_name = $self->_get_fk_name($table, $column, $references);
> +        $sql .= "IF EXISTS "

It'd be nice to use the <<ENDSQL method you used for triggers above, here.

@@ +334,5 @@
> +    }
> +
> +    my $ref_table  = $references->{TABLE};
> +    my $ref_column = $references->{COLUMN};
> +    foreach my $ACTION (qw{UPDATE DELETE}) {

Don't use all-caps variable names. Use qw() instead of qw{}.

@@ +339,5 @@
> +        if ($references->{$ACTION} eq 'CASCADE' 
> +         || $references->{$ACTION} eq 'SET NULL'
> +         || $references->{$ACTION} eq 'RESTRICT') {
> +            my $name = $self->_get_trigger_name($ACTION,$references->{$ACTION},
> +                               $ref_table,$ref_column,$table,$column);

Spaces after commas.

@@ +346,5 @@
> +                 .  "BEGIN\n"
> +                 .  "    DROP TRIGGER [$name]\n"
> +                 .  "END\n";
> +        }
> +    }

Why are you appending strings to $sql instead of creating an array of SQL statements and returning that?

Also, why are you checking if these things exist? (That is, why use IF EXISTS?) Bugzilla::DB handles failures itself, you shouldn't mask them from the caller.

@@ +373,5 @@
> +      WHERE d.parent_object_id = OBJECT_ID(N'dbo.$table', N'U')
> +        AND c.name = '$column'
> +         IF (\@default is not null) BEGIN
> +            EXEC('ALTER TABLE [$table] DROP CONSTRAINT ['+\@default+']')
> +        END 

All of these methods are designed to return lists. So instead of this complex T-SQL block, you should check if the column has a default defined, and add the SQL to the returned list.

@@ +382,5 @@
> +
> +sub _get_trigger_name {
> +    my $self = shift;
> +    my ($action, $type, $ref_table, $ref_column, $table, $column) = @_;
> +    my $id = "tr_$action\_$type\_$ref_table\_$ref_column\_$table\_$column";

Instead of escaping the underscores, you should do:

  my $id = "tr_${action}_${type}_" and so on. Also, you should start the name with ${table}_{$column}_${to_table}_${to_column}, like get_fk_name does.

  In fact, why do you need $action and $type in the name? Is there more than one trigger per FK?
Attachment #564481 - Flags: review?(mkanat) → review-

Comment 16

5 years ago
We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.
Target Milestone: Bugzilla 4.4 → ---

Comment 17

5 years ago
I do have another patch and I will post it here in another couple of weeks. 

Main issue I have come across recently was that any updates to serial columns (identity in MSSQL) require the entire table to be dropped and recreated which is a bit finicky.
You need to log in before you can comment on or make changes to this bug.