Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support --revert for tables #256

Open
stijntratsaertit opened this issue Jan 19, 2024 · 4 comments
Open

Support --revert for tables #256

stijntratsaertit opened this issue Jan 19, 2024 · 4 comments
Labels

Comments

@stijntratsaertit
Copy link

stijntratsaertit commented Jan 19, 2024

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 jmafc added the yamltodb label Jan 19, 2024
@jmafc
Copy link
Member

jmafc commented Jan 19, 2024

The initial submission for the --revert flag was added a little over ten years ago (see f00b74d and e296a46) 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
Copy link
Author

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

@stijntratsaertit
Copy link
Author

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
Copy link
Member

jmafc commented Feb 14, 2024

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.

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

No branches or pull requests

2 participants