Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Postgresql indexes #163

Open
wants to merge 38 commits into
base: develop
Choose a base branch
from

Conversation

alextech
Copy link
Contributor

@alextech alextech commented Sep 6, 2016

Add support for Index creation with PostgreSQL platform.
$table = CreateTable();
$table->addConstraint(new Index())
crashed with invalid syntax error. Same for AlterTable.

Can be considered as first step towards cleaning up #67
Continuing to work on finding the rest of the incompatibilities, but not sure if submit one large PR with full PgSQL support or do smaller more review manageable parts at a time. But then cannot make PRs depend on one another so, unless told otherwise will continue adding more commits here.

…onstraint construct. It also makes decorator creation at Platform level impossible.

To avoid missing parameter and causing invalid syntaxes, will propose adding exceptions in all DB components instead of getting cryptic SQL errors in logs.
…nstruct CREATE INDEX index_name ON table_name (columns...)

Intended to be used within CreateTableDecorator, but can also add it to platform if needed, as long as table name is supplied somehow.
Because AlterTable interface signature currently only takes in string, not AbstractConstraint, not able to inspect the types to determine best query to run. Modify specification to have IF EXISTS and perform operation both in ALTER TABLE and DROP INDEX  DDL to account for possibility of either.

 Get a notice in database logs, but better than overhead and extra dependency on performing a meta query.
@alextech alextech mentioned this pull request Sep 6, 2016
@ezimuel
Copy link
Contributor

ezimuel commented Mar 8, 2017

@alextech thanks for this PR but it will require some time to review. In the meantime, because this is a new feature, can you send againt develop branch? I'm putting in zend-db 3.0.0 milestone.

@ezimuel ezimuel self-assigned this Mar 8, 2017
@ezimuel ezimuel added this to the 3.0.0 milestone Mar 8, 2017
@alextech alextech changed the base branch from master to develop March 8, 2017 16:30
@alextech
Copy link
Contributor Author

alextech commented Mar 8, 2017

Changed.

@rarog
Copy link

rarog commented Sep 26, 2017

I checked the output of by generating a CreateTable for PostgreSQL.

The general syntax is nice and correct. Where it fails, is the correct escaping, if the escape character is inside column name.

The table name is escaped correctly, but the indices inside and outside of CREATE TABLE are going rogue.

$table = new CreateTable('t\'e"s`t');
$table->addColumn(new BigInteger('t\'e"s`tCol'))
    ->addColumn(new BigInteger('t\'e"s`tCol2'))
    ->addConstraint(new Constraint\PrimaryKey('t\'e"s`tCol'));
    ->addConstraint(new Index('t\'e"s`tCol2', 't\'e"s`tIndex'));

Expected result:

CREATE TABLE "t'e""s`t" ( 
    "t'e""s`tCol" BIGINT NOT NULL,
    "t'e""s`tCol2" BIGINT NOT NULL,
    PRIMARY KEY ("t'e""s`tCol") 
); 
CREATE INDEX "t'e""s`tIndex" ON "t'e""s`tCol2");

Actual result:

CREATE TABLE "t'e""s`t" ( 
    "t""'""e""""""s""`""tCol" BIGINT NOT NULL,
    "t""'""e""""""s""`""tCol2" BIGINT NOT NULL,
    PRIMARY KEY ("t""'""e""""""s""`""tCol") 
); 
CREATE INDEX "t""'""e""""""s""`""tIndex" ON "t""'""e""""""s""`""t"("t""'""e""""""s""`""tCol2");

Instead of just doubling the escape character, there seems to be done some wrong and multiple escaping.

This behaviour is heavily bugged for the code base, too. For this I a separate bug report #267 and perhaps we could work on a fix together for all components.

@alextech
Copy link
Contributor Author

Thanks of great magnitude giving this a test after all this time. One thing to note, to make non-MySQL tests more realistic, try to always test with schema as part of the name. There are reports of escaper not working when using semantics outlined in last comment of #206 and fixed at #232

Then, there is also #233 to consider, which fixes inconsistency in applying identifier vs identifier chain. I do not think it relates to this issue, I doubt applying it will magically fix it, but sounds familiar. Will look into it.

@rarog
Copy link

rarog commented Sep 26, 2017

Good point, but so far the Ddl doesn't seem to offer a possibility to use schema as contrary to Select syntax, where TableIdentifier receives schema as 2nd parameter.

The other problem with the broken column and index names doesn't result in any of your parts but from function processExpression in Zend\Db\Sql\AbstractSql

                } elseif ($type == ExpressionInterface::TYPE_IDENTIFIER) {
                    $values[$vIndex] = $platform->quoteIdentifierInFragment($value);

I'm about to comment on this in #267.

@alextech
Copy link
Contributor Author

alextech commented Dec 8, 2017

Thanks @rarog I will see if table identifer can be used in DDL too.

…onstraint construct. It also makes decorator creation at Platform level impossible.

To avoid missing parameter and causing invalid syntaxes, will propose adding exceptions in all DB components instead of getting cryptic SQL errors in logs.
…nstruct CREATE INDEX index_name ON table_name (columns...)

Intended to be used within CreateTableDecorator, but can also add it to platform if needed, as long as table name is supplied somehow.
Because AlterTable interface signature currently only takes in string, not AbstractConstraint, not able to inspect the types to determine best query to run. Modify specification to have IF EXISTS and perform operation both in ALTER TABLE and DROP INDEX  DDL to account for possibility of either.

 Get a notice in database logs, but better than overhead and extra dependency on performing a meta query.
…l_indexes

# Conflicts:
#	test/Sql/Platform/PlatformTest.php
@michalbundyra
Copy link
Member

This repository has been moved to laminas/laminas-db. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-db to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-db.
  • In your clone of laminas/laminas-db, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

@michalbundyra
Copy link
Member

This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at laminas/laminas-db#97.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants