From fa718e22ea0fbcb69996e7908e950b26fe3f6312 Mon Sep 17 00:00:00 2001 From: Marc0 Date: Sat, 4 Feb 2017 18:38:24 +0100 Subject: [PATCH 1/3] fix: Don't add indices on columns with UNIQUE constraint SQL::Translator::Parser::DBIx::Class adds an index to foreign key columns as this "is normally the sensible thing to do". Agreed. Besides SQL::Translator::Parser::DBIx::Class takes care to not add an additional index to primary key columns ("some RDBMS croak on this, and it generally doesn't make much sense") but doesn't consider columns with a UNIQUE constraint where it doesn't make any sense either. Generally this should not be dangerous but it doesn't help at all. From my understanding the SQL optimizer will use the better index thus the UNIQUE constraint would win in any case. Even worse any INSERT, UPDATE or DELETE operation would need to maintain the index which is never used. This does not sound sensible to me. The supposed patch avoids the adding of those indices if the column is checked if it is *the first column* of a UNIQUE constraint. If it's not the index is added, if it is the first column it is handled like a primary key. --- lib/SQL/Translator/Parser/DBIx/Class.pm | 10 ++++++++++ t/86sqlt.t | 17 ++++++++++++----- t/99dbic_sqlt_parser.t | 2 ++ t/lib/sqlite.sql | 4 ---- 4 files changed, 24 insertions(+), 9 deletions(-) diff --git a/lib/SQL/Translator/Parser/DBIx/Class.pm b/lib/SQL/Translator/Parser/DBIx/Class.pm index e197491aa..ad23b6e2d 100644 --- a/lib/SQL/Translator/Parser/DBIx/Class.pm +++ b/lib/SQL/Translator/Parser/DBIx/Class.pm @@ -146,6 +146,7 @@ sub parse { $table->primary_key(@primary) if @primary; my %unique_constraints = $source->unique_constraints; + my @unique; foreach my $uniq (sort keys %unique_constraints) { if (!$source->_compare_relationship_keys($unique_constraints{$uniq}, \@primary)) { $table->add_constraint( @@ -153,8 +154,12 @@ sub parse { name => $uniq, fields => $unique_constraints{$uniq} ); + # store first column of unique constraint to avoid useless + # automatic index creation over this column later + push @unique, $unique_constraints{$uniq}->[0]; } } + #{ local $" = q(, ); warn qq(Unique columns [$table_name]: @unique); } my @rels = $source->relationships(); @@ -284,6 +289,11 @@ sub parse { # needed next if join("\x00", @keys) eq join("\x00", @primary); + # Check that we do not create an index on a column being the + # first column of a unique constraint + #{ local $" = q(, ); warn qq(Checking for unique columns [$table_name]: [@keys] vs. [@unique]); } + next if join("\x00", @keys) eq join("\x00", @unique); + if ($add_fk_index_rel) { (my $idx_name = $table_name) =~ s/ ^ [^\.]+ \. //x; my $index = $table->add_index( diff --git a/t/86sqlt.t b/t/86sqlt.t index eccc5e245..3c7795a53 100644 --- a/t/86sqlt.t +++ b/t/86sqlt.t @@ -225,11 +225,18 @@ my %fk_constraints = ( # CD cd => [ { - 'display' => 'cd->artist', - 'name' => 'cd_fk_artist', 'index_name' => 'cd_idx_artist', - 'selftable' => 'cd', 'foreigntable' => 'artist', - 'selfcols' => ['artist'], 'foreigncols' => ['artistid'], - on_delete => 'CASCADE', on_update => 'CASCADE', deferrable => 1, + 'display' => 'cd->single_track', + 'name' => 'cd_fk_single_track', 'index_name' => 'cd_idx_single_track', + 'selftable' => 'cd', 'foreigntable' => 'track', + 'selfcols' => ['single_track'], 'foreigncols' => ['trackid'], + on_delete => 'CASCADE', on_update => '', deferrable => 1, + }, + { + 'display' => 'cd->genreid', + 'name' => 'cd_fk_genreid', 'index_name' => 'cd_idx_genreid', + 'selftable' => 'cd', 'foreigntable' => 'genre', + 'selfcols' => ['genreid'], 'foreigncols' => ['genreid'], + on_delete => 'SET NULL', on_update => 'CASCADE', deferrable => 1, }, ], diff --git a/t/99dbic_sqlt_parser.t b/t/99dbic_sqlt_parser.t index 0b368508b..55e8a91c3 100644 --- a/t/99dbic_sqlt_parser.t +++ b/t/99dbic_sqlt_parser.t @@ -108,6 +108,8 @@ my $idx_exceptions = { 'ForceForeign' => -1, 'LinerNotes' => -1, 'TwoKeys' => -1, # TwoKeys has the index turned off on the rel def + 'CD' => -1, # "track_cd_title" is UNIQUE constraint + 'LyricVersion' => -1, # "lyric_versions" is UNIQUE constraint }; { diff --git a/t/lib/sqlite.sql b/t/lib/sqlite.sql index 64ddc33c3..28d6dcd96 100644 --- a/t/lib/sqlite.sql +++ b/t/lib/sqlite.sql @@ -239,8 +239,6 @@ CREATE TABLE "cd" ( FOREIGN KEY ("genreid") REFERENCES "genre"("genreid") ON DELETE SET NULL ON UPDATE CASCADE ); -CREATE INDEX "cd_idx_artist" ON "cd" ("artist"); - CREATE INDEX "cd_idx_single_track" ON "cd" ("single_track"); CREATE INDEX "cd_idx_genreid" ON "cd" ("genreid"); @@ -285,8 +283,6 @@ CREATE TABLE "lyric_versions" ( FOREIGN KEY ("lyric_id") REFERENCES "lyrics"("lyric_id") ON DELETE CASCADE ON UPDATE CASCADE ); -CREATE INDEX "lyric_versions_idx_lyric_id" ON "lyric_versions" ("lyric_id"); - CREATE UNIQUE INDEX "lyric_versions_lyric_id_text" ON "lyric_versions" ("lyric_id", "text"); CREATE TABLE "tags" ( From fa24a5ba9b49604203f6f42f2f6f1989a71ce0b9 Mon Sep 17 00:00:00 2001 From: Marc0 Date: Sun, 5 Feb 2017 20:25:12 +0100 Subject: [PATCH 2/3] test: Add test for redundant indices I want to achieve a test checking the given schema for automatic indices on foreign key columns actually being covered by a UNIQUE constraint. Unfortunately I don't understand the difference between this test and t/86sqlt. * I have to "use SQL::Translator" * tables are not populated in SQL::Translator->schema * SQL::Translator->schema->get_table returns undef --- t/86sqlt-no-indices-on-unique-cols.t | 95 ++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 t/86sqlt-no-indices-on-unique-cols.t diff --git a/t/86sqlt-no-indices-on-unique-cols.t b/t/86sqlt-no-indices-on-unique-cols.t new file mode 100644 index 000000000..58d275e1a --- /dev/null +++ b/t/86sqlt-no-indices-on-unique-cols.t @@ -0,0 +1,95 @@ +use strict; +use warnings; +use Data::Dumper; +use SQL::Translator; # Why isn't that neccessary in t/86sqlt.t? + +use Test::More; +use lib qw(t/lib); +use DBICTest; + +BEGIN { + require DBIx::Class; + plan skip_all => 'Test needs ' + . DBIx::Class::Optional::Dependencies->req_missing_for('deploy') + unless DBIx::Class::Optional::Dependencies->req_ok_for('deploy'); +} ## end BEGIN + +note( + q(Checking there are no additional indices on first columns of unique constraints) +); + +note q(Change schema initialization to deploy automatically); +my $schema = DBICTest->init_schema(no_deploy => 1, no_populate => 1); +note q(Remove the custom deployment callback); +for my $t ($schema->sources) { + $schema->source($t)->sqlt_deploy_callback( + sub { + my ($self, $table) = @_; + note qq(Table resource $table was just deployed); + $self->default_sqlt_deploy_hook($table); + } + ); +} ## end for my $t ($schema->sources) +$schema->deploy; + +my $translator = SQL::Translator->new( + parser_args => { dbic_schema => $schema }, + parser => q(SQL::Translator::Parser::DBIx::Class), + producer_args => {}, +) or die SQL::Translator->error; +my $tschema = $translator->schema; + +TABLE: for ($schema->sources()) { + my $source = $schema->source($_); + my $table_name = $source->name; + note( + Data::Dumper->Dump( + [$_, $table_name], + [qw( schema_source source_name_before )] + ) + ); + my $tablename_type = ref $table_name; + if ($tablename_type) { + if ($tablename_type eq 'SCALAR') { + $tablename_type = $$table_name; + } + else { + note qq($_ type is skipped for unexpected type: $tablename_type); + next TABLE; + } + } ## end if ($tablename_type) + note( + Data::Dumper->Dump( + [$_, $table_name], + [qw( schema_source source_name_after )] + ) + ); + + my %ucs = $source->unique_constraints; + my @uc_first_cols = map { $ucs{$_}->[0] } keys %ucs; + + note qq(Searching indices of table $table_name); + my $_t = $tschema->get_table($table_name); # why are tables not populated?? + unless ($_t) { + note qq(Table not found: $table_name); + next; + } + + my @index_first_cols = $_t->get_indices; + + # table "cd" is my first demonstration + note( + explain( + { + $table_name => { + unique_constraints => [@uc_first_cols], + relations => [@index_first_cols], + } + } + ) + ) if $_ eq q(CD); +} ## end TABLE: for ($schema->sources) + +fail(q(!!! REMOVE ME WHEN FINISHED !!!)); + +done_testing(); From 2de8fe4a1e460516d306915cba4dfde49d85087c Mon Sep 17 00:00:00 2001 From: Marc0 Date: Sat, 1 Jul 2017 15:09:16 +0200 Subject: [PATCH 3/3] test: Add build dependency Test were failing earlier, thus adding SQL::Translator build dep. --- Makefile.PL | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Makefile.PL b/Makefile.PL index 044b0694b..17b94090c 100644 --- a/Makefile.PL +++ b/Makefile.PL @@ -94,6 +94,9 @@ my $test_requires = { # this is already a dep of n::c, but just in case - used by t/55namespaces_cleaned.t # remove and do a manual glob-collection if n::c is no longer a dep 'Package::Stash' => '0.28', + + # Why isn't that neccessary in t/86sqlt.t? + 'SQL::Translator' => 0, }; # if the user has this env var set and no SQLT installed, tests will fail