Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Migration: Rename tables error-prone #477

Closed
marcj opened this Issue Nov 4, 2013 · 21 comments

Comments

Projects
None yet
7 participants
Owner

marcj commented Nov 4, 2013

As seen in https://travis-ci.org/propelorm/Propel2/jobs/13457004.

PostgreSQL does not rename the autoIncrement sequences for us when we rename a table through the migration.

Owner

marcj commented Jan 11, 2014

We should remove the 'feature' that automatically renames tables through the ALTER TABLE way.
That produces actually more problems than it solves. A simple DROP and CREATE is way less error-prone than the ALTER TABLE RENAME statement as we can not guarantee without much work that all sequences and other information are renamed as well.

Member

staabm commented Jan 11, 2014

But drop+create will loose the data?

Owner

marcj commented Jan 11, 2014

True, but that renaming is heavily error-prone.

  • there is no guaranteed way to detect which tables should be renamed to which (if you remove two and add two new)
  • if you drop a table (not rename) and add a complete new one it can be that the new one has entries from the dropped - which is wrong
  • not all information are moved/merged to the new one (sequences)

If you want to keep all entries you can still overwrite the generated migration file.

Member

staabm commented Jan 11, 2014

Maybe we could create+move data+drop.

Another problem afair: meta changes are not transactional (at least in mysql), so rename is more like all or nothing while several statements could lead to various intermediate results

Owner

marcj commented Jan 11, 2014

What do you mean? The thing is, we could definitely create and then copy the data from old to the new created one so the third point above is eliminated but then you still have the first two to solve.

@marcj marcj referenced this issue Jan 12, 2014

Merged

Bugfix for #473. #476

3 of 4 tasks complete
Owner

marcj commented Jan 12, 2014

@jaugustin @willdurand @hhamon any ideas about that?

Member

staabm commented Jan 12, 2014

Migrations goal is to migrate structure&data from revision to revision. Migrations without data is not an option IMO

Owner

marcj commented Jan 12, 2014

I think you didn't get the point. We're talking not about removing the data migration at all. Only for table name changes respectively removing&adding a table with same columns, because we have no way to detect if it's only a renaming.

Owner

marcj commented Jan 12, 2014

And if we would have a way to detect it, we would have to implement another way of renaming tables (drop+create+copy-data)

Owner

marcj commented Jan 12, 2014

Another problem afair: meta changes are not transactional (at least in mysql), so rename is more like all or nothing while several statements could lead to various intermediate results

That is not a problem as we already build DDL with multiple statements. There's no way to combine all of those into one single.

Member

mpscholten commented Feb 27, 2014

PostgreSQL does not rename the autoIncrement sequences for us when we rename a table through the migration.

Can't we just disable table renaming for PostgreSQL?

Owner

marcj commented Feb 28, 2014

No, please not. And that wouldn't solve all other issues. :(

stood commented Mar 1, 2014

I'm not understand because when I have renamed my table in pgsql I have no problem. The sequence is called when you add new row and is called by

nextval('author_id_seq'::regclass)

The name of sequence not changed.

My version of pgsql is : 9.2 for travis ?

tacman commented Mar 1, 2014

We use PostGRES exclusively, and based on a comment from this list, we
switched to phinx. It working out much better for us, our workflow now is

  • update the schema.xml
  • generate the sql
  • create the migration stub (a php file) with phinx
  • do a diff of the sql generated by propel and manually create the php
    methods up() down() and/or change() based on the diff.

Of course, I'd love it if the last two steps could be at least partially
done with Propel.

We're using Propel1, we'll switch to Propel2 when the PropelBundle for
Symfony is functional, so maybe I'll have more ideas then about a more
Propel-ish integration.

Tac

On Sat, Mar 1, 2014 at 2:38 AM, Mikael notifications@github.com wrote:

I'm not understand because when I have renamed my table in pgsql I have no
problem. The sequence is called when you add new row and is called by

nextval('author_id_seq'::regclass)

The name of sequence not changed.

My version of pgsql is : 9.2 for travis ?

Reply to this email directly or view it on GitHubhttps://github.com/propelorm/Propel2/issues/477#issuecomment-36418791
.

@marcj marcj modified the milestones: alpha-3, beta-1 Mar 13, 2014

Owner

marcj commented Apr 9, 2014

Ok, I guess the only solution here is to make this renaming optional through configuration. It makes no sense to me to have a heavily error-prone component per default activated. I believe making it active through a flag/option is the best solution for those guys who want to use this feature and are able to double-check the outcome of the diffcommand.
At the moment it has been completely removed by PR #476. This issue should now represent the task of making it optional possible again.

@marcj marcj changed the title from [PostgreSQL] Migration: Rename also Sequences to Migration: Rename tables Apr 9, 2014

@marcj marcj changed the title from Migration: Rename tables to Migration: Rename tables error-prone Apr 9, 2014

Contributor

mhitza commented Apr 14, 2014

Would it be an option to check when a drop/create takes place upon tables with similar fields to issue a warning in the diff task, along the lines "Droping table x creating table y, revise migration file in case a rename is required"?

Owner

marcj commented Apr 14, 2014

@mhitza yes would be possible, good idea. 👍

Owner

marcj commented Apr 15, 2014

screen shot 2014-04-15 at 18 56 14

et voilà

@marcj marcj self-assigned this Apr 16, 2014

Owner

willdurand commented Apr 16, 2014

Nice!

stood commented Apr 16, 2014

👍

@marcj marcj closed this in #611 Apr 16, 2014

marcj added a commit that referenced this issue Apr 16, 2014

Merge pull request #611 from marcj/master
Fixed #477, Fixed #607, Fixed #604, Fixed #610
Member

mpscholten commented Apr 16, 2014

Very nice solution 👍

mpscholten added a commit to mpscholten/Propel2 that referenced this issue Aug 24, 2014

Fixed relation handling. Fixed #473, #518, #262.
Fixed bug: crossTable with at least one FK with multiple local columns
Fixed bug: crossTable with more than two FKs
Fixed bug: crossTable with two PK-FK and one PK-nonFK
Fixed bug: crossTable: setRelations(Collection),
removeRelation($rel), save() doesn’t remove $rel.
Added Partial-checking also to CrossFK getter.
Fixed bug: wrong "if ($obj->isNew())” check in getRelation() methods.
Improved performance of hashCode.
Improved ObjectCollection checks
Modified clearAllReferences as in PHP 5.4 there's no workaround like
that necessary anymore.
Fixed internal object relation reference tracking, especially
unlinking back-references.

Added also the table rename fix for #477, so we do not rename tables
anymore. This was necessary because otherwise with this new incoming
tests for relations the test suite won't be green.

mpscholten added a commit to mpscholten/Propel2 that referenced this issue Aug 24, 2014

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