added postgresql features to reset seq/check integrity #1648

Merged
merged 6 commits into from Dec 27, 2013

Conversation

Projects
None yet
3 participants
@Ragazzo
Contributor

Ragazzo commented Dec 27, 2013

Added some new features for postgresql. I also raised visibility of extremely useful method findTablesNames. Since it is different per database and user can not have one sql this method is very useful. I also enabled emulating prepare as i've described in comment.
@qiangxue please review when you will have time

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Dec 27, 2013

Contributor

tested it locally on database with several tables, works well, but to be sure can be tested by some other developers. Later maybe will provide tests (for now i am a little bit not known how to setup database for them in fw tests).

Contributor

Ragazzo commented Dec 27, 2013

tested it locally on database with several tables, works well, but to be sure can be tested by some other developers. Later maybe will provide tests (for now i am a little bit not known how to setup database for them in fw tests).

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Dec 27, 2013

Contributor

related with #1646

Contributor

Ragazzo commented Dec 27, 2013

related with #1646

@Ragazzo Ragazzo referenced this pull request Dec 27, 2013

Merged

fixed sequence reset #1649

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Dec 27, 2013

Contributor

fixed some bugs. ready for review.

Contributor

Ragazzo commented Dec 27, 2013

fixed some bugs. ready for review.

@samdark

View changes

framework/yii/db/pgsql/QueryBuilder.php
+ if ($table !== null && $table->sequenceName !== null) {
+ $sequence='"'.$table->sequenceName.'"';
+
+ if(strpos($sequence,'.')!==false)

This comment has been minimized.

@samdark

samdark Dec 27, 2013

Member

Please fix code style.

@samdark

samdark Dec 27, 2013

Member

Please fix code style.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Dec 27, 2013

Member

Looks good to me. Please fix the coding style pointed out by @samdark and also add a line to CHANGELOG. I will then merge it. Thanks!

Member

qiangxue commented Dec 27, 2013

Looks good to me. Please fix the coding style pointed out by @samdark and also add a line to CHANGELOG. I will then merge it. Thanks!

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Dec 27, 2013

Contributor

i also have small question, i've ported it from Yii1, so there was

if(strpos($sequence,'.')!==false)

and

if(strpos($tableName,'.')!==false)

to be true i dont remember when in postgresql schema can contain ., for table name in ``checkIntegrityit can be but we already specified schema in this method,is this checks needed and correct? Never saw this) Also why we$key = reset($table->primaryKey);` reset pk if it is still contain the same columns for table (this was taken from mysql query-builder of Yii2)?

Contributor

Ragazzo commented Dec 27, 2013

i also have small question, i've ported it from Yii1, so there was

if(strpos($sequence,'.')!==false)

and

if(strpos($tableName,'.')!==false)

to be true i dont remember when in postgresql schema can contain ., for table name in ``checkIntegrityit can be but we already specified schema in this method,is this checks needed and correct? Never saw this) Also why we$key = reset($table->primaryKey);` reset pk if it is still contain the same columns for table (this was taken from mysql query-builder of Yii2)?

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Dec 27, 2013

Member

I'm not sure about if(strpos($sequence,'.')!==false).

For if(strpos($tableName,'.')!==false), you are right that we don't need this check in 2.0. In 1.1, we need it because findTableNames() return table names prefixed with schema names.

$key = reset($table->primaryKey) gets the first column in the primary key. What's wrong with this?

Member

qiangxue commented Dec 27, 2013

I'm not sure about if(strpos($sequence,'.')!==false).

For if(strpos($tableName,'.')!==false), you are right that we don't need this check in 2.0. In 1.1, we need it because findTableNames() return table names prefixed with schema names.

$key = reset($table->primaryKey) gets the first column in the primary key. What's wrong with this?

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Dec 27, 2013

Contributor

I'm not sure about if(strpos($sequence,'.')!==false).

ok, then will leave it for any case.

For if(strpos($tableName,'.')!==false), you are right that we don't need this check in 2.0. In 1.1, we need it because findTableNames() return table names prefixed with schema names.

right, so maybe we need to make this:

#in checkIntegrity method
$schema = $schema ? $schema : $this->db->schema->defaultSchema;

#in foreach cycle
$tableName='"'.$schema.".".$tableName.'"';

or dont needed?

$key = reset($table->primaryKey) gets the first column in the primary key. What's wrong with this?

right, my misunderstanding.

Contributor

Ragazzo commented Dec 27, 2013

I'm not sure about if(strpos($sequence,'.')!==false).

ok, then will leave it for any case.

For if(strpos($tableName,'.')!==false), you are right that we don't need this check in 2.0. In 1.1, we need it because findTableNames() return table names prefixed with schema names.

right, so maybe we need to make this:

#in checkIntegrity method
$schema = $schema ? $schema : $this->db->schema->defaultSchema;

#in foreach cycle
$tableName='"'.$schema.".".$tableName.'"';

or dont needed?

$key = reset($table->primaryKey) gets the first column in the primary key. What's wrong with this?

right, my misunderstanding.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Dec 27, 2013

Member

Sounds good to me.

Member

qiangxue commented Dec 27, 2013

Sounds good to me.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Dec 27, 2013

Contributor

ok, so since `"ALTER TABLE "public"."tbl_user_profile" ENABLE TRIGGER ALL; "`` is valid in pgsql, will make changes according to the discussion and submit new push here.

Contributor

Ragazzo commented Dec 27, 2013

ok, so since `"ALTER TABLE "public"."tbl_user_profile" ENABLE TRIGGER ALL; "`` is valid in pgsql, will make changes according to the discussion and submit new push here.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Dec 27, 2013

Contributor

done, ready for review.

Contributor

Ragazzo commented Dec 27, 2013

done, ready for review.

qiangxue added a commit that referenced this pull request Dec 27, 2013

Merge pull request #1648 from Ragazzo/postgresql_features
added postgresql features to reset seq/check integrity

@qiangxue qiangxue merged commit f9c5d87 into yiisoft:master Dec 27, 2013

1 check passed

default The Travis CI build passed
Details
@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Dec 27, 2013

Member

Thanks!

Member

qiangxue commented Dec 27, 2013

Thanks!

@Ragazzo Ragazzo deleted the Ragazzo:postgresql_features branch Dec 27, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment