perseas / Pyrseas

Provides utilities for Postgres database schema versioning.
https://perseas.github.io/
BSD 3-Clause "New" or "Revised" License
394 stars 67 forks source link

Support --revert for tables #256

Open stijntratsaertit opened 6 months ago

stijntratsaertit commented 6 months ago

I want to generate up & down migrations for my database. I use yamltodb to generate the SQL. I saw the --revert flag was experimental, not for tables.

What's the status on this? What's required? If I would propose to work on this, do you have some tips?

jmafc commented 6 months ago

The initial submission for the --revert flag was added a little over ten years ago (see f00b74dcfa48c735771c083d58020345798ca5db and e296a46104d556fdd5ce8abffb75f20e21b55dca) for schemas and sequences. Needless to say, a lot of water has gone under the bridge, in particular, the topological sorting of database objects (implemented mostly by @dvarrazzo).

If you look at the comments on f00b74d, you'll notice that the basic strategy was to "swap the top level database objects". What that means is that in Database::diff_map() we swap self.db (the old or "source" database) with self.ndb (the new or "target" database). The problem is that the trees of objects "hanging" from the top level objects aren't exactly symmetrical, IIRC that was the case even before the topological ordering changes. The latter, however, introduced oid's because for the source database we fetch pg_depend information, whereas the "target" is represented by a YAML file and lacks the oid's (since the YAML could've been generated from a different PG database). Before committing Daniele's changes in 2017 for 0.8.0, I was trying to think of a way to replace or supplement oid's by some other combination of identifiers, so that the "old" and "new" db's could be compared symmetrically (see #119).

Anyway, if you'd like to give it a try, I would suggest that you create one or two simple tests in tests/dbobject/test_table.py, say, one to CREATE a table and verify that revert=True generates code to DROP it, and vice versa (you can look at test_schema.py and test_sequence.py for the basic approach). Then, I'm afraid to say, you'll have to put on your debugging hat.

stijntratsaertit commented 6 months ago

Thanks for the quick response! I'll see if I can have a go at it and toy around anytime soon

stijntratsaertit commented 5 months ago

I've been toying around a bit and got a revert table renaming test to work (which has been the hardest to fix up until now) by hacking it a bit (see here). The biggest issue there is that the oldnames have to get mapped from the old to the new target state. Would a suggested implementation of yours look similar to the hacky solution or are there other methods I could leverage to improve this?

jmafc commented 5 months ago

As I recall, the oldname attribute was introduced quite early (much before @dvarrazzo's topological sorting). It was my primitive attempt at trying to deal with RENAME-type changes and was never a satisfactory solution: it sort of worked in tests because it was easy to introduce the attribute in test data, but in practical use, it meant the user had to edit the YAML files (in order to generate the RENAME) but the edit could not be saved in a repository.

I'm fuzzy about what happened when Daniele worked on dependencies (I think he asked about oldname either in GH or in separate emails), but think he skirted around it. @dvarrazzo, do you recall (or do you have any suggestions for Stijn)?

Perhaps what should be done is to "disable" oldname and rename processing, before trying to implement the "revert" functionality. Unfortunately, I count 28 calls to obj.set_oldname and nine test_rename_* tests so it may be somewhat cumbersome to attempt such a change, but it may clarify the issues behind trying to revert more straightforward changes, i.e., CREATE TABLE/DROP TABLE and vice versa.