Code

Opened 6 years ago

Last modified 8 months ago

#7561 new New feature

post_syncdb signal should be emitted when syncdb actually finishes

Reported by: cpinto Owned by: nobody
Component: Core (Management commands) Version: master
Severity: Normal Keywords: geodjango
Cc: egmanoj@…, Ciantic, cmutel@…, wdoekes, 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 6 years ago.
final_post_sync.diff (3.1 KB) - added by aneil 4 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 (20)

Changed 6 years ago by cpinto

comment:1 Changed 6 years ago by adamfast

  • milestone set to 1.0
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 6 years ago by russellm

  • milestone changed from 1.0 to post-1.0
  • Triage Stage changed from Accepted to Design 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 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:4 Changed 5 years ago by Manoj Govindan <egmanoj@…>

  • Cc egmanoj@… added

Changed 4 years ago by aneil

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

comment:5 Changed 4 years ago by Ciantic

  • Cc Ciantic added

comment:6 Changed 4 years ago by cmutel@…

  • 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 Changed 3 years ago by russellm

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

comment:8 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to New feature

comment:9 Changed 3 years ago by bpeschier

  • Easy pickings unset
  • UI/UX unset

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

comment:10 Changed 3 years ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

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 Changed 3 years ago by russellm

@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 Changed 3 years ago by Alex

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 Changed 20 months ago by wdoekes

  • Cc wdoekes added

comment:14 Changed 19 months ago by ksnabb

  • Cc kristoffer.snabb@… added

comment:15 follow-up: Changed 14 months ago by wdoekes

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

comment:16 in reply to: ↑ 15 Changed 14 months ago by carljm

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 Changed 9 months ago by 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.

comment:18 Changed 8 months ago by anonymous

  • 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?

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.