Skip to content

query() is documented as read-only and transaction-free, but it holds a transaction for the whole block and lets writes commit #56

Closed

Description

@AlexeyShalaev

Versions: sqlalchemy-foundation-kit 0.4.0 (same code on master, adcd645), SQLAlchemy 2.0.54, asyncpg 0.31.0, Python 3.13.12, PostgreSQL 17

AsyncSQLAlchemyUnitOfWork.query() is documented as "a read-only query context without transaction management. This method is designed for read-only operations and does not start a transaction or perform any commit/rollback" (uow/sqlalchemy.py:440-443). The same wording appears in the class docstring at 234 and in uow/protocols.py:59, 97-98. The guides build on it:

  • docs/agents.md:64-65 says query() "starts no transaction of its own".
  • Rule 5 (docs/agents.md:377-379) says "It starts no transaction and commits nothing".
  • Rule 9 (docs/agents.md:399-401) says an advisory lock taken "in query() is a no-op with a True return".
  • docs/guide/advanced.md:96-101 says it is "For read-only operations without transaction overhead".

Measured, the block is a transaction from its first statement until it exits. SQLAlchemy autobegins, so between statements the connection sits idle in transaction. Locks scoped to the transaction (the AccessShareLock on every table read, and advisory xact locks) are held until the block exits. Nothing makes the block read-only: a write followed by qx.session.commit() is persisted. The Note at 461-463 admits the last point, but the summary above it says otherwise.

Reproduction

import asyncio

from sqlalchemy import text
from sqlalchemy.exc import DBAPIError
from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine
from sqlalchemy_foundation_kit import AsyncSQLAlchemyUnitOfWork, AsyncSQLAlchemyUowTransaction
from testcontainers.postgres import PostgresContainer


async def main(url: str) -> None:
    engine = create_async_engine(url)
    sessions = async_sessionmaker(engine, expire_on_commit=False)
    uow = AsyncSQLAlchemyUnitOfWork(sessions, AsyncSQLAlchemyUowTransaction)
    async with engine.begin() as conn:
        await conn.execute(text("CREATE TABLE notes (body text)"))

    # 1. "does not start a transaction"
    async with uow.query() as qx:
        pid = await qx.session.scalar(text("SELECT pg_backend_pid()"))
        await qx.session.execute(text("SELECT count(*) FROM notes"))
        same_now = await qx.session.scalar(text("SELECT now() = statement_timestamp()"))
        async with sessions() as observer:
            state = await observer.scalar(
                text("SELECT state FROM pg_stat_activity WHERE pid = :pid"), {"pid": pid}
            )
            locks = (await observer.execute(text(
                "SELECT mode FROM pg_locks WHERE pid = :pid AND relation = 'notes'::regclass"
            ), {"pid": pid})).scalars().all()
    print(f"1. inside query(): server state = {state!r}, now() = statement_timestamp(): {same_now}, "
          f"locks still held on notes: {locks}")

    # 2. a transaction-scoped advisory lock taken in query() is held until the block exits
    async with uow.query() as qx:
        mine = await qx.session.scalar(text("SELECT pg_try_advisory_xact_lock(42)"))
        async with sessions() as other:
            theirs = await other.scalar(text("SELECT pg_try_advisory_xact_lock(42)"))
    print(f"2. advisory lock in query(): acquired={mine}; another session's attempt meanwhile={theirs}")

    # 3. "read-only": an INSERT plus an explicit commit is persisted
    async with uow.query() as qx:
        await qx.session.execute(text("INSERT INTO notes VALUES ('written inside query()')"))
        await qx.session.commit()
    async with sessions() as s:
        rows = await s.scalar(text("SELECT count(*) FROM notes"))
    print(f"3. rows after INSERT + commit inside query(): {rows}")
    await engine.dispose()

    # 4. the proposal: begin the block's transaction READ ONLY (one-connection pool to see the reset)
    single_engine = create_async_engine(url, pool_size=1, max_overflow=0)
    single = async_sessionmaker(single_engine)
    async with single() as s:
        await s.connection(execution_options={"postgresql_readonly": True})
        pid = await s.scalar(text("SELECT pg_backend_pid()"))
        read_only = await s.scalar(text("SHOW transaction_read_only"))
        try:
            await s.execute(text("INSERT INTO notes VALUES ('x')"))
            outcome = "accepted"
        except DBAPIError as exc:
            outcome = f"rejected, SQLSTATE {exc.orig.sqlstate}"
    async with single() as s:
        next_pid = await s.scalar(text("SELECT pg_backend_pid()"))
        next_read_only = await s.scalar(text("SHOW transaction_read_only"))
    print(f"4. postgresql_readonly: transaction_read_only={read_only}, INSERT {outcome}; "
          f"next session, same connection={pid == next_pid}: transaction_read_only={next_read_only}")

    # 5. the same after the transaction has already run a statement (e.g. from an open_session() override)
    async with single() as s:
        await s.execute(text("SELECT 1"))
        await s.execute(text("SET TRANSACTION READ ONLY"))
        try:
            await s.execute(text("INSERT INTO notes VALUES ('x')"))
            outcome = "accepted"
        except DBAPIError as exc:
            outcome = f"rejected, SQLSTATE {exc.orig.sqlstate}"
    print(f"5. SET TRANSACTION READ ONLY after a first statement: INSERT {outcome}")
    await single_engine.dispose()


with PostgresContainer("postgres:17-alpine", driver="asyncpg") as pg:
    asyncio.run(main(pg.get_connection_url()))

Actual

1. inside query(): server state = 'idle in transaction', now() = statement_timestamp(): False, locks still held on notes: ['AccessShareLock']
2. advisory lock in query(): acquired=True; another session's attempt meanwhile=False
3. rows after INSERT + commit inside query(): 1
4. postgresql_readonly: transaction_read_only=on, INSERT rejected, SQLSTATE 25006; next session, same connection=True: transaction_read_only=off
5. SET TRANSACTION READ ONLY after a first statement: INSERT rejected, SQLSTATE 25006

Lines 1-3 contradict the docstring and the guides. Line 2 contradicts rule 9: the lock is not a no-op, it is held, and blocks other sessions, until the block exits. Lines 4-5 show the proposed fix working.

Why

query() (uow/sqlalchemy.py:465-468) yields the session from open_session() and leaves it alone. "No transaction" is true only of the unit of work, which calls neither begin() nor commit(). A SQLAlchemy 2.0 session autobegins on its first statement, the asyncpg dialect sends BEGIN with that statement, and closing the session rolls the transaction back. In practice:

  • A slow await inside query() keeps the connection idle in transaction. It counts against idle_in_transaction_session_timeout and keeps AccessShareLocks, which make any ALTER TABLE on those tables wait, and everything queued behind it.
  • now() is frozen at the first statement.
  • Behind a transaction-mode PgBouncer, the block holds one server connection from its first statement to exit, exactly as transaction() does.
  • A repository that writes and commits inside query() works, so "read-only" depends entirely on the caller.

Proposal

  1. Make query() actually read-only. When the session has not checked out a connection yet, pass execution_options={"postgresql_readonly": True} to session.connection(), in the same call that applies the isolation level, because the options apply only when the connection is first procured. asyncpg then opens the transaction READ ONLY with no extra round trip, writes fail with 25006, and the flag is reset when the connection returns to the pool (line 4). If an open_session() override has already run a statement (the TenantUnitOfWork example in its docstring does), issue SET TRANSACTION READ ONLY instead. PostgreSQL accepts it mid-transaction (line 5). This changes behaviour for anyone who writes through query(), so it deserves a changelog line, but turning those writes into errors is the point.
  2. Whether or not (1) lands, fix the wording: query() runs in a single transaction that is rolled back when the block exits, holds its locks until then, and does not commit. That covers uow/sqlalchemy.py:234, 440-443, 459, uow/protocols.py:59, 98, docs/agents.md:64-65, 226 (the "none started" cell), 377-379, 488, and docs/guide/advanced.md:96-101. Rule 9 (docs/agents.md:399-401) should say the lock is held for the whole query() block and released at exit, not that it is a no-op.

Activity

  1. AlexeyShalaev commented on Oct 16, 2026

    @AlexeyShalaev
    MemberAuthor

    Reproduced on master: all five lines match.

    I'm going with your proposal 1 plus the wording fix:

    • query() makes its transaction READ ONLY. If the session hasn't checked out a connection yet, that happens through session.connection(execution_options={"postgresql_readonly": True}), so asyncpg opens the transaction READ ONLY with no extra round trip, and the dialect resets the flag on checkin. If open_session() has already checked one out (an isolation level was asked for, or an override ran a statement), it issues SET TRANSACTION READ ONLY, which PostgreSQL accepts at that point. I checked that pg_try_advisory_xact_lock, set_config(..., true) (the search_path listener) and SET LOCAL still work in a READ ONLY transaction, and that nextval() doesn't (25006). That last one goes in the migration note.
    • The docstrings (uow/sqlalchemy.py, uow/protocols.py) and the guides say what the block is: one read-only transaction that is rolled back on exit and holds its locks and its snapshot until then. Rule 9 gets fixed: an advisory xact lock taken in query() is held for the whole block.

    The alternative was docs only. It keeps every caller working, but "read-only" would still depend on the caller, and what the reporter wanted was a guarantee. This breaks code that writes through query(), so the PR will be fix!: with a BREAKING CHANGE note. The way out is transaction().

  2. AlexeyShalaev commented on Oct 16, 2026

    @AlexeyShalaev
    MemberAuthor

    Done in #64, as planned: query() now opens its transaction READ ONLY, so your step 3 raises ReadOnlySQLTransactionError (25006) instead of persisting, and the docs describe it as one read-only transaction that holds its locks until exit. Rule 9 is corrected.

    This is marked breaking (fix!:): any write through query(), nextval() included, has to move to transaction().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    documentationImprovements or additions to documentationenhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions