Triple nested custom types cause binary dumper to use wrong dumper for nested objects

[exrok]

Hello, again :). With binary loading fixed in #545 🎉, next up is... binary dumping.
(aside: I suspect since the binary protocol isn't default, it just doesn't get tested as much.)

Observations

I have only observed this issue with the binary protocol, text protocol seems fine.

When serializing something of the form: Composite( Array[ Composite( Enum ) ] ),
The stack trace shows

• Composite Dumper abc.CollectionBinaryDumper
• Array Dumper abc.ItemListBinaryDumper
• Composite Dumper abc.ItemBinaryDumper
• Array Dumper abc.ItemListBinaryDumper (Expected Enum Dumper)

And the following error is thrown due to the enum, Kind, being passed to the array dumper.

File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/types/array.py", line 270, in dump_list
if len(L) != dims[dim]:
TypeError: object of type 'Kind' has no len()

Note: this can occur without an array.

In a custom BinaryDumper, adding self._tx.set_dumper_types(self.info.field_types, self.format) to
the top of the dump method seemed to workaround the problem.
However, the fix was only intermittent, sometimes still failing.

Env

• Python: v3.10
• OS: Arch Linux
• DB: Postgres 15.2
• psycopg 3.1.8 both with and without binary extra
• Don't know if it is used but I have libpq.so.5.15 on my system.

Reproduction

SQL to initialize the state.

DROP SCHEMA IF EXISTS test CASCADE;
CREATE SCHEMA IF NOT EXISTS test;

CREATE TYPE test.kind AS ENUM('A', 'B');

CREATE TYPE test.item AS (kind test.Kind, time timestamptz);

CREATE TYPE test.collection AS (items test.item[]);

Python to trigger the problem.

import psycopg
from enum import Enum, auto
from datetime import datetime, timezone
from psycopg.types.enum import EnumInfo, register_enum
from psycopg.types.composite import CompositeInfo, register_composite


class Kind(Enum):
    A = auto()
    B = auto()


with psycopg.connect("host=localhost user=postgres password=postgres") as conn:
    info = CompositeInfo.fetch(conn, "test.collection")
    register_composite(info, conn)
    Collection = info.python_type

    info = CompositeInfo.fetch(conn, "test.item")
    register_composite(info, conn)
    Item = info.python_type

    info = EnumInfo.fetch(conn, "test.kind")
    register_enum(info, conn, Kind)

    # Successful: Array of Items
    print(
        conn.execute(
            "SELECT %b",
            [[Item(kind=Kind.A, time=datetime.now(tz=timezone.utc))]],
        ).fetchall()
    )

    # Successful: Collection with empty array
    print(
        conn.execute(
            "SELECT %b",
            [Collection(items=[])],
        ).fetchall()
    )

    # Failure: Collection with non-empty array
    print(
        conn.execute(
            "SELECT %b",
            [Collection(items=[Item(kind=Kind.A, time=datetime.now(tz=timezone.utc))])],
        ).fetchall()
    )

Error Traceback Logs

Traceback when using psycopg[binary,pool] version 3.1.8

Traceback (most recent call last):
  File "/home/user/bugs/python/src/repro.py", line 43, in <module>
    conn.execute(
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/connection.py", line 876, in execute
    return cur.execute(query, params, prepare=prepare)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/cursor.py", line 719, in execute
    self._conn.wait(
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/connection.py", line 957, in wait
    return waiting.wait(gen, self.pgconn.socket, timeout=timeout)
  File "psycopg_binary/_psycopg/waiting.pyx", line 172, in psycopg_binary._psycopg.wait_c
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/cursor.py", line 195, in _execute_gen
    pgq = self._convert_query(query, params)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/cursor.py", line 468, in _convert_query
    pgq.convert(query, params)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/_queries.py", line 80, in convert
    self.dump(vars)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/_queries.py", line 91, in dump
    self.params = self._tx.dump_sequence(params, self._want_formats)
  File "psycopg_binary/_psycopg/transform.pyx", line 353, in psycopg_binary._psycopg.Transformer.dump_sequence
  File "psycopg_binary/_psycopg/transform.pyx", line 404, in psycopg_binary._psycopg.Transformer.dump_sequence
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/types/composite.py", line 85, in dump
    adapted = self._tx.dump_sequence(obj, self._formats)
  File "psycopg_binary/_psycopg/transform.pyx", line 353, in psycopg_binary._psycopg.Transformer.dump_sequence
  File "psycopg_binary/_psycopg/transform.pyx", line 379, in psycopg_binary._psycopg.Transformer.dump_sequence
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/types/array.py", line 289, in dump
    dump_list(obj, 0)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/types/array.py", line 277, in dump_list
    ad = self.sub_dumper.dump(item)  # type: ignore[union-attr]
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/types/composite.py", line 85, in dump
    adapted = self._tx.dump_sequence(obj, self._formats)
  File "psycopg_binary/_psycopg/transform.pyx", line 353, in psycopg_binary._psycopg.Transformer.dump_sequence
  File "psycopg_binary/_psycopg/transform.pyx", line 379, in psycopg_binary._psycopg.Transformer.dump_sequence
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/types/array.py", line 289, in dump
    dump_list(obj, 0)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/types/array.py", line 270, in dump_list
    if len(L) != dims[dim]:
TypeError: object of type 'Kind' has no len()

Traceback when using only psycopg[pool] version 3.1.8

Traceback (most recent call last):
  File "/home/user/bugs/python/src/repro.py", line 43, in <module>
    conn.execute(
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/connection.py", line 876, in execute
    return cur.execute(query, params, prepare=prepare)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/cursor.py", line 719, in execute
    self._conn.wait(
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/connection.py", line 957, in wait
    return waiting.wait(gen, self.pgconn.socket, timeout=timeout)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/waiting.py", line 214, in wait_select
    s = next(gen)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/cursor.py", line 195, in _execute_gen
    pgq = self._convert_query(query, params)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/cursor.py", line 468, in _convert_query
    pgq.convert(query, params)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/_queries.py", line 80, in convert
    self.dump(vars)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/_queries.py", line 91, in dump
    self.params = self._tx.dump_sequence(params, self._want_formats)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/_transform.py", line 188, in dump_sequence
    out[i] = dumper.dump(param)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/types/composite.py", line 85, in dump
    adapted = self._tx.dump_sequence(obj, self._formats)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/_transform.py", line 177, in dump_sequence
    out[i] = self._row_dumpers[i].dump(param)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/types/array.py", line 289, in dump
    dump_list(obj, 0)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/types/array.py", line 277, in dump_list
    ad = self.sub_dumper.dump(item)  # type: ignore[union-attr]
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/types/composite.py", line 85, in dump
    adapted = self._tx.dump_sequence(obj, self._formats)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/_transform.py", line 177, in dump_sequence
    out[i] = self._row_dumpers[i].dump(param)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/types/array.py", line 289, in dump
    dump_list(obj, 0)
  File "/home/user/bugs/python/.venv/lib/python3.10/site-packages/psycopg/types/array.py", line 270, in dump_list
    if len(L) != dims[dim]:
TypeError: object of type 'Kind' has no len()

[dvarrazzo]

Probably there is an error similar to the load side, where we reuse a dumper instead of creating different dumpers for different types.

[exrok]

There are similarities to the problem on the load side but I think that
at least one of the core problems is different here.

My break down of the problem

In TupleBinaryDumper, self._tx.set_dumper_types is called.

class TupleBinaryDumper(RecursiveDumper):
    ...

    def __init__(self, cls: type, context: Optional[abc.AdaptContext] = None):
        ...
        self._tx.set_dumper_types(self.info.field_types, self.format)
        ...

Then in set_dumper_types,

 def set_dumper_types(self, types: Sequence[int], format: pq.Format) -> None:
        self._row_dumpers = [self.get_dumper_by_oid(oid, format) for oid in types]
        self.types = tuple(types)
        self.formats = [format] * len(types)

we overwrite _row_dumpers on this shared transformer for this query.

This overwriting is a core problem. Consider the following

CREATE TYPE child as (leaf int)
CREATE TYPE parent as (node child)

import psycopg
from psycopg._transformer import Transformer
from psycopg._enums import PyFormat
from psycopg.types.composite import CompositeInfo, register_composite

with psycopg.connect("host=localhost user=postgres password=postgres") as conn:
    info = CompositeInfo.fetch(conn, "child")
    register_composite(info, conn)
    Child = info.python_type

    info = CompositeInfo.fetch(conn, "parent")
    register_composite(info, conn)
    Parent = info.python_type

    trans = Transformer.from_context(conn)
    trans.dump_sequence([Parent(node=Child(leaf=1))], [PyFormat.BINARY])

ParentBinaryDumper will get initialized via self.get_dumper in dump_sequence.
During initializing of ParentBinaryDumper, set_dumper_types is called.

Since Parent contains a field of type Child, it was cause ChildBinaryDumper
to be initialized via, get_dumper_by_oid during the set_dumper_types call
from ParentBinaryDumper::__init__.

# in class Transformer @ _py_transformer.py
 def set_dumper_types(self, types: Sequence[int], format: pq.Format) -> None:
        self._row_dumpers = [self.get_dumper_by_oid(oid, format) for oid in types]
        self.types = tuple(types)
        self.formats = [format] * len(types)

During the initialization of ChildBinaryDumper, set_dumper_types gets called. Which
configures the self._row_dumpers for the single int field it contains.

However, self._row_dumpers, self.types and self.formats then just get overwritten,
as the original call to set_dumper_types from ParentBinaryDumper finishes.

Note: that if the ChildBinaryDumper's set_dumper_types state where preserved here that
would also be a bug because then the fields of Parent would be dumped as if they where
the child fields.

More generally, when a BinaryEnumTuple dumper is initialized all BinaryEnumTuple
dumpers in fields are initialized recursively. Each dumper calls set_dumper_types during
initializes but mutation to the Transformer gets overwritten by the initial dumper.

So far, in my above break down the bug doesn't seem much like the previous loader one.
However, since we cache the dumpers their constructor is executed but once.
And so, the cache helps manifest the above issue.

Towards a Solution

First, I think doing order dependent mutation of the shared Transformer in the __init__
method of dumpers is a bit confusing and error prone.

Instead, we could pass _row_dumpers or the list of oids straight to dump_sequence,
at least for the composite tuple case.

For instance, the following 4 changes fixes the reproduction cases of the problem

Prototype of solution (as least for non-c version)

#composite.py
class TupleBinaryDumper(RecursiveDumper):
    format = pq.Format.BINARY

    # Subclasses must set an info
    info: CompositeInfo

    def __init__(self, cls: type, context: Optional[abc.AdaptContext] = None):
        super().__init__(cls, context)
        nfields = len(self.info.field_types)
        # self._tx.set_dumper_types(self.info.field_types, self.format) <-- Change 1: removed
        self._formats = (PyFormat.from_pq(self.format),) * nfields

    def dump(self, obj: Tuple[Any, ...]) -> bytearray:
        out = bytearray(pack_len(len(obj)))
        adapted = self._tx.dump_sequence(
            obj, self._formats, types=self.info.field_types  # <-- Change 2: Added types
        )
        for i in range(len(obj)):
            b = adapted[i]
            oid = self.info.field_types[i]
            if b is not None:
                out += _pack_oidlen(oid, len(b))
                out += b
            else:
                out += _pack_oidlen(oid, -1)

        return out

class Transformer(AdaptContext):
    def dump_sequence(
        self,
        params: Sequence[Any],
        formats: Sequence[PyFormat],
        types: Sequence[int] | None = None, # <-- Change 3: Added types
    ) -> Sequence[Optional[Buffer]]:
        nparams = len(params)
        out: List[Optional[Buffer]] = [None] * nparams

        if types is not None: # <-- Change 4: Use type oids if provided
            for i, oid in enumerate(types):
                param = params[i]
                if param is not None:
                    out[i] = self.get_dumper_by_oid(oid, pq.Format.BINARY).dump(param)
            return out

        ... # same as before

Edit:

We probably, want to avoid dumper_by_oid as used in above prototype? We could have multiple dumpers that dump by different python types to the same oid.

Should prioritize by python type dumper lookup and assert oid matches. If no specific dumper is found then look up by oid.

[dvarrazzo]

The fix seems simple:

--- a/psycopg/psycopg/adapt.py
+++ b/psycopg/psycopg/adapt.py
@@ -151,7 +151,7 @@ class RecursiveDumper(Dumper):
 
     def __init__(self, cls: type, context: Optional[abc.AdaptContext] = None):
         super().__init__(cls, context)
-        self._tx = Transformer.from_context(context)
+        self._tx = Transformer(context)
 
 
 class RecursiveLoader(Loader):

the problem here is that the transformer shouldn't be shared between the two composite dumpers. It was shared in the interest of reusing the Transformer caches (instantiated dumpers and loaders), but clearly we cannot do that if two dumpers both require to use dump_sequence().

I will need to do more tests to check if the other cases where Transformer.from_context() is used are sane.

Do things work for you with this change? I seem to understand that you are putting together quite a complex system with postgres types and I'm interested in your feedback.

[dvarrazzo]

I have made a more isolated change (fixing the composite dumper, rather than every recursive dumper).

Test is welcome, thank you!

[exrok]

Your changes seems to have fixed the issue nicely. :)
Using the binary dumpers previously caused 26 unit tests to fail in our repo.
When using the fix-recursive-composite-dumper branch, all tests pass for both
text and binary dumpers 🎉.

Some points on our use case.

• In some cases we are treating PostgreSQL as a typed document database.

• We are use domain driven design where we have aggregate roots containing
  1-to-1 relationships to value objects.

• We are modeling the aggregate root as a table containing the value objects
  as composite types in a way that closely matches the structure in python.

• Using adapters to directly map our domain objects in python to/from postgres
  has been quite ergonomic so far.

• This is the first time I am using Postgres in earnest. I have always used
  MariaDB for my RDBS needs in the past. I am almost certainly overusing
  composite types at the moment.

• We are using the PostGIS extension and shapely in python to do geo-spatial computation.
  Some of the geometry data we work with is quite large, so transferring it as a binary
  representation directly between the GEOS powered shapely and the GEOS in PostGIS
  is appealing. Previously, we where using Dgraph and had to convert to/from GeoJSON
  for transmission.

• For performance critical components we use Rust, but we still don't want our python
  to be too slow.

• The most complex type has the same structure described in the initial issue description.
  The collection has more fields, and collection as a whole is optional, and so it is nice
  to bundle into a composite to match the python structure.

  Further, collection itself is rarely mutated and the number of items is quite small
  usually 3-4 and max 15. Rarely, we need to generate statistics from these collections
  modeling them this way made the queries straightforward.

• Instead of mapping by order we generally map by name, for instance.

Dump via field name

class ViaTupleBinaryDumper(TupleBinaryDumper):
    """
    Postgres dumper converting python types to postgres binary format via
    a function mapping from python types to either a tuple of column values
    ordered by definition in table or a dictionary mapping column names to values.
    """

    # Using a tuple to avoid python passing `self` argument
    # TODO: probably should use staticmethod(...) on assignment instead.
    obj_into_columns: tuple[Callable[[Any], TupleType[Any, ...] | dict[str, Any]]]

    def dump(self, obj: Any) -> bytes:
        columns = self.obj_into_columns[0](obj)

        if isinstance(columns, dict):
            columns_in_order = tuple(columns[field] for field in self.info.field_names)
            return super().dump(columns_in_order)
        else:
            return super().dump(columns)

Insert via field name

   conn.execute(
       *pg.insert("schema.table")(
           id=entity.id,
           col1=entity.value,
           col2="hello",
           col3=pg.Tuple("thing", 1),
       )
   )

[dvarrazzo]

Thank you for your feedback, @exrok

Maybe you are overusing Composite, yes :) You might find problems building indexes for instance, so it's something you might be thinking about. Consider that every table also duplicates as a composite type (if you have a table called tbl you can say select tbl from tbl where ... and you get a record whose oid is the one of the type tbl and which you can fetch using the same composite objects). Dually, you can have normal tables and use a row factory to obtain a Python type from several columns.

Thank you for your use case!