Opened 17 years ago

Closed 11 years ago

#7561 closed New feature (fixed)

post_syncdb signal should be emitted when syncdb actually finishes

Reported by: cpinto Owned by: nobody
Component: Core (Management commands) Version: dev
Severity: Normal Keywords: geodjango
Cc: egmanoj@…, Jari Pennanen, cmutel@…, Walter Doekes, kristoffer.snabb@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Attachments (2)

post_syncdb_signal.patch (1016 bytes ) - added by cpinto 17 years ago.
final_post_sync.diff (3.1 KB ) - added by aneil 15 years ago.
Adds a signal django.db.models.signals.final_post_syncdb as suggested in comments (see also ticket 13826)

Download all attachments as: .zip

Change History (21)

by cpinto, 17 years ago

Attachment: post_syncdb_signal.patch added

comment:1 by adamfast, 16 years ago

milestone: 1.0
Triage Stage: UnreviewedAccepted

comment:2 by Russell Keith-Magee, 16 years ago

milestone: 1.0post-1.0
Triage Stage: AcceptedDesign decision needed

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.

comment:3 by (none), 16 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:4 by Manoj Govindan <egmanoj@…>, 15 years ago

Cc: egmanoj@… added

by aneil, 15 years ago

Attachment: final_post_sync.diff added

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

comment:5 by Jari Pennanen, 14 years ago

Cc: Jari Pennanen added

comment:6 by cmutel@…, 14 years ago

Cc: cmutel@… added
Keywords: geodjango added

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);

comment:7 by Russell Keith-Magee, 14 years ago

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

comment:8 by Luke Plant, 14 years ago

Severity: Normal
Type: New feature

comment:9 by Bas Peschier, 14 years ago

Easy pickings: unset
UI/UX: unset

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

comment:10 by Jacob, 13 years ago

Triage Stage: Design decision neededAccepted

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.

comment:11 by Russell Keith-Magee, 13 years ago

@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.

comment:12 by Alex Gaynor, 13 years ago

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.

comment:13 by Walter Doekes, 12 years ago

Cc: Walter Doekes added

comment:14 by Kristoffer Snabb, 12 years ago

Cc: kristoffer.snabb@… added

comment:15 by Walter Doekes, 12 years ago

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

in reply to:  15 comment:16 by Carl Meyer, 12 years ago

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.

comment:17 by flyingfrog, 11 years ago

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.

comment:18 by anonymous, 11 years ago

Patch needs improvement: set

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?

comment:19 by Aymeric Augustin, 11 years ago

Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.
Back to Top