- major simplification of _collect_update_commands. in particular,

we only call upon the history API fully for primary key columns.
We also now skip the whole step of looking at PK columns and using
any history at all if no net changes are detected on the object.

4 files changed, 77 insertions, 88 deletions

diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index 1e1291857..14dc5d7f8 100644
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -1894,6 +1894,19 @@ class Mapper(InspectionAttr):
     """
 
     @_memoized_configured_property
+    def _propkey_to_col(self):
+        return dict(
+            (
+                table,
+                dict(
+                    (self._columntoproperty[col].key, col)
+                    for col in columns
+                )
+            )
+            for table, columns in self._cols_by_table.items()
+        )
+
+    @_memoized_configured_property
     def _col_to_propkey(self):
         return dict(
             (
diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py
index d511c0816..228cfef3a 100644
--- a/lib/sqlalchemy/orm/persistence.py
+++ b/lib/sqlalchemy/orm/persistence.py
@@ -304,96 +304,70 @@ def _collect_update_commands(base_mapper, uowtransaction,
         params = {}
         value_params = {}
 
-        hasdata = hasnull = False
+        propkey_to_col = mapper._propkey_to_col[table]
 
-        for col in mapper._cols_by_table[table]:
-            if col is mapper.version_id_col:
-                params[col._label] = \
-                    mapper._get_committed_state_attr_by_column(
-                        row_switch or state,
-                        row_switch and row_switch.dict
-                        or state_dict,
-                        col)
+        for propkey in set(propkey_to_col).intersection(state.committed_state):
+            value = state_dict[propkey]
+            col = propkey_to_col[propkey]
 
-                prop = mapper._columntoproperty[col]
-                history = state.manager[prop.key].impl.get_history(
-                    state, state_dict, attributes.PASSIVE_NO_INITIALIZE
-                )
-                if history.added:
-                    params[col.key] = history.added[0]
-                    hasdata = True
+            if not state.manager[propkey].impl.is_equal(
+                    value, state.committed_state[propkey]):
+                if isinstance(value, sql.ClauseElement):
+                    value_params[col] = value
                 else:
-                    if mapper.version_id_generator is not False:
-                        val = mapper.version_id_generator(params[col._label])
-                        params[col.key] = val
-
-                        # HACK: check for history, in case the
-                        # history is only
-                        # in a different table than the one
-                        # where the version_id_col is.
-                        for prop in mapper._columntoproperty.values():
-                            history = (
-                                state.manager[prop.key].impl.get_history(
-                                    state, state_dict,
-                                    attributes.PASSIVE_NO_INITIALIZE))
-                            if history.added:
-                                hasdata = True
+                    params[col.key] = value
+
+        if mapper.version_id_col is not None:
+            col = mapper.version_id_col
+            params[col._label] = \
+                mapper._get_committed_state_attr_by_column(
+                    row_switch if row_switch else state,
+                    row_switch.dict if row_switch else state_dict,
+                    col)
+
+            if col.key not in params and \
+                    mapper.version_id_generator is not False:
+                val = mapper.version_id_generator(params[col._label])
+                params[col.key] = val
+
+        if not (params or value_params):
+            continue
+
+        pk_params = {}
+        for col in pks:
+            propkey = mapper._columntoproperty[col].key
+            history = state.manager[propkey].impl.get_history(
+                state, state_dict, attributes.PASSIVE_OFF)
+
+            if row_switch and not history.deleted and history.added:
+                # row switch present.  convert a row that thought
+                # it would be an INSERT into an UPDATE, by removing
+                # the PK value from the SET clause and instead putting
+                # it in the WHERE clause.
+                del params[col.key]
+                pk_params[col._label] = history.added[0]
+            elif history.added:
+                # we're updating the PK value.
+                assert history.deleted, (
+                    "New PK value without an old one not "
+                    "possible for an UPDATE")
+                # check if an UPDATE of the PK value
+                # has already occurred as a result of ON UPDATE CASCADE.
+                # If so, use the new value to locate the row.
+                if ("pk_cascaded", state, col) in uowtransaction.attributes:
+                    pk_params[col._label] = history.added[0]
+                else:
+                    # else, use the old value to locate the row
+                    pk_params[col._label] = history.deleted[0]
             else:
-                prop = mapper._columntoproperty[col]
-                history = state.manager[prop.key].impl.get_history(
-                    state, state_dict,
-                    attributes.PASSIVE_OFF if col in pks else
-                    attributes.PASSIVE_NO_INITIALIZE)
-                if history.added:
-                    if isinstance(history.added[0],
-                                  sql.ClauseElement):
-                        value_params[col] = history.added[0]
-                    else:
-                        value = history.added[0]
-                        params[col.key] = value
-
-                    if col in pks:
-                        if history.deleted and \
-                                not row_switch:
-                            # if passive_updates and sync detected
-                            # this was a  pk->pk sync, use the new
-                            # value to locate the row, since the
-                            # DB would already have set this
-                            if ("pk_cascaded", state, col) in \
-                                    uowtransaction.attributes:
-                                value = history.added[0]
-                                params[col._label] = value
-                            else:
-                                # use the old value to
-                                # locate the row
-                                value = history.deleted[0]
-                                params[col._label] = value
-                            hasdata = True
-                        else:
-                            # row switch logic can reach us here
-                            # remove the pk from the update params
-                            # so the update doesn't
-                            # attempt to include the pk in the
-                            # update statement
-                            del params[col.key]
-                            value = history.added[0]
-                            params[col._label] = value
-                        if value is None:
-                            hasnull = True
-                    else:
-                        hasdata = True
-                elif col in pks:
-                    value = history.unchanged[0]
-                    if value is None:
-                        hasnull = True
-                    params[col._label] = value
+                pk_params[col._label] = history.unchanged[0]
 
-        if hasdata:
-            if hasnull:
+        if params or value_params:
+            if None in pk_params.values():
                 raise orm_exc.FlushError(
-                    "Can't update table "
-                    "using NULL for primary "
+                    "Can't update table using NULL for primary "
                     "key value")
+            params.update(pk_params)
             update.append((state, state_dict, params, mapper,
                            connection, value_params))
     return update
diff --git a/test/orm/test_dynamic.py b/test/orm/test_dynamic.py
index bc47ba3f3..950ff1953 100644
--- a/test/orm/test_dynamic.py
+++ b/test/orm/test_dynamic.py
@@ -510,10 +510,6 @@ class UOWTest(
             testing.db,
             sess.flush,
             CompiledSQL(
-                "SELECT users.id AS users_id, users.name AS users_name "
-                "FROM users WHERE users.id = :param_1",
-                lambda ctx: [{"param_1": u1_id}]),
-            CompiledSQL(
                 "SELECT addresses.id AS addresses_id, addresses.email_address "
                 "AS addresses_email_address FROM addresses "
                 "WHERE addresses.id = :param_1",
@@ -523,7 +519,11 @@ class UOWTest(
                 "UPDATE addresses SET user_id=:user_id WHERE addresses.id = "
                 ":addresses_id",
                 lambda ctx: [{'addresses_id': a2_id, 'user_id': None}]
-            )
+            ),
+            CompiledSQL(
+                "SELECT users.id AS users_id, users.name AS users_name "
+                "FROM users WHERE users.id = :param_1",
+                lambda ctx: [{"param_1": u1_id}]),
         )
 
     def test_rollback(self):
diff --git a/test/orm/test_unitofworkv2.py b/test/orm/test_unitofworkv2.py
index 122fe2514..374a77237 100644
--- a/test/orm/test_unitofworkv2.py
+++ b/test/orm/test_unitofworkv2.py
@@ -1277,6 +1277,8 @@ class RowswitchAccountingTest(fixtures.MappedTest):
         old = attributes.get_history(p3, 'child')[2][0]
         assert old in sess
 
+        # essentially no SQL should emit here,
+        # because we've replaced the row with another identical one
         sess.flush()
 
         assert p3.child._sa_instance_state.session_id == sess.hash_key