Re: [Django] #7561: post_syncdb signal should be emitted when syncdb actually finishes
Django
-------------------------------------+-------------------------------------
Reporter: cpinto | Owner: nobody
Type: New | Status: new
feature | Component: Core (Management
Milestone: | commands)
Version: SVN | Severity: Normal
Resolution: | Keywords: geodjango
Triage Stage: Design | Has patch: 1
decision needed | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by bpeschier):
* ui_ux: => 0
* easy: => 0
Comment:
#16160 reported another use case for changing the post-sync signals.
--
Ticket URL: <https://code.djangoproject.com/ticket/7561#comment:9>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: cpinto | Owner: nobody
Type: New | Status: new
feature | Component: Core (Management
Milestone: | commands)
Version: SVN | Severity: Normal
Resolution: | Keywords: geodjango
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by jacob):
* stage: Design decision needed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/7561#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: cpinto | Owner: nobody
Type: New | Status: new
feature | Component: Core (Management
Milestone: | commands)
Version: SVN | Severity: Normal
Resolution: | Keywords: geodjango
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/7561#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: cpinto | Owner: nobody
Type: New | Status: new
feature | Component: Core (Management
Milestone: | commands)
Version: SVN | Severity: Normal
Resolution: | Keywords: geodjango
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/7561#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: cpinto | Owner: nobody
Component: Core (Management | Version: master
commands) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: geodjango | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by wdoekes):
* cc: wdoekes (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/7561#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: cpinto | Owner: nobody
Type: New feature | Status: new
Component: Core (Management | Version: master
commands) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: geodjango | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: kristoffer.snabb@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/7561#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: cpinto | Owner: nobody
Type: New feature | Status: new
Component: Core (Management | Version: master
commands) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: geodjango | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by wdoekes):
A gentle bump here. Can we put this change in master?
--
Ticket URL: <https://code.djangoproject.com/ticket/7561#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: cpinto | Owner: nobody
Type: New feature | Status: new
Component: Core (Management | Version: master
commands) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: geodjango | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by carljm):
Replying to [comment:15 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/7561#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: cpinto | Owner: nobody
Type: New feature | Status: new
Component: Core (Management | Version: master
commands) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: geodjango | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/7561#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: cpinto | Owner: nobody
Type: New feature | Status: new
Component: Core (Management | Version: master
commands) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: geodjango | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/7561#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: cpinto | Owner: nobody
Component: Core (Management | Version: master
Severity: Normal | Triage Stage: Accepted
Keywords: geodjango | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/7561#comment:19>