Support psycopg as a PostgreSQL driver #18999

clokep · 2025-09-30T19:12:01Z

This adds support for psycopg (aka psycopg3) as a PostgreSQL driver in addition to psycopg2. This requires adding some abstractions between psycopg2 and psycopg although most of the queries can be identical (since it is just PostgreSQL underneath).

This is a branch that @realtyem and myself have been working on for several years.

The bulk of this PR is splitting the PostgresEngine into a PsycopgEngine and Psycopg2Engine which use the correct underlying driver. We then need to update some if-statements in the code to differentiate between these.

A bunch of configuration code also needs to be adjusted to handle having multiple engines for postgres.

This also updates the test matrix for trial to run against psycopg2 and psycopg.

This is step 1 of #14586.

Use the proper isolation level on psycopg, where it is no longer an int.

…opg3

… for importing

…tch() instead of execute_values() execute_values() from psycopg2 can do insertions with fetch=False, but psycopg is better off inserting with execute_batch() instead(because of pipelining support)

…eter $3

clokep · 2025-10-06T20:16:19Z

I think this is ready for a review, I'm sure it'll need a few changes, but overall I think it is sound.

reivilibre

Thanks both of you, looks like a tremendous amount of work has gone into this!

From my perspective, this looks virtually ready, just the really minor things I've pointed out, I'd say:

as said, I would like more eyes on the COPY-based replacement for batch execution. It is probably fine, though.
It looks like some optimisations have been walked back compared to Psycopg2. I personally think this is fine, though it would be ideal to have a tracking issue containing a ticklist of these, so we can track their re-implementation as a prerequisite to considering Psycopg support 'complete'/first-class/ready to replace Psycopg2.

Other than that, it all looks clear and reasonable to me!

reivilibre · 2025-10-08T11:59:28Z

synapse/storage/engines/postgres.py


+    def set_statement_timeout(self, cursor: Cursor, statement_timeout: int) -> None:
+        """Configure the current cursor's statement timeout."""
+        cursor.execute("SET statement_timeout TO ?", (statement_timeout,))


slight preference to using the explicit SET SESSION form. But this is only cosmetic, no semantic change

reivilibre · 2025-10-08T12:03:08Z

synapse/storage/engines/postgres.py

+    Generic[ConnectionType, CursorType, IsolationLevelType],
+    BaseDatabaseEngine[ConnectionType, CursorType, IsolationLevelType],
+    metaclass=abc.ABCMeta,
 ):


a mini docstring pointing out this is (now) an abstract class, with Pyscopg2Engine and PsycopgEngine as concrete implementations, might be nice

reivilibre · 2025-10-08T12:23:15Z

synapse/storage/database.py

+            # We use fetch = False to mean a writable query. You *might* be able
+            # to morph that into a COPY (...) FROM STDIN, but it isn't worth the
+            # effort for the few places we set fetch = False.
+            assert fetch is True
+
+            # execute_values requires a single replacement, but we need to expand it
+            # for COPY. This assumes all inner sequences are the same length.
+            value_str = "(" + ", ".join("?" for _ in next(iter(values))) + ")"
+            sql = sql.replace("?", ", ".join(value_str for _ in values))
+
+            # Wrap the SQL in the COPY statement.
+            sql = f"COPY ({sql}) TO STDOUT"
+
+            def f(
+                the_sql: str, the_args: Sequence[Sequence[Any]]
+            ) -> Iterable[Tuple[Any, ...]]:
+                with self.txn.copy(the_sql, the_args) as copy:
+                    yield from copy.rows()
+
+            # Flatten the values.
+            return self._do_execute(f, sql, list(itertools.chain.from_iterable(values)))


I'm not saying there's anything wrong with this, but this looks non-trivial I'd like to get some extra pair of eyes on this.

Slightly suspicious about next(iter(values)) — would that advance an iterator such as a generator expression, in a way that would drop the first row?

Slightly suspicious about next(iter(values)) — would that advance an iterator such as a generator expression, in a way that would drop the first row?

This exact concern is obsoleted by d49827d (now accepts a Collection, which shouldn't have this risk).

Still think I'd like to take a second look at this aspect with a fresh mind and/or get some extra eyes, but in principle it seems fine

reivilibre · 2025-10-08T12:24:09Z

synapse/storage/database.py

                lambda the_sql: execute_batch(self.txn, the_sql, args), sql
            )
+
+            # TODO Can psycopg3 do anything better?


Just opening this as a thread for discussion to see whether we care enough to either do this now, or open a ticket for it, or something else.

reivilibre · 2025-10-08T12:24:27Z

synapse/storage/database.py


            txn.execute_values(sql, args, fetch=False)

+            # TODO Maybe improve for psycopg.


Just opening this as a thread for discussion to see whether we care enough to either do this now, or open a ticket for it, or something else.

reivilibre · 2025-10-08T12:27:22Z

synapse/storage/database.py

+            if isinstance(sql, psycopg.sql.Composed):
+                return sql.as_string(None)


would appreciate a comment to explain why we handle Composed instances here (seems to go against the method signature?)

But more concretely: do we need to also strip out any potential newlines from the stringification here?

Obsoleted by f5b6429 which takes this out again

reivilibre · 2025-10-08T12:35:12Z

synapse/storage/database.py

        """

-        if isinstance(txn.database_engine, PostgresEngine):
+        if isinstance(txn.database_engine, Psycopg2Engine):


maybe we open a ticket about this as well. A solution might be to pass in an array and rely on UNNEST, but that is something I'm more than happy to defer to later, it just would be nice to remember to do it

reivilibre · 2025-10-08T12:42:25Z

scripts-dev/complement.sh

 else
  export PASS_SYNAPSE_COMPLEMENT_USE_WORKERS=
-  if [[ -n "$POSTGRES" ]]; then
+  if [[ "$POSTGRES" = "psycopg" ]]; then


judging from the CI yaml this needs to be case-insensitive (to match Psycopg from the CI matrix.database); or am I wrong?

clokep and others added 25 commits October 9, 2024 16:31

Add psycopg package.

ecb20a4

Method to set statement timeout.

c7e3120

Separate PostgresEngine into Psycopg2Engine and PsycopgEngine.

91ef287

Run tests in CI against psycopg.

0a06be1

Update user directory to handle psycopg3.

260a5b7

Support execute_values on psycopg3.

ce8ad96

Fix-up simple_* tests.

93b1740

Fix-up calls to end_to_end_keys.

d49827d

Use superclass version of executescript() (#1)

287f0a6

Switch out formatting placeholder for what psycopg2 is expecting (#2)

9f77ac4

Merge remote-tracking branch 'upstream/develop' into psycopg3

3bbd562

Linting (and a fix) (#3)

f5b6429

Use the proper isolation level on psycopg, where it is no longer an int.

Merge remote-tracking branch 'refs/remotes/origin/psycopg3' into psyc…

7ff4584

…opg3

Try running complement builds?

5353f8d

Merge branch 'develop' into psycopg3

1bec3d7

Adjust type: ignore line to where mypy will apply it

f6c2364

Add both PsycopgEngine and Psycopg2Engine to database.engines.__all__…

279791d

… for importing

Adjust _mark_state_groups_as_pending_deletion_txn() to use execute_ba…

1ceb332

…tch() instead of execute_values() execute_values() from psycopg2 can do insertions with fetch=False, but psycopg is better off inserting with execute_batch() instead(because of pipelining support)

Adjust set_profile_field() insertion sql for a narrower type on param…

95eb7f8

…eter $3

Try an update to the config schema?

4ad733f

Adjust unit tests to reflect updated minimal versions of python/postgres

f65a885

Minor changes to profile field

425971f

Merge remote-tracking branch 'upstream/develop' into psycopg3

61a3aaa

Newsfragment

f31d8d2

Merge remote-tracking branch 'upstream/develop' into psycopg3

ad229a9

github-actions bot deployed to PR Documentation Preview September 30, 2025 19:19 Active

poetry lock again

2f4352b

github-actions bot deployed to PR Documentation Preview September 30, 2025 20:08 Active

Merge branch 'develop' into psycopg3

467a8c4

github-actions bot deployed to PR Documentation Preview October 6, 2025 14:08 Active

clokep marked this pull request as ready for review October 6, 2025 20:15

clokep requested a review from a team as a code owner October 6, 2025 20:15

MadLittleMods added the A-Database label Oct 6, 2025

reivilibre requested changes Oct 8, 2025

View reviewed changes

This comment was marked as duplicate.

Sign in to view


		txn.execute_values(sql, args, fetch=False)

		# TODO Maybe improve for psycopg.

		if isinstance(sql, psycopg.sql.Composed):
		return sql.as_string(None)

Support psycopg as a PostgreSQL driver #18999

Are you sure you want to change the base?

Support psycopg as a PostgreSQL driver #18999

Uh oh!

Conversation

clokep commented Sep 30, 2025

Uh oh!

clokep commented Oct 6, 2025

Uh oh!

reivilibre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as duplicate.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants