Opened 15 years ago

Closed 13 years ago

Last modified 13 years ago

#10215 closed (fixed)

Dont rollback transaction on errors in management/loaddata in "non-commit" mode.

Reported by: Vadim Fint Owned by: nobody
Component: Core (Other) Version: dev
Severity: Keywords:
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

loaddata command by default tries to use transaction for all it's work. But it accepts commit=False, which can made whole loaddata call part of outer transaction (this is usefull for call_command :)). But.. on any error - loaddata will close transaction in any way :(.

  1. Start transaction
  2. Create something in db
  3. Call loaddata with commit=False (in try..except block -- we can have possible error and we know that)
  4. Got an error while loaddata
  5. Ooops.. loaddata rollbacks whole transaction, thus my changes in 1st step are gone
  6. grrrrr.

But.. loaddata for easy using in non-interactive mode should also return something "okay" to callee, right now there is no way to determine that. But this is not covered by this ticket :).
p.s. not sure about component -- "Core framework" or "django-admin.py"?

Attachments (1)

10215-loaddata-valid-commit-r1.patch (2.7 KB ) - added by Vadim Fint 15 years ago.
fixup (r1).

Download all attachments as: .zip

Change History (13)

by Vadim Fint, 15 years ago

fixup (r1).

comment:1 by Vadim Fint, 15 years ago

Needs tests: set

auch, forgot about trac links (in attachment above "r1" means "revision 1 of patch") :).

comment:2 by Vadim Fint, 15 years ago

  1. Start transaction
  2. Create something in db
  3. Call loaddata with commit=False (in try..except block -- we can have possible error and we know that)
  4. Got an error while loaddata
  5. Ooops.. loaddata rollbacks whole transaction, thus my changes in 1st step are gone
  6. grrrrr.

comment:3 by Karen Tracey, 15 years ago

Triage Stage: UnreviewedDesign decision needed

Note r8336 added this loaddata commit option (calling it "stealth" -- it's not documented and not available from the command line) with an eye to using it for implementing #8138 (speeding up tests by using transaction rollback instead of flushing/reloading the DB). As it turns out this loaddata option is not necessary given the way #8138 was ultimately implemented, but I didn't actually remove it. (I meant to ask if others thought it should be removed, but I seem to have forgotten to do that.) Given it's not needed for #8138, at this point I don't know if it would be better to remove it entirely or try to fix problems like this. I'm not exactly sure how you are going to proceed after loaddata runs into a problem but doesn't rollback -- it may have partially loaded fixtures into the DB...you're not going to want to ultimately commit that, are you?

Personally I think it would be better to remove the option entirely since I don't believe it was ever intended for outside use and the internal use it was thought to be needed for got implemented in a way that made it unnecessary. Other opinions?

comment:4 by Alex Gaynor, 15 years ago

I agree it should be removed, partially loading fixtures seems like an all around bad idea, considering it gives way to inserting data without referential integrity, in addition to other concerns about incomplete data.

comment:5 by Vadim Fint, 15 years ago

Note that partially loading textures is bad idea in any way - I'm completely agree with that. But aggressive rollbacks is a bad idea too.

I'm noticed this bug (feature?) to you, because one half of function tries to deal with graceful rollbacks and commits, another half - dont. This functionality should be either removed at all, or completed.

kmtracey, yes you are right. I dont want to commit changes to db at all, this is quite useful for testing (experimenting with (already working!) forked testrunner :)). In transaction test model (#8138, for example) such loaddata error raises hard to detect error -- somewhere in the test db backend will scream about out of sync in commands.

I'm not thought about that -- possibly where strict reason for not doing that.... but... what about simply replace explicit rollback/commit in loaddata to commit_unless_managed and rollback_unless_managed?

Also that should be a good idea to make some way to globally disable transaction management. #8138 introduces settings.DATABASE_SUPPORTS_TRANSACTIONS (which is AWFUL! what if my db user has no permissions to create new tables?), but it seem to be used in #8138 only right now. There is an ugly technique (again, from #8138) - replace transaction functions with nop().

comment:6 by Russell Keith-Magee, 15 years ago

Karen - when I labeled this as a stealth option, it was because there isn't any use for this as an exposed command line option, and we don't have a place to document management command options that are available programatically but not from the command line. In this case, the option is a niche requirement - granted, it isn't needed for #8138, but I can conceive of ways that it could be used. If anyone is smart enough to realize that transactionless loaddata is what they need, they're smart enough to read the code and work out that it is possible. The code is well documented in this area, so it shouldn't be difficult to use the option (MockSoul certainly managed :-)

Alex - integrity isn't the issue here. If you're loading data in a transaction, you either get all the data or you get none of the data. By definition, you can't get into an invalid state. If you're not using a transaction, but you are using a database with referential integrity, then any non-trivial fixture (i.e., a fixture with forward references) won't even load, so you're going to hit problems pretty early. If your database doesn't have referential integrity, then you've already taken on the burden of validating references at runtime before use.

Put me down as a +0 for keeping this option; in which case, MockSoul's patch seems like the right thing to do. If we're deferring responsibility of the transaction to the external method, we should defer _everything_ to the external method. Coupled with #10200, that should give MockSoul (et al) everything they need to correctly handle transactions in an external method.

comment:7 by Vadim Fint, 15 years ago

Change django to fit my needs is not hard. Change everything - is not hard. The main "oops" is that error is absolutely strange :). If loaddata will fail (silently for code, unless #10200) and it was surrounded by outer transaction - you will need to check all transactions before your error (which will be in first db query). I took ~1hour for me to understood that's going on. I just dont want somebody else to learn django in such way ;).

#10200 will be enough if there is no need to continue after loaddata fail (I suppose 99% users dont).

comment:8 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

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

Resolution: fixed
Status: newclosed

(In [13978]) Fixed #10215 -- Ensured that there is parity between enter and leave transaction calls in loaddata when commit=False. The test case for this is the fixtures_regress unittests under MyISAM, which were failing previous to this fix. Thanks to MockSoul for the report.

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

(In [13979]) [1.2.X] Fixed #10215 -- Ensured that there is parity between enter and leave transaction calls in loaddata when commit=False. The test case for this is the fixtures_regress unittests under MyISAM, which were failing previous to this fix. Thanks to MockSoul for the report.

Backport of r13978 from trunk.

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

For the benefit of history: Karen and I discussed this on IRC. We came to the conclusion that although commit=False is a stealth option, it's been around for long enough that removing it wasn't really a viable option. Correcting this problem still leaves the issue of how to appropriately identify and handle fixture errors in external code (See #10200), but that's a separate problem.

comment:12 by Vadim Fint, 13 years ago

That was a long waiting.. :) Thanks, guys!

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