Skip to content

When using psycopg2 with use_batch_mode=True, false delete warnings are issued #4661

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lauramroa opened this issue May 3, 2019 · 9 comments
Labels
bug Something isn't working orm postgresql
Milestone

Comments

@lauramroa
Copy link

I'm using psycopg2 with use_batch_mode=True. I have a parent child relationship with two children. If I delete the parent, SQL Alchemy gives me the following warning:

SAWarning: DELETE statement on table 'child' expected to delete 2 row(s); 1 were matched.  Please set confirm_deleted_rows=False within the mapper configuration to prevent this warning.

This does not happen if I don't use batch mode.

Drilling down, the warning is issued in sqlalchemy/orm/persistence.py around line 1300 (on current master). The problem is that the delete query on line 1329 reports an incorrect row count. Even though the query deletes two rows, the rowcount is 1.

The following is a script that reproduces this error. It reproduces both the false warning and the problem whereby rowcount is incorrect:

from sqlalchemy import Column, Integer, ForeignKey, create_engine, String
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.orm import scoped_session, sessionmaker, relationship

Base = declarative_base()


class Parent(Base):
    __tablename__ = "parent"

    pk = Column(Integer, primary_key=True)

    children = relationship(
        "Child", back_populates="parent", cascade="all, delete-orphan"
    )


class Child(Base):
    __tablename__ = "child"

    pk = Column(Integer, primary_key=True)
    parent_pk = Column(Integer, ForeignKey("parent.pk"), nullable=False)

    parent = relationship("Parent", back_populates="children")


# Setup the engine + session + tables
engine = create_engine("postgresql:///transiter_test_db_2", use_batch_mode=True)
session_factory = sessionmaker(bind=engine, autoflush=False)
Session = scoped_session(session_factory)
session = Session()
Base.metadata.drop_all(engine)
Base.metadata.create_all(engine)


def insert_parent_and_two_children():
    # Put one parent with two children
    parent = Parent()
    child_1 = Child()
    child_2 = Child()
    parent.children = [child_1, child_2]
    session.merge(parent)
    session.commit()
    print("Number of children: ", len(session.query(Child).all()))


def delete_parent_using_orm():
    for parent in session.query(Parent):
        session.delete(parent)
    session.commit()
    print("Number of children: ", len(session.query(Child).all()))


def delete_children_using_native_query():
    sql = "DELETE FROM child WHERE pk =:pk"
    # Note, following the script then 3 and 4 are the PKs created
    result = session.execute(sql, [{"pk": 3}, {"pk": 4}])
    session.commit()
    print("Number of children deleted: ", result.rowcount)
    print("Number of children: ", len(session.query(Child).all()))


insert_parent_and_two_children()
delete_parent_using_orm() # note the warning

print('----')

insert_parent_and_two_children()
delete_children_using_native_query() # note the mismatching numbers
@lauramroa
Copy link
Author

Just to make it clear: in all cases the correct number of rows are deleted. This is merely an issue with the reported rowcount.

@zzzeek zzzeek added bug Something isn't working orm postgresql labels May 3, 2019
@zzzeek zzzeek added this to the 1.3.xx milestone May 3, 2019
@zzzeek
Copy link
Member

zzzeek commented May 3, 2019

the rowcount should not be consulted if the dialect reports supports_sane_multi_rowcount=False which is the case for use_batch_mode=True so this a bit of a mystery.

@sqla-tester
Copy link
Collaborator

Mike Bayer has proposed a fix for this issue in the master branch:

Don't warn on multi delete rowcount if supports_sane_multi is False https://gerrit.sqlalchemy.org/1249

@sqla-tester
Copy link
Collaborator

Mike Bayer has proposed a fix for this issue in the rel_1_3 branch:

Don't warn on multi delete rowcount if supports_sane_multi is False https://gerrit.sqlalchemy.org/1250

@zzzeek
Copy link
Member

zzzeek commented May 3, 2019

we were just warning in the ORM. old bug

@jamespfennell
Copy link

Accidentally filed this under my girlfriend's Github account. Awesome! Thanks for the fix.

sqlalchemy-bot pushed a commit that referenced this issue May 7, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Fixed an issue where the "number of rows matched" warning would emit even if
the dialect reported "supports_sane_multi_rowcount=False", as is the case
for psycogp2 with ``use_batch_mode=True`` and others.

Fixes: #4661
Change-Id: I93aaf7f597b6083e860ab3cbcd620ba5621c57a8
(cherry picked from commit aa94afc)
@zzzeek
Copy link
Member

zzzeek commented May 11, 2019

the fix for this is wrong, tests fail for a DB that doesnt have sane multi row count. additionally, the dialect should be supporting -1 for rowcount if this is not supported.

@zzzeek zzzeek reopened this May 11, 2019
@sqla-tester
Copy link
Collaborator

Mike Bayer has proposed a fix for this issue in the master branch:

Correct fix and tests for #4661 https://gerrit.sqlalchemy.org/1262

@sqla-tester
Copy link
Collaborator

Mike Bayer has proposed a fix for this issue in the rel_1_3 branch:

Correct fix and tests for #4661 https://gerrit.sqlalchemy.org/1263

sqlalchemy-bot pushed a commit that referenced this issue May 11, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
For #4661 we need to still warn if we are only deleting one row,
even if sane multi rowcount is false.   Tests were failing for
pyodbc since the warning was removed for the single-row case.
the UPDATE logic raises if a single row doesn't match even
if sane multi rowcount is false, so this is now more consistent
with that.  Add tests for the UPDATE case also.  It is possible
there are already tests for this but as the DELETE case wasn't
well covered it's not clear.

Fixes: #4661
Change-Id: Ie57f765ff31bf806206837c5fbfe449b02ebf4be
(cherry picked from commit bdafff8)
@zzzeek zzzeek modified the milestones: 1.3.xx, 1.3.x Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working orm postgresql
Projects
None yet
Development

No branches or pull requests

4 participants