post_syncdb signal should be emitted when syncdb actually finishes

[cpinto]

Currently the post_syncdb signal is emitted when all models are synced but before syncdb actually finishes. The proposed patch moves this signal emission further down, to the end of the syncdb pipeline. By doing this, application developers can better manipulate important stuff like database indexes, e.g. dropping table indexes created by Django in favor or better tuned ones, which isn't at all possible otherwise.

[Russell Keith-Magee]

This would be a big backwards incompatible change, as post_syncdb has always been called before the index and custom SQL calls. This means that post_syncdb handlers can insert data indexes are created, which is the usual mode of use for post_syncdb handlers.

There may be an argument for having another signal for post index/custom SQL handling, but moving post_syncdb isn't really a viable option.

Either way, this isn't a bug - it's a feature request, and we're past the v1.0 beta deadline without any discussion. Deferring to post-1.0.

[(none)]

Milestone post-1.0 deleted

[aneil]

Adds a signal django.db.models.signals.final_post_syncdb as suggested in comments (see also ticket 13826)

[cmutel@…]

This also makes working with GeoDjango models in the signal impossible, as the geo columns are not yet created. Instead of being part of the table schema, they are (at least in PostGIS) created by a function that is called when the indices are added, e.g.

SELECT AddGeometryColumn('geo_geography', 'linestring_shape', 4326, 'LINESTRING', 2);

[Russell Keith-Magee]

#13826 reported a specific use case for changing the post-sync signals.

[Bas Peschier]

#16160 reported another use case for changing the post-sync signals.

[Jacob]

Talking with Alex, we're pretty sure that moving the signal to later in the process is the right answer. A second signal just seems janky, and leaving it where it is basically ensures that you can't use post_syncdb handlers with GeoDjango. I don't quite understand Russ's comment above either -- that is, I'm not clear what moving it would break. Further, we've never really specified *when* syncdb runs, so moving it seems like an option.

Marking accepted. Worst case we might need to do some sort of deprecation pathway for the old location, but I think the right thing to do is just to move it.

[Russell Keith-Magee]

@jacob -- my comment was about ordering, and the guarantees that exist when post_sycndb is executed. At the moment, the post_syncdb signal is emitted, and then custom SQL is executed, and then indexes are installed. Moving the signal changes this ordering, and therefore changes the preconditions that you can assume in your post_syncdb handler.

For example -- if your custom SQL does any table modifications, the code in your post_syncdb handler doesn't currently need to worry about that. If you move the post_sycndb handler to be at the end, it will. The problem mostly stems from the fact that the custom SQL hook is an open license to do almost anything with their tables, and we don't know how far people have taken that license.

That said, I'm in complete agreement that it would be desirable to have a signal that is genuinely after everything has been synchronized. I'm also willing to entertain the idea that this is a change that is worth making, and hang the backwards incompatibility. In practice, *I* certainly don't have any code that would be affected, except in that post_sycndb handlers inserting data will be slightly slower because they're inserting after the creation of the index.

[Alex Gaynor]

I'm really struggling to come up with the custom SQL you could execute that would both break your post_syncdb handlers *and* leave your models in a valid state. The current state breaks post_syncdb handlers when used with Django, so we know bugs in the reverse exist.

[Walter Doekes]

A gentle bump here. Can we put this change in master?

[Carl Meyer]

Replying to wdoekes:

A gentle bump here. Can we put this change in master?

The next step is for someone (anyone except the patch author) to review the patch, confirm it still applies cleanly to master, the code looks sane/maintainable, the patch contains tests which fail prior to applying the patch, the patch doc updates as needed (this probably needs a mention in the release notes at least), all tests pass with the patch, and it fixes the issue. If all of that is true, move it to Ready for Checkin triage state and it'll get the attention of someone who can merge it.

[flyingfrog]

The proposed patch (slightly modified to fit into master) seems to be solving an issue with geodjango, specifically postgis.
I got the issue when trying to create a test database and post sync hooks get called before postgis extension is used to create "geometry" type columns.
For example when trying to run tests on my app, it hangs with:

Running post-sync handlers for application contenttypes
DatabaseError: column mymodels_mymodel.point does not exist

I did run tests against a clean django master clone as described in ​https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/unit-tests/#running-the-unit-tests and repeated the tests after applying the patch and results are exactly the same (though in both cases there's some failure... ).
I would gladly contribute to the process of moving the patch to "Ready for Checkin" but i think I'm gonna need some guidance there as I'm a total newcomer to django.

[anonymous]

The patch no longer applies cleanly since schema migrations have been merged and syncdb/post_syncdb have been deprecated in favor of migrate/post_migrate. Could we also write a test for the change?

[Aymeric Augustin]

The original use case was "dropping table indexes created by Django in favor or better tuned ones".

The new migrations framework provide excellent tools to do this, with its RunSQL operation.

It can be positioned where needed by declaring dependencies.