Error with multirow updates + eager_defaults set to true

[Pep Ribas Prats]

Updating multiple rows fails when eager_default is set to true with psycopg2.

sqlalchemy.exc.ProgrammingError: (psycopg2.ProgrammingError) no results to fetch

Here you have an example to reproduce it.

from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import declarative_base

Base = declarative_base()


class A(Base):
    __tablename__ = 'a'
    id = Column(Integer, primary_key=True)
    value = Column(Integer)

    __mapper_args__ = {'eager_defaults': True}

e = create_engine("postgresql:///test", echo=True)
Base.metadata.create_all(e)

s = Session(e)
a1 = A(value=1)
a2 = A(value=2)
s.add(a1)
s.add(a2)
s.commit()

a1.value = 10
a2.value = 20
s.commit()

[Mike Bayer]

the first issue is in Core and is causing all table columns to be added to RETURNING for no reason, this is probably a separate issue:

diff --git a/lib/sqlalchemy/sql/crud.py b/lib/sqlalchemy/sql/crud.py
index 18b9601..c5495cc 100644
--- a/lib/sqlalchemy/sql/crud.py
+++ b/lib/sqlalchemy/sql/crud.py
@@ -493,6 +493,7 @@ def _append_param_update(
         else:
             compiler.postfetch.append(c)
     elif implicit_return_defaults and \
+            stmt._return_defaults is not True and \
             c in implicit_return_defaults:
         compiler.returning.append(c)

with that fixed, we revise the test to include an actual server_onupdate to trigger it again:

class A(Base):
    __tablename__ = 'a'
    id = Column(Integer, primary_key=True)
    value = Column(Integer)
    foo = Column(Integer, server_onupdate=FetchedValue())

    __mapper_args__ = {'eager_defaults': True}

for that part we need to replicate the "has_all_defaults" feature of inserts in persistence.py for updates

[Mike Bayer]

this patch also includes a potential fix for the insert version of this, overall eager_defaults is lacking test support:

diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index 5ade4b9..95aa14a 100644
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -1970,12 +1970,24 @@ class Mapper(InspectionAttr):
             (
                 table,
                 frozenset([
-                    col for col in columns
+                    col.key for col in columns
                     if col.server_default is not None])
             )
             for table, columns in self._cols_by_table.items()
         )

+    @_memoized_configured_property
+    def _server_onupdate_default_cols(self):
+        return dict(
+            (
+                table,
+                frozenset([
+                    col.key for col in columns
+                    if col.server_onupdate is not None])
+            )
+            for table, columns in self._cols_by_table.items()
+        )
+
     @property
     def selectable(self):
         """The :func:`.select` construct this :class:`.Mapper` selects from
diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py
index 768c114..9b631d2 100644
--- a/lib/sqlalchemy/orm/persistence.py
+++ b/lib/sqlalchemy/orm/persistence.py
@@ -397,7 +397,7 @@ def _collect_insert_commands(

             if mapper.base_mapper.eager_defaults:
                 has_all_defaults = mapper._server_default_cols[table].\
-                    issubset(params)
+                    issubset(set(params).union(value_params))
             else:
                 has_all_defaults = True
         else:
@@ -448,6 +448,7 @@ def _collect_update_commands(
                 set(propkey_to_col).intersection(state_dict).difference(
                     mapper._pk_keys_by_table[table])
             )
+            has_all_defaults = True
         else:
             params = {}
             for propkey in set(propkey_to_col).intersection(
@@ -463,6 +464,12 @@ def _collect_update_commands(
                         value, state.committed_state[propkey]) is not True:
                     params[col.key] = value

+            if mapper.base_mapper.eager_defaults:
+                has_all_defaults = mapper._server_onupdate_default_cols[table].\
+                    issubset(set(params).union(value_params))
+            else:
+                has_all_defaults = True
+
         if update_version_id is not None and \
                 mapper.version_id_col in mapper._cols_by_table[table]:

@@ -529,7 +536,7 @@ def _collect_update_commands(
             params.update(pk_params)
             yield (
                 state, state_dict, params, mapper,
-                connection, value_params)
+                connection, value_params, has_all_defaults)


 def _collect_post_update_commands(base_mapper, uowtransaction, table,
@@ -628,14 +635,16 @@ def _emit_update_statements(base_mapper, uowtransaction,

     statement = base_mapper._memo(('update', table), update_stmt)

-    for (connection, paramkeys, hasvalue), \
+    for (connection, paramkeys, hasvalue, has_all_defaults), \
         records in groupby(
             update,
             lambda rec: (
                 rec[4],  # connection
                 set(rec[2]),  # set of parameter keys
-                bool(rec[5]))):  # whether or not we have "value" parameters
-
+                bool(rec[5]),  # whether or not we have "value" parameters
+                rec[6]  # has_all_defaults
+            )
+    ):
         rows = 0
         records = list(records)

@@ -645,11 +654,11 @@ def _emit_update_statements(base_mapper, uowtransaction,
         assert_singlerow = connection.dialect.supports_sane_rowcount
         assert_multirow = assert_singlerow and \
             connection.dialect.supports_sane_multi_rowcount
-        allow_multirow = not needs_version_id
+        allow_multirow = has_all_defaults and not needs_version_id

         if hasvalue:
             for state, state_dict, params, mapper, \
-                    connection, value_params in records:
+                    connection, value_params, has_all_defaults in records:
                 c = connection.execute(
                     statement.values(value_params),
                     params)
@@ -669,7 +678,7 @@ def _emit_update_statements(base_mapper, uowtransaction,
             if not allow_multirow:
                 check_rowcount = assert_singlerow
                 for state, state_dict, params, mapper, \
-                        connection, value_params in records:
+                        connection, value_params, has_all_defaults in records:
                     c = cached_connections[connection].\
                         execute(statement, params)

@@ -699,7 +708,7 @@ def _emit_update_statements(base_mapper, uowtransaction,
                 rows += c.rowcount

                 for state, state_dict, params, mapper, \
-                        connection, value_params in records:
+                        connection, value_params, has_all_defaults in records:
                     if bookkeeping:
                         _postfetch(
                             mapper,
diff --git a/lib/sqlalchemy/sql/crud.py b/lib/sqlalchemy/sql/crud.py
index 18b9601..c5495cc 100644
--- a/lib/sqlalchemy/sql/crud.py
+++ b/lib/sqlalchemy/sql/crud.py
@@ -493,6 +493,7 @@ def _append_param_update(
         else:
             compiler.postfetch.append(c)
     elif implicit_return_defaults and \
+            stmt._return_defaults is not True and \
             c in implicit_return_defaults:
         compiler.returning.append(c)

[Mike Bayer]

ref #3609 wip

→ <<cset c59ea573e12e>>

[Mike Bayer]

just to confirm, we've just put out 1.0.10 and another 1.0 isn't due for awhile. are you just testing this feature? im considering if this change should only be in 1.1.

[Pep Ribas Prats]

Hi Mike, First of all, thanks for your awesome support.

We are currently using this feature so we would love to have a bugfix release. If you want to wait till 1.1 I can just create our own bugfix release backporting your fix on top of 1.0.10.

[Mike Bayer]

it should be fine for 1.0.11 beause this is such an enormous bug, the fact that nobody's reported it yet means this feature isnt used that widely :).

[Pep Ribas Prats]

that's great! thanks!

[Mike Bayer]

ref #3609 wip

→ <<cset c59ea573e12e>>

[Mike Bayer]

0e4c4d7

a6387b0