From dc7334087cdb375005cc0f188d551d3d446c61fe Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 17 Sep 2012 11:13:41 +1200 Subject: [PATCH 1/3] FIXED: Improved parsing of index strings to support more index formats (array, string, variable index types in either form, etc). UPDATED: Syntax to conform (better) to SS coding convention UPDATED: Refactor, cleanup, and simplification of alterTable to reduce duplication of effort. Use of index parsing mechanism to pre-prepare indexe specifications for generation. UPDATED: Better naming of variables (For instance, $indexName instead of $k) BUG: Index generation is still not working properly. To investigate. --- code/PostgreSQLDatabase.php | 368 +++++++++++++++++++----------------- 1 file changed, 196 insertions(+), 172 deletions(-) diff --git a/code/PostgreSQLDatabase.php b/code/PostgreSQLDatabase.php index 3d60e45..2b3ad24 100755 --- a/code/PostgreSQLDatabase.php +++ b/code/PostgreSQLDatabase.php @@ -483,17 +483,15 @@ class PostgreSQLDatabase extends SS_Database { */ public function alterTable($tableName, $newFields = null, $newIndexes = null, $alteredFields = null, $alteredIndexes = null, $alteredOptions = null, $advancedOptions = null) { - $fieldSchemas = $indexSchemas = ""; $alterList = array(); - if($newFields) foreach($newFields as $k => $v) $alterList[] .= "ADD \"$k\" $v"; + if($newFields) foreach($newFields as $fieldName => $fieldSpec) { + $alterList[] = "ADD \"$fieldName\" $fieldSpec"; + } - if($alteredFields) { - foreach($alteredFields as $k => $v) { - - $val=$this->alterTableAlterColumn($tableName, $k, $v); - if($val!='') - $alterList[] .= $val; - } + if($alteredFields) foreach($alteredFields as $indexName => $indexSpec) { + $val=$this->alterTableAlterColumn($tableName, $indexName, $indexSpec); + if($val!='') + $alterList[] = $val; } //Do we need to do anything with the tablespaces? @@ -510,15 +508,15 @@ class PostgreSQLDatabase extends SS_Database { $fulltexts=false; $drop_triggers=false; $triggers=false; - if($alteredIndexes) foreach($alteredIndexes as $key=>$v) { - //We are only going to delete indexes which exist - $indexes=$this->indexList($tableName); + if($alteredIndexes) foreach($alteredIndexes as $indexName=>$indexSpec) { + + $indexSpec = $this->parseIndexSpec($indexName, $indexSpec); - if($v['type']=='fulltext'){ + if($indexSpec['type']=='fulltext') { //For full text indexes, we need to drop the trigger, drop the index, AND drop the column //Go and get the tsearch details: - $ts_details=$this->fulltext($v, $tableName, $key); + $ts_details = $this->fulltext($indexSpec, $tableName, $indexName); //Drop this column if it already exists: @@ -527,60 +525,41 @@ class PostgreSQLDatabase extends SS_Database { $fulltexts.="ALTER TABLE \"{$tableName}\" DROP COLUMN \"{$ts_details['ts_name']}\";"; } - $drop_triggers.= 'DROP TRIGGER IF EXISTS ts_' . strtolower($tableName) . '_' . strtolower($key) . ' ON "' . $tableName . '";'; - $alterIndexList[] = 'DROP INDEX IF EXISTS ix_' . strtolower($tableName) . '_' . strtolower($v['value']) . ';'; - - //We'll execute these later: + // We'll execute these later: + $drop_triggers.= 'DROP TRIGGER IF EXISTS ts_' . strtolower($tableName) . '_' . strtolower($indexName) . ' ON "' . $tableName . '";'; $fulltexts.="ALTER TABLE \"{$tableName}\" ADD COLUMN {$ts_details['fulltexts']};"; - $triggers.=$ts_details['triggers']; - } else { - if(isset($indexes[$v['value']])){ - if(is_array($v)) - $alterIndexList[] = 'DROP INDEX IF EXISTS ix_' . strtolower($tableName) . '_' . strtolower($v['value']) . ';'; - else - $alterIndexList[] = 'DROP INDEX IF EXISTS ix_' . strtolower($tableName) . '_' . strtolower(trim($v, '()')) . ';'; - - $k=$v['value']; - $createIndex=$this->getIndexSqlDefinition($tableName, $k, $v); - if($createIndex!==false) - $alterIndexList[] .= $createIndex; - } + $triggers .= $ts_details['triggers']; } + + // Create index action (including fulltext) + $alterIndexList[] = 'DROP INDEX IF EXISTS ix_' . strtolower($tableName) . '_' . strtolower($indexName) . ';'; + $createIndex = $this->getIndexSqlDefinition($tableName, $indexName, $indexSpec); + if($createIndex!==false) $alterIndexList[] = $createIndex; } - //If we have a fulltext search request, then we need to create a special column - //for GiST searches - //Pick up the new indexes here: - if($newIndexes){ - foreach($newIndexes as $name=>$this_index){ - if(is_array($this_index) && $this_index['type']=='fulltext'){ - $ts_details=$this->fulltext($this_index, $tableName, $name); - if(!isset($fieldList[$ts_details['ts_name']])){ - $fulltexts.="ALTER TABLE \"{$tableName}\" ADD COLUMN {$ts_details['fulltexts']};"; - $triggers.=$ts_details['triggers']; - } + //Add the new indexes: + if($newIndexes) foreach($newIndexes as $indexName => $indexSpec){ + + $indexSpec = $this->parseIndexSpec($indexName, $indexSpec); + + //If we have a fulltext search request, then we need to create a special column + //for GiST searches + //Pick up the new indexes here: + if($indexSpec['type']=='fulltext') { + $ts_details=$this->fulltext($indexSpec, $tableName, $indexName); + if(!isset($fieldList[$ts_details['ts_name']])){ + $fulltexts.="ALTER TABLE \"{$tableName}\" ADD COLUMN {$ts_details['fulltexts']};"; + $triggers.=$ts_details['triggers']; } } - } - - //Add the new indexes: - if($newIndexes) foreach($newIndexes as $k=>$v){ + //Check that this index doesn't already exist: $indexes=$this->indexList($tableName); - if(!is_array($v)){ - $name=trim($v, '()'); - } else { - $name=(isset($v['name'])) ? $v['name'] : $k; - } - if(isset($indexes[$name])){ - if(is_array($v)){ - $alterIndexList[] = 'DROP INDEX IF EXISTS ix_' . strtolower($tableName) . '_' . strtolower($v['value']) . ';'; - } else { - $alterIndexList[] = 'DROP INDEX IF EXISTS ' . $indexes[$name]['indexname'] . ';'; - } + if(isset($indexes[$indexName])){ + $alterIndexList[] = 'DROP INDEX IF EXISTS ix_' . strtolower($tableName) . '_' . strtolower($indexName) . ';'; } - $createIndex=$this->getIndexSqlDefinition($tableName, $k, $v); + $createIndex=$this->getIndexSqlDefinition($tableName, $indexName, $indexSpec); if($createIndex!==false) $alterIndexList[] = $createIndex; } @@ -605,10 +584,8 @@ class PostgreSQLDatabase extends SS_Database { } //Create any fulltext columns and triggers here: - if($fulltexts) - $this->query($fulltexts); - if($drop_triggers) - $this->query($drop_triggers); + if($fulltexts) $this->query($fulltexts); + if($drop_triggers) $this->query($drop_triggers); if($triggers) { $this->query($triggers); @@ -624,8 +601,7 @@ class PostgreSQLDatabase extends SS_Database { } } - foreach($alterIndexList as $alteration) - $this->query($alteration); + foreach($alterIndexList as $alteration) $this->query($alteration); //If we have a partitioning requirement, we do that here: if($advancedOptions && isset($advancedOptions['partitions'])){ @@ -668,17 +644,17 @@ class PostgreSQLDatabase extends SS_Database { $pattern = '/^([\w()]+)\s?((?:not\s)?null)?\s?(default\s[\w\']+)?\s?(check\s[\w()\'",\s]+)?$/i'; preg_match($pattern, $colSpec, $matches); - if(sizeof($matches)==0) - return ''; + if(sizeof($matches)==0) return ''; - if($matches[1]=='serial8') - return ''; + if($matches[1]=='serial8') return ''; if(isset($matches[1])) { $alterCol = "ALTER COLUMN \"$colName\" TYPE $matches[1]\n"; // SET null / not null - if(!empty($matches[2])) $alterCol .= ",\nALTER COLUMN \"$colName\" SET $matches[2]"; + if(!empty($matches[2])) { + $alterCol .= ",\nALTER COLUMN \"$colName\" SET $matches[2]"; + } // SET default (we drop it first, for reasons of precaution) if(!empty($matches[3])) { @@ -699,10 +675,12 @@ class PostgreSQLDatabase extends SS_Database { //We have to run this as a query, not as part of the alteration queries due to the way they are constructed. $updateConstraint=''; $updateConstraint.="UPDATE \"{$tableName}\" SET \"$colName\"='$default' WHERE \"$colName\" NOT IN ($constraint_values);"; - if($this->hasTable("{$tableName}_Live")) + if($this->hasTable("{$tableName}_Live")) { $updateConstraint.="UPDATE \"{$tableName}_Live\" SET \"$colName\"='$default' WHERE \"$colName\" NOT IN ($constraint_values);"; - if($this->hasTable("{$tableName}_versions")) + } + if($this->hasTable("{$tableName}_versions")) { $updateConstraint.="UPDATE \"{$tableName}_versions\" SET \"$colName\"='$default' WHERE \"$colName\" NOT IN ($constraint_values);"; + } DB::query($updateConstraint); } @@ -793,7 +771,6 @@ class PostgreSQLDatabase extends SS_Database { //Check to see if there's a constraint attached to this column: //$constraint=$this->query("SELECT conname,pg_catalog.pg_get_constraintdef(r.oid, true) FROM pg_catalog.pg_constraint r WHERE r.contype = 'c' AND conname='" . $table . '_' . $field['column_name'] . "_check' ORDER BY 1;")->first(); $constraint=$this->constraintExists($table . '_' . $field['column_name'] . '_check'); - $enum=''; if($constraint){ //Now we need to break this constraint text into bits so we can see what we have: //Examples: @@ -882,10 +859,8 @@ class PostgreSQLDatabase extends SS_Database { * @return boolean */ function clearCachedFieldlist($tableName=false){ - if($tableName!=false){ - unset(self::$cached_fieldlists[$tableName]); - } else - self::$cached_fieldlists=array(); + if($tableName) unset(self::$cached_fieldlists[$tableName]); + else self::$cached_fieldlists=array(); return true; } @@ -898,8 +873,7 @@ class PostgreSQLDatabase extends SS_Database { */ public function createIndex($tableName, $indexName, $indexSpec) { $createIndex=$this->getIndexSqlDefinition($tableName, $indexName, $indexSpec); - if($createIndex!==false) - $this->query(); + if($createIndex!==false) $this->query(); } /* @@ -907,6 +881,7 @@ class PostgreSQLDatabase extends SS_Database { * and turns it into a proper string. * Some indexes may be arrays, such as fulltext and unique indexes, and this allows database-specific * arrays to be created. + * @see parseIndexSpec() for approximate inverse */ public function convertIndexSpec($indexSpec, $asDbValue=false, $table=''){ @@ -915,9 +890,7 @@ class PostgreSQLDatabase extends SS_Database { //Here we create a db-specific version of whatever index we need to create. switch($indexSpec['type']){ case 'fulltext': - //We need to include the fields so if we change the columns it's indexing, but not the name, - //then the change will be picked up. - $indexSpec='(' . $indexSpec['name'] . ',' . $indexSpec['value'] . ')'; + $indexSpec='fulltext (' . $indexSpec['value'] . ')'; break; case 'unique': $indexSpec='unique (' . $indexSpec['value'] . ')'; @@ -937,6 +910,81 @@ class PostgreSQLDatabase extends SS_Database { } return $indexSpec; } + + /** + * Splits a spec string safely, considering quoted columns, whitespace, + * and cleaning brackets + * @param string $spec The input index specification + * @return array List of columns in the spec + */ + function explodeColumnString($spec) { + // Remove any leading/trailing brackets and outlying modifiers + // E.g. 'unique (Title, "QuotedColumn");' => 'Title, "QuotedColumn"' + $containedSpec = preg_replace('/(.*\(\s*)|(\s*\).*)/', '', $spec); + + // Split potentially quoted modifiers + // E.g. 'Title, "QuotedColumn"' => array('Title', 'QuotedColumn') + return preg_split('/"?\s*,\s*"?/', trim($containedSpec, '(") ')); + } + + /** + * Builds a properly quoted column list from an array + * @param array $columns List of columns to implode + * @return string A properly quoted list of column names + */ + function implodeColumnList($columns) { + if(empty($columns)) return ''; + return '"' . implode('","', $columns) . '"'; + } + + /** + * Given an index specification in the form of a string ensure that each + * column name is property quoted, stripping brackets + * @param string $spec The input specification + * @return string The properly quoted column list + */ + function quoteColumnSpecString($spec) { + $bits = $this->explodeColumnString($spec); + return $this->implodeColumnList($bits); + } + + /** + * Given an index spec determines the index type + * @param type $spec + * @return string + */ + function determineIndexType($spec) { + // check array spec + if(is_array($spec) && isset($spec['type'])) { + return $spec['type']; + } elseif (!is_array($spec) && preg_match('/(?\w+)\s*\(/', $spec, $matchType)) { + return strtolower($matchType['type']); + } else { + return 'index'; + } + } + + /** + * Converts an array or string index spec into a universally useful array + * @see convertIndexSpec() for approximate inverse + * @param string|array $spec + * @return array The resulting spec array with the required fields name, type, and value + */ + function parseIndexSpec($name, $spec){ + + // Do minimal cleanup on any already parsed spec + if(is_array($spec)) { + $spec['value'] = $this->quoteColumnSpecString($spec['value']); + return $spec; + } + + // Nicely formatted spec! + return array( + 'name' => $name, + 'value' => $this->quoteColumnSpecString($spec), + 'type' => $this->determineIndexType($spec) + ); + } protected function getIndexSqlDefinition($tableName, $indexName, $indexSpec, $asDbValue=false) { @@ -946,86 +994,60 @@ class PostgreSQLDatabase extends SS_Database { //NOTE: it is possible for *_renamed tables to have indexes whose names are not updates //Therefore, we now check for the existance of indexes before we create them. //This is techically a bug, since new tables will not be indexed. - - if(!$asDbValue){ - - $tableCol= 'ix_' . str_replace("\\", "_", $tableName) . '_' . $indexName; - if(strlen($tableCol)>64){ - $tableCol=substr($indexName, 0, 59) . rand(1000, 9999); - } - - //It is possible to specify indexes through strings: - if(!is_array($indexSpec)){ - $indexSpec=trim($indexSpec, '()'); - $bits=explode(',', $indexSpec); - $indexes="\"" . implode("\",\"", $bits) . "\""; - // if some of the indexes have already been quoted then we need to remove the suplus double quotes - $indexes = str_replace('""', '"', $indexes); - - //One last check: - $existing=DB::query("SELECT tablename FROM pg_indexes WHERE indexname='" . strtolower($tableCol) . "';")->first(); - if(!$existing) - return "create index $tableCol ON \"" . $tableName . "\" (" . $indexes . ");"; - else - return false; - } else { - - //Arrays offer much more flexibility and many more options: - - //Misc options first: - $fillfactor=$where=''; - if(isset($indexSpec['fillfactor'])) - $fillfactor='WITH (FILLFACTOR = ' . $indexSpec['fillfactor'] . ')'; - if(isset($indexSpec['where'])) - $where='WHERE ' . $indexSpec['where']; - - //Fix up the value entry to be quoted: - $value_bits=explode(',', $indexSpec['value']); - $new_values=Array(); - foreach($value_bits as $value){ - $new_values[]="\"" . trim($value, ' "') . "\""; - } - $indexSpec['value']=implode(',', $new_values); - - //One last check: - $existing=DB::query("SELECT tablename FROM pg_indexes WHERE indexname='" . strtolower($tableCol) . "';"); - if(!$existing->first()){ - //create a type-specific index - //NOTE: hash should be removed. This is only here to demonstrate how other indexes can be made - switch($indexSpec['type']){ - case 'fulltext': - $spec="create index $tableCol ON \"" . $tableName . "\" USING " . $this->default_fts_cluster_method . "(\"ts_" . $indexName . "\") $fillfactor $where"; - break; - - case 'unique': - $spec="create unique index $tableCol ON \"" . $tableName . "\" (" . $indexSpec['value'] . ") $fillfactor $where"; - break; - - case 'btree': - $spec="create index $tableCol ON \"" . $tableName . "\" USING btree (" . $indexSpec['value'] . ") $fillfactor $where"; - break; - - case 'hash': - //NOTE: this is not a recommended index type - $spec="create index $tableCol ON \"" . $tableName . "\" USING hash (" . $indexSpec['value'] . ") $fillfactor $where"; - break; - - case 'index': - //'index' is the same as default, just a normal index with the default type decided by the database. - default: - $spec="create index $tableCol ON \"" . $tableName . "\" (" . $indexSpec['value'] . ") $fillfactor $where"; - } - - return trim($spec) . ';'; - } else { - - return false; - } - } - } else { + + // If requesting the definition rather than the DDL + if($asDbValue) { $indexName=trim($indexName, '()'); return $indexName; } + + // Determine index name and check for existence + $tableCol= 'ix_' . str_replace("\\", "_", $tableName) . '_' . $indexName; + if(strlen($tableCol)>64){ + $tableCol=substr($indexName, 0, 59) . rand(1000, 9999); + } + $existing=DB::query("SELECT tablename FROM pg_indexes WHERE indexname='" . strtolower($tableCol) . "';")->first(); + if($existing) return false; + + // Consolidate/Cleanup spec into array format + $indexSpec = $this->parseIndexSpec($indexName, $indexSpec); + + //Misc options first: + $fillfactor=$where=''; + if(isset($indexSpec['fillfactor'])) { + $fillfactor='WITH (FILLFACTOR = ' . $indexSpec['fillfactor'] . ')'; + } + if(isset($indexSpec['where'])) { + $where='WHERE ' . $indexSpec['where']; + } + + //create a type-specific index + //NOTE: hash should be removed. This is only here to demonstrate how other indexes can be made + switch($indexSpec['type']){ + case 'fulltext': + // @see fulltext() for the definition of the trigger that ts_$IndexName uses for fulltext searching + $spec="create index $tableCol ON \"" . $tableName . "\" USING " . $this->default_fts_cluster_method . "(\"ts_" . $indexName . "\") $fillfactor $where"; + break; + + case 'unique': + $spec="create unique index $tableCol ON \"" . $tableName . "\" (" . $indexSpec['value'] . ") $fillfactor $where"; + break; + + case 'btree': + $spec="create index $tableCol ON \"" . $tableName . "\" USING btree (" . $indexSpec['value'] . ") $fillfactor $where"; + break; + + case 'hash': + //NOTE: this is not a recommended index type + $spec="create index $tableCol ON \"" . $tableName . "\" USING hash (" . $indexSpec['value'] . ") $fillfactor $where"; + break; + + case 'index': + //'index' is the same as default, just a normal index with the default type decided by the database. + default: + $spec="create index $tableCol ON \"" . $tableName . "\" (" . $indexSpec['value'] . ") $fillfactor $where"; + } + return trim($spec) . ';'; } function getDbSqlDefinition($tableName, $indexName, $indexSpec){ @@ -1067,29 +1089,34 @@ class PostgreSQLDatabase extends SS_Database { $indexList=Array(); foreach($indexes as $index) { + //We don't actually need the entire created command, just a few bits: $prefix=''; //Check for uniques: - if(substr($index['indexdef'], 0, 13)=='CREATE UNIQUE') + if(substr($index['indexdef'], 0, 13)=='CREATE UNIQUE') { $prefix='unique '; + } //check for hashes, btrees etc: - if(strpos(strtolower($index['indexdef']), 'using hash ')!==false) + if(strpos(strtolower($index['indexdef']), 'using hash ')!==false) { $prefix='using hash '; + } //TODO: Fix me: btree is the default index type: //if(strpos(strtolower($index['indexdef']), 'using btree ')!==false) // $prefix='using btree '; - if(strpos(strtolower($index['indexdef']), 'using rtree ')!==false) + if(strpos(strtolower($index['indexdef']), 'using rtree ')!==false) { $prefix='using rtree '; + } $value=explode(' ', substr($index['indexdef'], strpos($index['indexdef'], ' USING ')+7)); if(sizeof($value)>2){ - for($i=2; $idbConn, $this->schema); $result=$this->query("SELECT tablename FROM pg_catalog.pg_tables WHERE schemaname = '{$schema_SQL}' AND tablename='" . $this->addslashes($tableName) . "';")->first(); - if($result) - return true; - else - return false; - + return !empty($result); } /** @@ -1449,11 +1477,7 @@ class PostgreSQLDatabase extends SS_Database { */ function fulltext($this_index, $tableName, $name){ //For full text search, we need to create a column for the index - $columns=explode(',', $this_index['value']); - for($i=0; $iquoteColumnSpecString($this_index['value']); $fulltexts="\"ts_$name\" tsvector"; $triggerName="ts_{$tableName}_{$name}"; From 3291147c8e9eae40bbbaa7de9b5447e7769aef2d Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 17 Sep 2012 16:15:00 +1200 Subject: [PATCH 2/3] FIXED: Issue with correct extraction of index names from the database. The root cause of this issue was the way that columns from indxes were retrieved. It was assumed that the column names formed the index name, which isn't necessarily true (E.g. when the index is named "SearchFields"). The behaviour of the module was updated to create case-sensitive index and trigger names, which could then be used to later tell Silverstripe which indexes existed in the database. These could be compared to the SiteTree::$indexes property in a case-sensitive fashion to determine which indexes needed to be created / updated. This update fixes a lot of the unnecessary/broken DDL operations that occurred. --- code/PostgreSQLDatabase.php | 246 +++++++++++++++++++++++------------- 1 file changed, 156 insertions(+), 90 deletions(-) diff --git a/code/PostgreSQLDatabase.php b/code/PostgreSQLDatabase.php index 2b3ad24..fd59b9c 100755 --- a/code/PostgreSQLDatabase.php +++ b/code/PostgreSQLDatabase.php @@ -38,10 +38,10 @@ class PostgreSQLDatabase extends SS_Database { private $database_original; /** - * The database schema name. - * @var string - */ - private $schema; + * The database schema name. + * @var string + */ + private $schema; /* * This holds the parameters that the original connection was created with, @@ -353,30 +353,30 @@ class PostgreSQLDatabase extends SS_Database { return $this->query("SELECT nspname FROM pg_catalog.pg_namespace WHERE nspname = '{$SQL_name}';")->first() ? true : false; } - /** - * Creates a schema in the current database - * @param string $name - */ - public function createSchema($name) { - $SQL_name = pg_escape_string($this->dbConn, $name); - $this->query("CREATE SCHEMA \"{$SQL_name}\";"); - } + /** + * Creates a schema in the current database + * @param string $name + */ + public function createSchema($name) { + $SQL_name = pg_escape_string($this->dbConn, $name); + $this->query("CREATE SCHEMA \"{$SQL_name}\";"); + } - /** - * Drops a schema from the database. Use carefully! - * @param string $name - */ - public function dropSchema($name) { - $SQL_name = pg_escape_string($this->dbConn, $name); - $this->query("DROP SCHEMA \"{$SQL_name}\" CASCADE;"); - } + /** + * Drops a schema from the database. Use carefully! + * @param string $name + */ + public function dropSchema($name) { + $SQL_name = pg_escape_string($this->dbConn, $name); + $this->query("DROP SCHEMA \"{$SQL_name}\" CASCADE;"); + } - /** - * Returns the name of the current schema in use - */ - public function currentSchema() { - return $this->query('SELECT current_schema()')->value(); - } + /** + * Returns the name of the current schema in use + */ + public function currentSchema() { + return $this->query('SELECT current_schema()')->value(); + } /** * Utility method to manually set the schema to an alternative @@ -409,7 +409,7 @@ class PostgreSQLDatabase extends SS_Database { $args[$key] = '"' . pg_escape_string($this->dbConn, $schema) . '"'; $args_SQL =implode(",", $args); $this->query("SET search_path TO {$args_SQL}"); - } + } public function createTable($tableName, $fields = null, $indexes = null, $options = null, $extensions = null) { @@ -473,6 +473,38 @@ class PostgreSQLDatabase extends SS_Database { return $tableName; } + /** + * Builds the internal Postgres index name given the silverstripe table and index name + * @param string $tableName + * @param string $indexName + * @param string $prefix The optional prefix for the index. Defaults to "ix" for indexes. + * @return string The postgres name of the index + */ + function buildPostgresIndexName($tableName, $indexName, $prefix = 'ix') { + + // Replace namespace character + $tableNameSafe = str_replace("\\", "_", $tableName); + + // Assume all indexes also contain the table name + $indexNamePG = "{$prefix}_{$tableNameSafe}_{$indexName}"; + + // Limit to 63 characters + if (strlen($indexNamePG) > 63) + return substr($indexNamePG, 0, 63); + return $indexNamePG; + } + + /** + * Builds the internal Postgres trigger name given the silverstripe table and trigger name + * @param string $tableName + * @param string $triggerName + * @return string The postgres name of the trigger + */ + function buildPostgresTriggerName($tableName, $triggerName) { + // Kind of cheating, but behaves the same way as indexes + return $this->buildPostgresIndexName($tableName, $triggerName, 'ts'); + } + /** * Alter a table's schema. * @param $table The name of the table to alter @@ -488,10 +520,9 @@ class PostgreSQLDatabase extends SS_Database { $alterList[] = "ADD \"$fieldName\" $fieldSpec"; } - if($alteredFields) foreach($alteredFields as $indexName => $indexSpec) { - $val=$this->alterTableAlterColumn($tableName, $indexName, $indexSpec); - if($val!='') - $alterList[] = $val; + if ($alteredFields) foreach ($alteredFields as $indexName => $indexSpec) { + $val = $this->alterTableAlterColumn($tableName, $indexName, $indexSpec); + if (!empty($val)) $alterList[] = $val; } //Do we need to do anything with the tablespaces? @@ -511,6 +542,7 @@ class PostgreSQLDatabase extends SS_Database { if($alteredIndexes) foreach($alteredIndexes as $indexName=>$indexSpec) { $indexSpec = $this->parseIndexSpec($indexName, $indexSpec); + $indexNamePG = $this->buildPostgresIndexName($tableName, $indexName); if($indexSpec['type']=='fulltext') { //For full text indexes, we need to drop the trigger, drop the index, AND drop the column @@ -526,22 +558,23 @@ class PostgreSQLDatabase extends SS_Database { } // We'll execute these later: - $drop_triggers.= 'DROP TRIGGER IF EXISTS ts_' . strtolower($tableName) . '_' . strtolower($indexName) . ' ON "' . $tableName . '";'; - $fulltexts.="ALTER TABLE \"{$tableName}\" ADD COLUMN {$ts_details['fulltexts']};"; + $triggerNamePG = $this->buildPostgresTriggerName($tableName, $indexName); + $drop_triggers.= "DROP TRIGGER IF EXISTS \"$triggerNamePG\" ON \"$tableName\";"; + $fulltexts .= "ALTER TABLE \"{$tableName}\" ADD COLUMN {$ts_details['fulltexts']};"; $triggers .= $ts_details['triggers']; } // Create index action (including fulltext) - $alterIndexList[] = 'DROP INDEX IF EXISTS ix_' . strtolower($tableName) . '_' . strtolower($indexName) . ';'; + $alterIndexList[] = "DROP INDEX IF EXISTS \"$indexNamePG\";"; $createIndex = $this->getIndexSqlDefinition($tableName, $indexName, $indexSpec); if($createIndex!==false) $alterIndexList[] = $createIndex; - } + } //Add the new indexes: if($newIndexes) foreach($newIndexes as $indexName => $indexSpec){ $indexSpec = $this->parseIndexSpec($indexName, $indexSpec); - + $indexNamePG = $this->buildPostgresIndexName($tableName, $indexName); //If we have a fulltext search request, then we need to create a special column //for GiST searches //Pick up the new indexes here: @@ -553,10 +586,10 @@ class PostgreSQLDatabase extends SS_Database { } } - //Check that this index doesn't already exist: + //Check that this index doesn't already exist: $indexes=$this->indexList($tableName); if(isset($indexes[$indexName])){ - $alterIndexList[] = 'DROP INDEX IF EXISTS ix_' . strtolower($tableName) . '_' . strtolower($indexName) . ';'; + $alterIndexList[] = "DROP INDEX IF EXISTS \"$indexNamePG\";"; } $createIndex=$this->getIndexSqlDefinition($tableName, $indexName, $indexSpec); @@ -609,8 +642,9 @@ class PostgreSQLDatabase extends SS_Database { } //Lastly, clustering goes here: - if($advancedOptions && isset($advancedOptions['cluster'])){ - DB::query("CLUSTER \"$tableName\" USING ix_{$tableName}_{$advancedOptions['cluster']};"); + if ($advancedOptions && isset($advancedOptions['cluster'])) { + $clusterIndex = $this->buildPostgresIndexName($tableName, $advancedOptions['cluster']); + DB::query("CLUSTER \"$tableName\" USING \"$clusterIndex\";"); } else { //Check that clustering is not on this table, and if it is, remove it: @@ -939,9 +973,10 @@ class PostgreSQLDatabase extends SS_Database { /** * Given an index specification in the form of a string ensure that each - * column name is property quoted, stripping brackets - * @param string $spec The input specification - * @return string The properly quoted column list + * column name is property quoted, stripping brackets and modifiers. + * This index may also be in the form of a "CREATE INDEX..." sql fragment + * @param string $spec The input specification or query. E.g. 'unique (Column1, Column2)' + * @return string The properly quoted column list. E.g. '"Column1", "Column2"' */ function quoteColumnSpecString($spec) { $bits = $this->explodeColumnString($spec); @@ -1001,56 +1036,52 @@ class PostgreSQLDatabase extends SS_Database { return $indexName; } - // Determine index name and check for existence - $tableCol= 'ix_' . str_replace("\\", "_", $tableName) . '_' . $indexName; - if(strlen($tableCol)>64){ - $tableCol=substr($indexName, 0, 59) . rand(1000, 9999); - } - $existing=DB::query("SELECT tablename FROM pg_indexes WHERE indexname='" . strtolower($tableCol) . "';")->first(); - if($existing) return false; + // Determine index name + $tableCol = $this->buildPostgresIndexName($tableName, $indexName); // Consolidate/Cleanup spec into array format $indexSpec = $this->parseIndexSpec($indexName, $indexSpec); - + //Misc options first: - $fillfactor=$where=''; - if(isset($indexSpec['fillfactor'])) { - $fillfactor='WITH (FILLFACTOR = ' . $indexSpec['fillfactor'] . ')'; + $fillfactor = $where = ''; + if (isset($indexSpec['fillfactor'])) { + $fillfactor = 'WITH (FILLFACTOR = ' . $indexSpec['fillfactor'] . ')'; } - if(isset($indexSpec['where'])) { - $where='WHERE ' . $indexSpec['where']; + if (isset($indexSpec['where'])) { + $where = 'WHERE ' . $indexSpec['where']; } //create a type-specific index - //NOTE: hash should be removed. This is only here to demonstrate how other indexes can be made - switch($indexSpec['type']){ + // NOTE: hash should be removed. This is only here to demonstrate how other indexes can be made + // NOTE: Quote the index name to preserve case sensitivity + switch ($indexSpec['type']) { case 'fulltext': // @see fulltext() for the definition of the trigger that ts_$IndexName uses for fulltext searching - $spec="create index $tableCol ON \"" . $tableName . "\" USING " . $this->default_fts_cluster_method . "(\"ts_" . $indexName . "\") $fillfactor $where"; + $spec = "create index \"$tableCol\" ON \"$tableName\" USING " . $this->default_fts_cluster_method . "(\"ts_" . $indexName . "\") $fillfactor $where"; break; case 'unique': - $spec="create unique index $tableCol ON \"" . $tableName . "\" (" . $indexSpec['value'] . ") $fillfactor $where"; + $spec = "create unique index \"$tableCol\" ON \"$tableName\" (" . $indexSpec['value'] . ") $fillfactor $where"; break; case 'btree': - $spec="create index $tableCol ON \"" . $tableName . "\" USING btree (" . $indexSpec['value'] . ") $fillfactor $where"; + $spec = "create index \"$tableCol\" ON \"$tableName\" USING btree (" . $indexSpec['value'] . ") $fillfactor $where"; break; case 'hash': //NOTE: this is not a recommended index type - $spec="create index $tableCol ON \"" . $tableName . "\" USING hash (" . $indexSpec['value'] . ") $fillfactor $where"; + $spec = "create index \"$tableCol\" ON \"$tableName\" USING hash (" . $indexSpec['value'] . ") $fillfactor $where"; break; case 'index': - //'index' is the same as default, just a normal index with the default type decided by the database. + //'index' is the same as default, just a normal index with the default type decided by the database. default: - $spec="create index $tableCol ON \"" . $tableName . "\" (" . $indexSpec['value'] . ") $fillfactor $where"; + $spec = "create index \"$tableCol\" ON \"$tableName\" (" . $indexSpec['value'] . ") $fillfactor $where"; } return trim($spec) . ';'; } - function getDbSqlDefinition($tableName, $indexName, $indexSpec){ + function getDbSqlDefinition($tableName, $indexName, $indexSpec) { return $this->getIndexSqlDefinition($tableName, $indexName, $indexSpec, true); } @@ -1072,9 +1103,39 @@ class PostgreSQLDatabase extends SS_Database { $indexType = "index"; } - $this->query("DROP INDEX $indexName"); + $this->query("DROP INDEX \"$indexName\""); $this->query("ALTER TABLE \"$tableName\" ADD $indexType \"$indexName\" $indexFields"); } + + /** + * Given a trigger name attempt to determine the columns upon which it acts + * @param string $triggerName Postgres trigger name + * @return array List of columns + */ + protected function extractTriggerColumns($triggerName) + { + $trigger = DB::query($statement = sprintf( + "SELECT tgargs FROM pg_catalog.pg_trigger WHERE tgname='%s'", $this->addslashes($triggerName) + ))->first(); + + // Trigger columns will be extracted in an ugly hex format with null- + // terminated strings, needs some coaxing into a readable format + $tgargsHex = $trigger['tgargs']; + $tgargs = array(); + $tgarg = ''; + for ($i = 0; $i < strlen($tgargsHex); $i+=2) { + $hexChar = substr($tgargsHex, $i, 2); + if($hexChar == '00') { + $tgargs[] = $tgarg; + $tgarg = ''; + } else { + $tgarg .= chr(hexdec($hexChar)); + } + } + + // Drop first two arguments (trigger name and config name) and implode into nice list + return array_slice($tgargs, 2); + } /** * Return the list of indexes in a table. @@ -1090,6 +1151,15 @@ class PostgreSQLDatabase extends SS_Database { $indexList=Array(); foreach($indexes as $index) { + // Determine the name of the index + if (stristr($index['indexname'], '_pkey')) { + $indexName = 'ID'; + } else { + // Extract index name by splitting the ix_TableName_ from the start of the name + $indexNamePrefix = $this->buildPostgresIndexName($table, ''); + $indexName = substr($index['indexname'], strlen($indexNamePrefix)); + } + //We don't actually need the entire created command, just a few bits: $prefix=''; @@ -1111,20 +1181,20 @@ class PostgreSQLDatabase extends SS_Database { $prefix='using rtree '; } - $value=explode(' ', substr($index['indexdef'], strpos($index['indexdef'], ' USING ')+7)); + // For fulltext indexes we need to extract the columns from another source + if (stristr($index['indexdef'], 'using gin')) { + $prefix = 'fulltext '; + // Extract trigger information from postgres + $triggerName = $this->buildPostgresTriggerName($table, $indexName); + $columns = $this->extractTriggerColumns($triggerName); + $columnString = $this->implodeColumnList($columns); + } else { + $columnString = $this->quoteColumnSpecString($index['indexdef']); + } - if(sizeof($value)>2){ - for($i=2; $iquoteColumnSpecString($this_index['value']); - $fulltexts="\"ts_$name\" tsvector"; - $triggerName="ts_{$tableName}_{$name}"; - $language=$this->get_search_language(); + $fulltexts = "\"ts_$name\" tsvector"; + $triggerName = $this->buildPostgresTriggerName($tableName, $name); + $language = $this->get_search_language(); $this->dropTrigger($triggerName, $tableName); - $triggers="CREATE TRIGGER $triggerName BEFORE INSERT OR UPDATE + $triggers = "CREATE TRIGGER \"$triggerName\" BEFORE INSERT OR UPDATE ON \"$tableName\" FOR EACH ROW EXECUTE PROCEDURE tsvector_update_trigger(\"ts_$name\", 'pg_catalog.$language', $columns);"; - return Array('name'=>$name, 'ts_name'=>"ts_{$name}", 'fulltexts'=>$fulltexts, 'triggers'=>$triggers); + return Array('name' => $name, 'ts_name' => "ts_{$name}", 'fulltexts' => $fulltexts, 'triggers' => $triggers); } /** @@ -1594,13 +1664,9 @@ class PostgreSQLDatabase extends SS_Database { /* * This changes the index name depending on database requirements. */ - function modifyIndex($index, $spec){ - - if(is_array($spec) && $spec['type']=='fulltext') - return 'ts_' . str_replace(',', '_', $index); - else - return str_replace('_', ',', $index); + function modifyIndex($index, $spec) { + return $index; } /** From 37199fc08c22d0d5c7bacaa573529e896514789f Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 17 Sep 2012 16:51:20 +1200 Subject: [PATCH 3/3] FIXED: Incorrect paging on full text search results --- code/PostgreSQLDatabase.php | 1 + 1 file changed, 1 insertion(+) diff --git a/code/PostgreSQLDatabase.php b/code/PostgreSQLDatabase.php index fd59b9c..8d9b005 100755 --- a/code/PostgreSQLDatabase.php +++ b/code/PostgreSQLDatabase.php @@ -1779,6 +1779,7 @@ class PostgreSQLDatabase extends SS_Database { if(isset($objects)) $results = new ArrayList($objects); else $results = new ArrayList(); $list = new PaginatedList($results); + $list->setLimitItems(false); $list->setPageStart($start); $list->setPageLength($pageLength); $list->setTotalItems($totalCount);