From 0044c263684f0915c62a2a7942e84c5ebe2207f7 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Tue, 19 Sep 2006 17:49:51 +0000 Subject: [PATCH] - post_update behavior improved; does a better job at not updating too many rows, updates only required columns [ticket:208] --- CHANGES | 3 + lib/sqlalchemy/orm/dependency.py | 25 ++-- lib/sqlalchemy/orm/mapper.py | 7 +- lib/sqlalchemy/orm/sync.py | 5 +- lib/sqlalchemy/orm/unitofwork.py | 27 +++- test/orm/cycles.py | 218 +++++++++++++++++++++++-------- 6 files changed, 209 insertions(+), 76 deletions(-) diff --git a/CHANGES b/CHANGES index bf64c367d9..89c0877b56 100644 --- a/CHANGES +++ b/CHANGES @@ -19,6 +19,9 @@ after commit/rollback - added an implicit close() on the cursor in ResultProxy when the result closes - added scalar() method to ComposedSQLEngine +- post_update behavior improved; does a better job at not +updating too many rows, updates only required columns +[ticket:208] 0.2.8 - cleanup on connection methods + documentation. custom DBAPI diff --git a/lib/sqlalchemy/orm/dependency.py b/lib/sqlalchemy/orm/dependency.py index 1af368431b..b734b34bec 100644 --- a/lib/sqlalchemy/orm/dependency.py +++ b/lib/sqlalchemy/orm/dependency.py @@ -58,7 +58,7 @@ class DependencyProcessor(object): return (obj1, obj2) else: return (obj2, obj1) - + def process_dependencies(self, task, deplist, uowcommit, delete = False): """this method is called during a flush operation to synchronize data between a parent and child object. it is called within the context of the various mappers and sometimes individual objects sorted according to their @@ -81,6 +81,12 @@ class DependencyProcessor(object): this dependency processor represents""" return sessionlib.attribute_manager.get_history(obj, self.key, passive = passive) + def _conditional_post_update(self, obj, uowcommit, related): + if obj is not None and self.post_update: + for x in related: + if x is not None and (uowcommit.is_deleted(x) or not hasattr(x, '_instance_key')): + uowcommit.register_object(obj, postupdate=True, post_update_cols=self.syncrules.dest_columns()) + break class OneToManyDP(DependencyProcessor): def register_dependencies(self, uowcommit): @@ -103,21 +109,18 @@ class OneToManyDP(DependencyProcessor): for child in childlist.deleted_items(): if child is not None and childlist.hasparent(child) is False: self._synchronize(obj, child, None, True) - if self.post_update: - uowcommit.register_object(child, postupdate=True) + self._conditional_post_update(child, uowcommit, [obj]) for child in childlist.unchanged_items(): if child is not None: self._synchronize(obj, child, None, True) - if self.post_update: - uowcommit.register_object(child, postupdate=True) + self._conditional_post_update(child, uowcommit, [obj]) else: for obj in deplist: childlist = self.get_object_dependencies(obj, uowcommit, passive=True) if childlist is not None: for child in childlist.added_items(): self._synchronize(obj, child, None, False) - if child is not None and self.post_update: - uowcommit.register_object(child, postupdate=True) + self._conditional_post_update(child, uowcommit, [obj]) for child in childlist.deleted_items(): if not self.cascade.delete_orphan: self._synchronize(obj, child, None, True) @@ -194,16 +197,16 @@ class ManyToOneDP(DependencyProcessor): # before we can DELETE the row for obj in deplist: self._synchronize(obj, None, None, True) - uowcommit.register_object(obj, postupdate=True) + childlist = self.get_object_dependencies(obj, uowcommit, passive=True) + self._conditional_post_update(obj, uowcommit, childlist.deleted_items() + childlist.unchanged_items() + childlist.added_items()) else: for obj in deplist: childlist = self.get_object_dependencies(obj, uowcommit, passive=True) if childlist is not None: for child in childlist.added_items(): self._synchronize(obj, child, None, False) - if self.post_update: - uowcommit.register_object(obj, postupdate=True) - + self._conditional_post_update(obj, uowcommit, childlist.deleted_items() + childlist.unchanged_items() + childlist.added_items()) + def preprocess_dependencies(self, task, deplist, uowcommit, delete = False): #print self.mapper.mapped_table.name + " " + self.key + " " + repr(len(deplist)) + " PRE process_dep isdelete " + repr(delete) + " direction " + repr(self.direction) # TODO: post_update instructions should be established in this step as well diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py index 837f17a332..194b2b9e6f 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -703,12 +703,13 @@ class Mapper(object): def _setattrbycolumn(self, obj, column, value): self.columntoproperty[column][0].setattr(obj, value) - - def save_obj(self, objects, uow, postupdate=False): + + def save_obj(self, objects, uow, postupdate=False, post_update_cols=None): """called by a UnitOfWork object to save objects, which involves either an INSERT or an UPDATE statement for each table used by this mapper, for each element of the list.""" #print "SAVE_OBJ MAPPER", self.class_.__name__, objects + connection = uow.transaction.connection(self) if not postupdate: @@ -785,6 +786,8 @@ class Mapper(object): # doing an UPDATE ? get the history for the attribute, with "passive" # so as not to trigger any deferred loads. if there is a new # value, add it to the bind parameters + if post_update_cols is not None and col not in post_update_cols: + continue prop = self._getpropbycolumn(col, False) if prop is None: continue diff --git a/lib/sqlalchemy/orm/sync.py b/lib/sqlalchemy/orm/sync.py index 8004776c8b..c43bd69e0a 100644 --- a/lib/sqlalchemy/orm/sync.py +++ b/lib/sqlalchemy/orm/sync.py @@ -81,7 +81,10 @@ class ClauseSynchronizer(object): sqlclause.accept_visitor(processor) if len(self.syncrules) == rules_added: raise ArgumentError("No syncrules generated for join criterion " + str(sqlclause)) - + + def dest_columns(self): + return [r.dest_column for r in self.syncrules if r.dest_column is not None] + def execute(self, source, dest, obj=None, child=None, clearkeys=None): for rule in self.syncrules: rule.execute(source, dest, obj, child, clearkeys) diff --git a/lib/sqlalchemy/orm/unitofwork.py b/lib/sqlalchemy/orm/unitofwork.py index 2dac05b1a6..0a56692271 100644 --- a/lib/sqlalchemy/orm/unitofwork.py +++ b/lib/sqlalchemy/orm/unitofwork.py @@ -239,7 +239,7 @@ class UOWTransaction(object): self.__is_executing = False # TODO: shouldnt be able to register stuff here that is not in the enclosing Session - def register_object(self, obj, isdelete = False, listonly = False, postupdate=False, **kwargs): + def register_object(self, obj, isdelete = False, listonly = False, postupdate=False, post_update_cols=None, **kwargs): """adds an object to this UOWTransaction to be updated in the database. 'isdelete' indicates whether the object is to be deleted or saved (update/inserted). @@ -258,7 +258,7 @@ class UOWTransaction(object): self.mappers.add(mapper) task = self.get_task_by_mapper(mapper) if postupdate: - mod = task.append_postupdate(obj) + mod = task.append_postupdate(obj, post_update_cols) if mod: self._mark_modified() return @@ -283,7 +283,13 @@ class UOWTransaction(object): #if self.__is_executing: # raise "test assertion failed" self.__modified = True + + def is_deleted(self, obj): + mapper = object_mapper(obj) + task = self.get_task_by_mapper(mapper) + return task.is_deleted(obj) + def get_task_by_mapper(self, mapper, dontcreate=False): """every individual mapper involved in the transaction has a single corresponding UOWTask object, which stores all the operations involved @@ -650,9 +656,11 @@ class UOWTask(object): rec.isdelete = True return retval - def append_postupdate(self, obj): + def append_postupdate(self, obj, post_update_cols): # postupdates are UPDATED immeditely (for now) - self.mapper.save_obj([obj], self.uowtransaction, postupdate=True) + # convert post_update_cols list to a Set so that __hashcode__ is used to compare columns + # instead of __eq__ + self.mapper.save_obj([obj], self.uowtransaction, postupdate=True, post_update_cols=util.Set(post_update_cols)) return True def delete(self, obj): @@ -711,7 +719,16 @@ class UOWTask(object): if obj in self.objects: return True return False - + + def is_inserted(self, obj): + return not hasattr(obj, '_instance_key') + + def is_deleted(self, obj): + try: + return self.objects[obj].isdelete + except KeyError: + return False + def get_elements(self, polymorphic=False): if polymorphic: for task in self.polymorphic_tasks(): diff --git a/test/orm/cycles.py b/test/orm/cycles.py index dbc234d858..dd28364697 100644 --- a/test/orm/cycles.py +++ b/test/orm/cycles.py @@ -214,10 +214,12 @@ class OneToManyManyToOneTest(AssertMixin): ball = Table('ball', metadata, Column('id', Integer, Sequence('ball_id_seq', optional=True), primary_key=True), Column('person_id', Integer), + Column('data', String(30)) ) person = Table('person', metadata, Column('id', Integer, Sequence('person_id_seq', optional=True), primary_key=True), Column('favorite_ball_id', Integer, ForeignKey('ball.id')), + Column('data', String(30)) # Column('favorite_ball_id', Integer), ) @@ -267,10 +269,12 @@ class OneToManyManyToOneTest(AssertMixin): def testpostupdate_m2o(self): """tests a cycle between two rows, with a post_update on the many-to-one""" class Person(object): - pass + def __init__(self, data): + self.data = data class Ball(object): - pass + def __init__(self, data): + self.data = data Ball.mapper = mapper(Ball, ball) Person.mapper = mapper(Person, person, properties= dict( @@ -281,12 +285,12 @@ class OneToManyManyToOneTest(AssertMixin): print str(Person.mapper.props['balls'].primaryjoin) - b = Ball() - p = Person() + b = Ball('some data') + p = Person('some data') p.balls.append(b) - p.balls.append(Ball()) - p.balls.append(Ball()) - p.balls.append(Ball()) + p.balls.append(Ball('some data')) + p.balls.append(Ball('some data')) + p.balls.append(Ball('some data')) p.favorateBall = b sess = create_session() sess.save(b) @@ -294,24 +298,24 @@ class OneToManyManyToOneTest(AssertMixin): self.assert_sql(db, lambda: sess.flush(), [ ( - "INSERT INTO person (favorite_ball_id) VALUES (:favorite_ball_id)", - {'favorite_ball_id': None} + "INSERT INTO person (favorite_ball_id, data) VALUES (:favorite_ball_id, :data)", + {'favorite_ball_id': None, 'data':'some data'} ), ( - "INSERT INTO ball (person_id) VALUES (:person_id)", - lambda ctx:{'person_id':p.id} + "INSERT INTO ball (person_id, data) VALUES (:person_id, :data)", + lambda ctx:{'person_id':p.id, 'data':'some data'} ), ( - "INSERT INTO ball (person_id) VALUES (:person_id)", - lambda ctx:{'person_id':p.id} + "INSERT INTO ball (person_id, data) VALUES (:person_id, :data)", + lambda ctx:{'person_id':p.id, 'data':'some data'} ), ( - "INSERT INTO ball (person_id) VALUES (:person_id)", - lambda ctx:{'person_id':p.id} + "INSERT INTO ball (person_id, data) VALUES (:person_id, :data)", + lambda ctx:{'person_id':p.id, 'data':'some data'} ), ( - "INSERT INTO ball (person_id) VALUES (:person_id)", - lambda ctx:{'person_id':p.id} + "INSERT INTO ball (person_id, data) VALUES (:person_id, :data)", + lambda ctx:{'person_id':p.id, 'data':'some data'} ), ( "UPDATE person SET favorite_ball_id=:favorite_ball_id WHERE person.id = :person_id", @@ -320,24 +324,24 @@ class OneToManyManyToOneTest(AssertMixin): ], with_sequences= [ ( - "INSERT INTO person (id, favorite_ball_id) VALUES (:id, :favorite_ball_id)", - lambda ctx:{'id':ctx.last_inserted_ids()[0], 'favorite_ball_id': None} + "INSERT INTO person (id, favorite_ball_id, data) VALUES (:id, :favorite_ball_id, :data)", + lambda ctx:{'id':ctx.last_inserted_ids()[0], 'favorite_ball_id': None, 'data':'some data'} ), ( - "INSERT INTO ball (id, person_id) VALUES (:id, :person_id)", - lambda ctx:{'id':ctx.last_inserted_ids()[0],'person_id':p.id} + "INSERT INTO ball (id, person_id, data) VALUES (:id, :person_id, :data)", + lambda ctx:{'id':ctx.last_inserted_ids()[0],'person_id':p.id, 'data':'some data'} ), ( - "INSERT INTO ball (id, person_id) VALUES (:id, :person_id)", - lambda ctx:{'id':ctx.last_inserted_ids()[0],'person_id':p.id} + "INSERT INTO ball (id, person_id, data) VALUES (:id, :person_id, :data)", + lambda ctx:{'id':ctx.last_inserted_ids()[0],'person_id':p.id, 'data':'some data'} ), ( - "INSERT INTO ball (id, person_id) VALUES (:id, :person_id)", - lambda ctx:{'id':ctx.last_inserted_ids()[0],'person_id':p.id} + "INSERT INTO ball (id, person_id, data) VALUES (:id, :person_id, :data)", + lambda ctx:{'id':ctx.last_inserted_ids()[0],'person_id':p.id, 'data':'some data'} ), ( - "INSERT INTO ball (id, person_id) VALUES (:id, :person_id)", - lambda ctx:{'id':ctx.last_inserted_ids()[0],'person_id':p.id} + "INSERT INTO ball (id, person_id, data) VALUES (:id, :person_id, :data)", + lambda ctx:{'id':ctx.last_inserted_ids()[0],'person_id':p.id, 'data':'some data'} ), # heres the post update ( @@ -369,10 +373,12 @@ class OneToManyManyToOneTest(AssertMixin): def testpostupdate_o2m(self): """tests a cycle between two rows, with a post_update on the one-to-many""" class Person(object): - pass + def __init__(self, data): + self.data = data class Ball(object): - pass + def __init__(self, data): + self.data = data Ball.mapper = mapper(Ball, ball) Person.mapper = mapper(Person, person, properties= dict( @@ -383,14 +389,14 @@ class OneToManyManyToOneTest(AssertMixin): print str(Person.mapper.props['balls'].primaryjoin) - b = Ball() - p = Person() + b = Ball('some data') + p = Person('some data') p.balls.append(b) - b2 = Ball() + b2 = Ball('some data') p.balls.append(b2) - b3 = Ball() + b3 = Ball('some data') p.balls.append(b3) - b4 = Ball() + b4 = Ball('some data') p.balls.append(b4) p.favorateBall = b sess = create_session() @@ -398,24 +404,24 @@ class OneToManyManyToOneTest(AssertMixin): self.assert_sql(db, lambda: sess.flush(), [ ( - "INSERT INTO ball (person_id) VALUES (:person_id)", - {'person_id':None} + "INSERT INTO ball (person_id, data) VALUES (:person_id, :data)", + {'person_id':None, 'data':'some data'} ), ( - "INSERT INTO ball (person_id) VALUES (:person_id)", - {'person_id':None} + "INSERT INTO ball (person_id, data) VALUES (:person_id, :data)", + {'person_id':None, 'data':'some data'} ), ( - "INSERT INTO ball (person_id) VALUES (:person_id)", - {'person_id':None} + "INSERT INTO ball (person_id, data) VALUES (:person_id, :data)", + {'person_id':None, 'data':'some data'} ), ( - "INSERT INTO ball (person_id) VALUES (:person_id)", - {'person_id':None} + "INSERT INTO ball (person_id, data) VALUES (:person_id, :data)", + {'person_id':None, 'data':'some data'} ), ( - "INSERT INTO person (favorite_ball_id) VALUES (:favorite_ball_id)", - lambda ctx:{'favorite_ball_id':b.id} + "INSERT INTO person (favorite_ball_id, data) VALUES (:favorite_ball_id, :data)", + lambda ctx:{'favorite_ball_id':b.id, 'data':'some data'} ), # heres the post update on each one-to-many item ( @@ -437,24 +443,24 @@ class OneToManyManyToOneTest(AssertMixin): ], with_sequences=[ ( - "INSERT INTO ball (id, person_id) VALUES (:id, :person_id)", - lambda ctx:{'id':ctx.last_inserted_ids()[0], 'person_id':None} + "INSERT INTO ball (id, person_id, data) VALUES (:id, :person_id, :data)", + lambda ctx:{'id':ctx.last_inserted_ids()[0], 'person_id':None, 'data':'some data'} ), ( - "INSERT INTO ball (id, person_id) VALUES (:id, :person_id)", - lambda ctx:{'id':ctx.last_inserted_ids()[0], 'person_id':None} + "INSERT INTO ball (id, person_id, data) VALUES (:id, :person_id, :data)", + lambda ctx:{'id':ctx.last_inserted_ids()[0], 'person_id':None, 'data':'some data'} ), ( - "INSERT INTO ball (id, person_id) VALUES (:id, :person_id)", - lambda ctx:{'id':ctx.last_inserted_ids()[0], 'person_id':None} + "INSERT INTO ball (id, person_id, data) VALUES (:id, :person_id, :data)", + lambda ctx:{'id':ctx.last_inserted_ids()[0], 'person_id':None, 'data':'some data'} ), ( - "INSERT INTO ball (id, person_id) VALUES (:id, :person_id)", - lambda ctx:{'id':ctx.last_inserted_ids()[0], 'person_id':None} + "INSERT INTO ball (id, person_id, data) VALUES (:id, :person_id, :data)", + lambda ctx:{'id':ctx.last_inserted_ids()[0], 'person_id':None, 'data':'some data'} ), ( - "INSERT INTO person (id, favorite_ball_id) VALUES (:id, :favorite_ball_id)", - lambda ctx:{'id':ctx.last_inserted_ids()[0], 'favorite_ball_id':b.id} + "INSERT INTO person (id, favorite_ball_id, data) VALUES (:id, :favorite_ball_id, :data)", + lambda ctx:{'id':ctx.last_inserted_ids()[0], 'favorite_ball_id':b.id, 'data':'some data'} ), ( "UPDATE ball SET person_id=:person_id WHERE ball.id = :ball_id", @@ -498,11 +504,109 @@ class OneToManyManyToOneTest(AssertMixin): ), ( "DELETE FROM ball WHERE ball.id = :id", - None - # the order of deletion is not predictable, but its roughly: - # lambda ctx:[{'id': b.id}, {'id': b2.id}, {'id': b3.id}, {'id': b4.id}] + lambda ctx:[{'id': b.id}, {'id': b2.id}, {'id': b3.id}, {'id': b4.id}] ) ]) + +class SelfReferentialPostUpdateTest(AssertMixin): + def setUpAll(self): + global metadata, node_table + metadata = BoundMetaData(testbase.db) + node_table = Table('node', metadata, + Column('id', Integer, primary_key=True), + Column('path', String(50), nullable=False), + Column('parent_id', Integer, ForeignKey('node.id'), nullable=True), + Column('prev_sibling_id', Integer, ForeignKey('node.id'), nullable=True), + Column('next_sibling_id', Integer, ForeignKey('node.id'), nullable=True) + ) + node_table.create() + def tearDownAll(self): + node_table.drop() + + def testbasic(self): + """test that post_update only fires off when needed. + + this test case used to produce many superfluous update statements, particularly upon delete""" + class Node(object): + def __init__(self, path=''): + self.path = path + + n_mapper = mapper(Node, node_table, properties={ + 'children': relation( + Node, + primaryjoin=node_table.c.id==node_table.c.parent_id, + lazy=True, + cascade="all", + backref=backref("parent", primaryjoin=node_table.c.parent_id==node_table.c.id, foreignkey=node_table.c.id) + ), + 'prev_sibling': relation( + Node, + primaryjoin=node_table.c.prev_sibling_id==node_table.c.id, + foreignkey=node_table.c.id, + lazy=True, + uselist=False + ), + 'next_sibling': relation( + Node, + primaryjoin=node_table.c.next_sibling_id==node_table.c.id, + foreignkey=node_table.c.id, + lazy=True, + uselist=False, + post_update=True + ) + }) + + session = create_session() + + def append_child(parent, child): + if len(parent.children): + parent.children[-1].next_sibling = child + child.prev_sibling = parent.children[-1] + parent.children.append(child) + + def remove_child(parent, child): + child.parent = None + node = child.next_sibling + node.prev_sibling = child.prev_sibling + child.prev_sibling.next_sibling = node + session.delete(child) + root = Node('root') + + about = Node('about') + cats = Node('cats') + stories = Node('stories') + bruce = Node('bruce') + + append_child(root, about) + assert(about.prev_sibling is None) + append_child(root, cats) + assert(cats.prev_sibling is about) + assert(cats.next_sibling is None) + assert(about.next_sibling is cats) + assert(about.prev_sibling is None) + append_child(root, stories) + append_child(root, bruce) + session.save(root) + session.flush() + + remove_child(root, cats) + # pre-trigger lazy loader on 'cats' to make the test easier + cats.children + + self.assert_sql(db, lambda: session.flush(), [ + ( + "UPDATE node SET prev_sibling_id=:prev_sibling_id WHERE node.id = :node_id", + lambda ctx:{'prev_sibling_id':about.id, 'node_id':stories.id} + ), + ( + "UPDATE node SET next_sibling_id=:next_sibling_id WHERE node.id = :node_id", + lambda ctx:{'next_sibling_id':stories.id, 'node_id':about.id} + ), + ( + "DELETE FROM node WHERE node.id = :id", + lambda ctx:[{'id':cats.id}] + ), + ]) if __name__ == "__main__": testbase.main() -- 2.47.2