Add raw query support with PostgreSQL native placeholders

[joelonsql]

This commit introduces support for raw queries with PostgreSQL's native placeholders ($1, $2, etc.) in psycopg3. By setting the use_raw_query attribute to True in a custom cursor class, users can enable the use of raw queries with native placeholders.

The code demonstrates how to create a custom RawQueryCursor class that sets the use_raw_query attribute to True. This custom cursor class can be set as the cursor_factory when connecting to the database, allowing users to choose between PostgreSQL's native placeholders or the standard %s placeholder in their queries. The code also demonstrates how both styles of placeholders can coexist. Test cases are included to verify the correct behavior of the new feature.

Modifications include changes to the PostgresQuery class, BaseCursor class, and the addition of a new test file test_use_raw_query.py. The documentation has been updated as well to reflect the new feature.

[dvarrazzo]

Hello,

I have been thinking about providing such feature sometimes.

I am not convinced about the interface, with the need to create a subclass. I would have said that we should just provide a class, but then they would need to be at least 4 different other ones (server and client cursor, sync and async), so I don't know about that.

It's also worth noticing that execute() cannot support a mapping anymore, so, to be pedantic, the execute[many]() signatures change, taking just a sequence.

Other option to evaluate:

• pass raw_queries=True to the Cursor constructor. This way cursor_factory might be a closure.
• pass raw_queries to execute()
• have raw_queries a settable cursor attribute.

Implementation details: maybe there should be a PostgresRawQuery object, subclassing PostgresQuery, for the internal processing.

[dvarrazzo]

(this is not a proper code review, more throwing there some discussion hints)

[joelonsql]

Hello,

Hello dvarrazo, nice to e-meet and thanks for all your work, very much appreciated.

I have been thinking about providing such feature sometimes.

Glad to hear you also see some potential value!

I am not convinced about the interface, with the need to create a subclass. I would have said that we should just provide a class, but then they would need to be at least 4 different other ones (server and client cursor, sync and async), so I don't know about that.

It's also worth noticing that execute() cannot support a mapping anymore, so, to be pedantic, the execute[many]() signatures change, taking just a sequence.

Right... I understand and agree.

Other option to evaluate:

• pass raw_queries=True to the Cursor constructor. This way cursor_factory might be a closure.
• pass raw_queries to execute()
• have raw_queries a settable cursor attribute.

Thanks for some great ideas!

I've pushed a new version with changes.
I skipped the pass raw_queriestoexecute()` since I think it should be sufficient to control it per cursor, but maybe there is some interesting use case where granularity on the execute() would be desirable?

I also added the option of passing it via connect() i.e. connect(DSN, raw_queries=True), for convenience. This is what I personally would prefer, since I want to use PostgreSQL placeholders throughout my entire code base, to ensure the same queries are compatible between different programming languages. However, maybe it's redundant, and the cursor_factory method is enough. I would be happy with either one of them. Do you see any particular pros/cons over one or the other?

Implementation details: maybe there should be a PostgresRawQuery object, subclassing PostgresQuery, for the internal processing.

That sounds like a good idea. Not implemented yet though.

[joelonsql]

I see mypy complains on type signatures due to the passing a closure as cursor_factory:

psycopg/psycopg/cursor.py:453: error: "BaseCursor[ConnectionType, Row]" has no attribute "_raw_queries"  [attr-defined]
tests/test_use_raw_query.py:14: error: No overload variant matches argument types "Any", "Callable[[Any, DefaultNamedArg(Any, 'row_factory')], Any]"  [call-overload]
tests/test_use_raw_query.py:14: note: Possible overload variants:
tests/test_use_raw_query.py:14: note:     def [Row] connect(cls, conninfo: str = ..., *, autocommit: bool = ..., row_factory: RowFactory[Row], prepare_threshold: Optional[int] = ..., cursor_factory: Optional[Type[Cursor[Row]]] = ..., context: Optional[AdaptContext] = ..., raw_queries: bool = ..., **kwargs: Union[None, int, str]) -> Connection[Row]
tests/test_use_raw_query.py:14: note:     def [Row] connect(cls, conninfo: str = ..., *, autocommit: bool = ..., prepare_threshold: Optional[int] = ..., cursor_factory: Optional[Type[Cursor[Any]]] = ..., context: Optional[AdaptContext] = ..., raw_queries: bool = ..., **kwargs: Union[None, int, str]) -> Connection[Tuple[Any, ...]]

I guess the fix would be something like a Union[] to also accept a Callable[], something like this, in connection.py:

-    cursor_factory: Type[Cursor[Row]]
+    cursor_factory: Union[Type[Cursor[Row]], Callable[[Any, RowFactory[Row]], Cursor[Row]]]

Seems like there are quite a lot of such places that needs to be fixed. I wonder if it perhaps isn't worth the added complexity of a Union[] type, and might be simpler/cleaner to just go for the raw_queries=True input param to connect() and possibly also cursor(). Toughts?

[dvarrazzo]

I think that raw_query should be at most a cursor property, not so much a connection property. The cursors are in charge of making queries and return results, the connections are in charge of keeping connection and transaction state.

We don't have concepts such as server-side vs. client-side querying, server- vs. client- side binding, binary parameters etc. in the connection. So I think raw_query should be at most at conn.cursor() level, not higher than that.

[joelonsql]

Thanks for your feedback! I understand your perspective on classifying raw_query as a cursor property. However, I think raw_query has a relational context with both cursors and connections - you might want to enable it for all or just one cursor in a connection, so I think from a relational point of view, it makes sense to talk about "raw_query" for a connection.

My main need is a simple way to enable raw_query for all queries in a connection without altering all cursor() or execute() calls.

The changes to _queries.py shouldn't be affected by the interface design, perhaps that could be reviewed first, to see if it's a viable approach.

[dvarrazzo]

The use case of having all the cursors the same way should be covered by the cursor_factory property.

[joelonsql]

Thanks for guidance! I like the cursor_factory approach. Pushed a new commit with implementation and updated docs. Btw, sorry for all the mypy/black noise, I didn't have them installed locally, but now I'm checking they pass before pushing to the branch.

[dlax]

I'm not sure about the user-facing interface, i.e. the raw_queries property on Cursor, because:

1. the term "raw queries" does not feel familiar to me,
2. this would not scale if/when someone will want another parameter-style, e.g. ?, :name or {}

Therefore, how about a param_style property with values from an enum?

class ParamStyle(enum.Enum):
    percent = enum.auto()  # psycopg style
    numeric_dollar = enum.auto()  # postgresql "native" style

(The "param style" term comes from https://peps.python.org/pep-0249/#paramstyle.)

[dlax]

Shouldn't this be:

Suggested change

	def __init__(self, transformer: "Transformer", raw_queries: Optional[bool] = False):
	def __init__(self, transformer: "Transformer", raw_queries: bool = False):

?

I mean, how is the None value different from False?

[joelonsql]

Yes, I agree, better to fix all users of PostgresQuery so they all pass raw_queries explicitly, perhaps then also the default can be skipped there, to only have one place where the default (= False) is specified.

[joelonsql]

Fixed in d767b96

[dvarrazzo]

@dlax unfortunately the dbapi doesn't make provision for placeholders $1 so it's hard to give them a name that makes sense for the specs.

@joelonsql adding an attribute to the cursor doesn't go in the right direction. You should use the client-side-binding cursors as an example, because they solve exactly the same problem that you have to solve (changing the way the Python query and arguments are manipulated) and they do in the OOP way the codebase is organised, not if'ing every step.

I also predict that your implementation will be much smaller because what you are doing doesn't have to interact with prepared queries and pipeline mode in a way different than its base class. So, in short, your cursor will have only to replace _convert_query(). You will need a different ClientQuery subclass, whose only difference will be in _query2pg(), of which you will need your own implementation... which will have to do nothing, because the query is already in postgres format.

If any, the _queries module could be refactored:

• _query2pg should become a class or static method of PostgresQuery
• _query2pg_client should become the _query2pg of PostgresClientQuery
• maybe PostgresClientQuery should be moved into the client_cursor module? But it should be clear that it's an internal class not to depend on, so it should be called _PostgresClientQuery, or even better _ClientPostgresQuery.

At this point you have the blueprint of your code: you can make a RawCursorMixin class, only different from the parent class because it uses a RawPostgresQuery, which is only different from the parent class because it has a no-op _query2pg(). You can then make a RawCursor and an AsyncRawCursor by using the mixin on the two available classes.

The way clients would use them should be similar to the how client-binding cursors are documented to be used:

• Using the cursor factory: conn = psycopg.connect(DSN, cursor_factory=RawCursor)
• Instantiating a cursor: cur = psycopg.RawCursor(conn)

The ClientCursor classes are exposed in the psycopg module. Maybe that wasn't a good idea and we could have left them in a psycopg.client_cursor module. I'm tempted to start pushing this paradigm, changing the docs in that direction, and in that case the raw cursor would be exposed from psycopg.raw_cursor module.

[dlax]

@dlax <https://github.com/dlax> unfortunately the dbapi doesn't make provision for placeholders $1 so it's hard to give them a name that makes sense for the specs.

Sqlalchemy defines some names we could reuse: https://github.com/sqlalchemy/sqlalchemy/blob/f71c73696ac7a664391513ae4c21a56d18a3c3bf/lib/sqlalchemy/engine/interfaces.py#L262

[joelonsql]

@dvarrazzo That's a great idea, that completely eliminates the added complexity to cursor.py, which is now completely unchanged. New commit pushed.

[dvarrazzo]

This has now lost the @lru_cache, which is very important.

The need to have the cache was the reason why the function doesn't take the vars. They are unused in this method, but they make it impossible to cache.

[joelonsql]

Sorry I should have mentioned I left that out because I couldn't figure out the unhashability of list error, but now I've fixed it, which is a better solution anyway, since passing the number of parameters is all we need. See new commit.

[dvarrazzo]

Please keep this list sorted

[joelonsql]

Fixed

[dvarrazzo]

It will be 3.2

[joelonsql]

Fixed

[dvarrazzo]

You should also document that the raw cursor only takes positional arguments.

[joelonsql]

Nice catch, that's very important to point out. Fixed in pushed commit.

[dvarrazzo]

I see: if you don't have vars here you can't work out the want_formats vector.

What you can do is to return None here, and augment dump() to set the formats if None (or complain if it's not None, but they are a different length).

[joelonsql]

Smart! Fixed in pushed commit.

[dvarrazzo]

Thank you @joelonsql, that looks much better! I've left you a few comments.

There will be the need for many more tests. Pretty much copying the whole tests/test_cursor.py (and async equivalent) and adapt it to these cursors wherever it makes sense.

[joelonsql]

Thank you @joelonsql, that looks much better! I've left you a few comments.

Thanks, glad to hear!

There will be the need for many more tests. Pretty much copying the whole tests/test_cursor.py (and async equivalent) and adapt it to these cursors wherever it makes sense.

Roger! Would you like only tests that make use of placeholders, or all of them, even those without use of placeholders?
If the latter, then maybe those without placeholders could be extracted to a separate test, that then test the same tests with RawCursor and a normal cursor, since they should work unmodified, maybe using @pytest.mark.parametrize. Thoughts?

[dvarrazzo]

I would like also tests of the cursor functionality that don't involve the placeholder, because I don't have guarantees that there aren't regressions with e.g. prepared statements or the many other features interacting with the cursor.

For instance, as I'm writing this, and thinking about things happened in the past, #484 is a bug that will come and bite on the raw cursors too. In that case, we were smart enough to add a test so we would have noticed if you copied all the tests for raw cursors too.

https://github.com/psycopg/psycopg/blob/4d0337afd5b9992ed3d5caa3f33f7ec1a6d1670d/tests/test_client_cursor.py#L861C5-L863

The fix for that problem was wrong, or at least it didn't consider that a cursor would have come, in the future, for which _get_info_query() would have been wrong. The right fix should be (untested, likely circular import problem):

diff --git a/psycopg/psycopg/_typeinfo.py b/psycopg/psycopg/_typeinfo.py
index f3386617..dda14bb4 100644
--- a/psycopg/psycopg/_typeinfo.py
+++ b/psycopg/psycopg/_typeinfo.py
@@ -16,6 +16,8 @@ from . import errors as e
 from .abc import AdaptContext, Query
 from .rows import dict_row
 from ._encodings import conn_encoding
+from .cursor import Cursor
+from .cursor_async import AsyncCursor
 
 if TYPE_CHECKING:
     from .connection import BaseConnection, Connection
@@ -96,7 +98,7 @@ class TypeInfo:
             with conn.transaction():
                 if conn_encoding(conn) == "ascii":
                     conn.execute("set local client_encoding to utf8")
-                with conn.cursor(row_factory=dict_row) as cur:
+                with Cursor(conn, row_factory=dict_row) as cur:
                     cur.execute(cls._get_info_query(conn), {"name": name})
                     recs = cur.fetchall()
         except e.UndefinedObject:
@@ -112,7 +114,7 @@ class TypeInfo:
             async with conn.transaction():
                 if conn_encoding(conn) == "ascii":
                     await conn.execute("set local client_encoding to utf8")
-                async with conn.cursor(row_factory=dict_row) as cur:
+                async with AsyncCursor(conn, row_factory=dict_row) as cur:
                     await cur.execute(cls._get_info_query(conn), {"name": name})
                     recs = await cur.fetchall()
         except e.UndefinedObject:

and the same care should be taken for every other use of Connection.cursor() in psycopg codebase.

Yes, there's a lot of duplication, that I see for instance diffing test_cursor with test_client_cursor. Maybe it would be possible to reduce it with a parametrized fixture rather than with pytest.mark: the client cursor has this private fixture redefinition:

@pytest.fixture
def conn(conn):
    conn.cursor_factory = psycopg.ClientCursor
    return conn

If, instead, conn was parametrized, generating three connections, each with a different cursor_factory (Cursor, ClientCursor, RawCursor) then most tests could have a common definition and only class-specific tests would be on their own.

[joelonsql]

As a start, I'm working on porting the tests with placeholders from test_cursor.py.

I cannot wrap my head around this one:

def test_query_params_execute(conn):
    cur = conn.cursor()
    assert cur._query is None

    cur.execute("select %t, %s::text", [1, None])
    assert cur._query is not None
    assert cur._query.query == b"select $1, $2::text"
    assert cur._query.params == [b"1", None]

So %t is a "text placeholder"? I assume that means it will threat the param value as text, even if it's some other non-text type? So in this case it will implicitly cast the integer 1 to the text string "1"?

Is it relevant to port these %t tests to the raw queries tests?
If so, then I suppose $1::text would be most equivalent to %t, since then we can pass e.g. the int 1, to get text when the 1 is casted to "1" by Postgres.
But .params will of course be different. When I run the test, it looks like the int 1 becomes bytearray(b'\x00\x01'), which seems odd. Why would it be two bytes?

def test_query_params_execute(conn):
    with RawCursor(conn) as cur:
        assert cur._query is None

        cur.execute("select $1::text, $2::text", [1, None])
        assert cur._query is not None
        assert cur._query.query == b"select $1::text, $2::text"
        assert cur._query.params == [bytearray(b'\x00\x01'), None]

        cur.execute("select 1")
        assert cur._query.query == b"select 1"
        assert not cur._query.params

        with pytest.raises(DataError):
            cur.execute("select $1::int", ["wat"])

        assert cur._query.query == b"select $1::int"
        assert cur._query.params == [b"wat"]

[joelonsql]

I've now added all the tests with placeholders from test_cursor.py.

mypy is complaining though, but I'm not sure if it's a false alarm, or if it's really a problem:

tests/test_raw_queries.py:82: error: Item "None" of "Optional[PGresult]" has no attribute "get_value"  [union-attr]
tests/test_raw_queries.py:83: error: Item "None" of "Optional[PGresult]" has no attribute "get_value"  [union-attr]
tests/test_raw_queries.py:84: error: Item "None" of "Optional[PGresult]" has no attribute "get_value"  [union-attr]

Line 82-84 seen below:

        assert cur.pgresult.get_value(0, 0) == b"1"
        assert cur.pgresult.get_value(0, 1) == b"foo"
        assert cur.pgresult.get_value(0, 2) is None

[dvarrazzo]

Is it relevant to port these %t tests to the raw queries tests?

"text" here doesn't refer to the text data type. It refers to the parameter format - see documentation. Normal psycopg queries support placeholders %t, %b to force an argument to be passed as text or as binary. Using %s, psycopg will try to make the best choice (usually binary is more performing, but for some data types there are problems, so text is preferred).

Using the raw query I don't see the possibility to give the user a choice of format. We could use auto (as you are doing) or text (which is always present). My preference is auto, because it would keep the behaviours of raw and normal cursor more similar and so far we had no report that our "auto" choices are wrong.

When I run the test, it looks like the int 1 becomes bytearray(b'\x00\x01'), which seems odd. Why would it be two bytes?

It's the binary representation of 1 as 16 bits.

assert cur.pgresult.get_value(0, 0) == b"1"

It is complaining that cur.pgresult might be None. So you can add an assert cur.pgresult before the other lines.

[joelonsql]

Using the raw query I don't see the possibility to give the user a choice of format. We could use auto (as you are doing) or text (which is always present). My preference is auto, because it would keep the behaviours of raw and normal cursor more similar and so far we had no report that our "auto" choices are wrong.

SGTM.

When I run the test, it looks like the int 1 becomes bytearray(b'\x00\x01'), which seems odd. Why would it be two bytes?

It's the binary representation of 1 as 16 bits.

But why 16 bits? The int 1 conveniently fit in just one byte (8 bits), I would assume it to be encoded as b'\x01'. Why are 2 bytes needed?

assert cur.pgresult.get_value(0, 0) == b"1"
It is complaining that cur.pgresult might be None. So you can add an assert cur.pgresult before the other lines.

OK, I see, that silenced mypy, thanks. But why isn't assert cur.pgresult needed also in test_cursor.py that has the exact same test? See below:

def test_execute_sequence(conn):
    cur = conn.cursor()
    rv = cur.execute("select %s::int, %s::text, %s::text", [1, "foo", None])
    assert rv is cur
    assert len(cur._results) == 1
    assert cur.pgresult.get_value(0, 0) == b"1"
    assert cur.pgresult.get_value(0, 1) == b"foo"
    assert cur.pgresult.get_value(0, 2) is None
    assert cur.nextset() is None

[dvarrazzo]

But why 16 bits? The int 1 conveniently fit in just one byte (8 bits), I would assume it to be encoded as b'\x01'. Why are 2 bytes needed?

It's not our decision, it is what the database expects.

OK, I see, that silenced mypy, thanks. But why isn't assert cur.pgresult needed also in test_cursor.py that has the exact same test? See below:

No idea. You should investigate that.

[joelonsql]

But why 16 bits? The int 1 conveniently fit in just one byte (8 bits), I would assume it to be encoded as b'\x01'. Why are 2 bytes needed?

It's not our decision, it is what the database expects.

Ah, right. Now I get it, thanks.

OK, I see, that silenced mypy, thanks. But why isn't assert cur.pgresult needed also in test_cursor.py that has the exact same test? See below:

No idea. You should investigate that.

Feels like a mypy limitation. I don't think it's necessary to investigate further, we understand the idea of the error reported, and how to fix it. I just think it's not "smart enough" to realise the assert on the line before (assert cur.fetchone() == (1, None)) effectively ensures that cur.pgresult cannot be None, so the extra assert cur.pgresult is unnecessary, so this wouldn't happen in production code anyway.

[joelonsql]

@dvarrazzo I've pushed a new commit which parametrizes the existing test_cursor.py to avoid code duplication. Here is the approach I went for:

@pytest.mark.parametrize("cursor_factory, query", [
    (None, "select %s::int, %s::text, %s::text"),
    (psycopg.RawCursor, "select $1::int, $2::text, $3::text"),
])
def test_execute_sequence(conn, cursor_factory, query):
    if cursor_factory:
        conn.cursor_factory=cursor_factory
    cur = conn.cursor()
    rv = cur.execute(query, [1, "foo", None])
    assert rv is cur
    assert len(cur._results) == 1
    assert cur.pgresult.get_value(0, 0) == b"1"
    assert cur.pgresult.get_value(0, 1) == b"foo"
    assert cur.pgresult.get_value(0, 2) is None
    assert cur.nextset() is None

I wanted the existing test to be functionally unmodified, hence the if cursor_factory: instead of setting cursor_factory to psycopg.Cursor or psycopg.RawCursor, since we don't currently explicitly set cursor_factory, so I felt it was important to continue to not do that, by letting it use the default cursor.

Maybe would have been cleaner to pass cursor_factory via the connect() constructor, but that would be more invasive, since it would require changing them to take (dsn) instead of (conn) as input param.

I hope you'll like this approach! But, if you see an even better way, I'm of course eager to know how it should be done instead!

Thanks for guidance.

[joelonsql]

@dvarrazzo Just to confirm, I know the async equivalent test remains to be fixed. Just wanted your feedback before proceeding.

[joelonsql]

I went ahead and made test_cursor_async.py parametrized as well.

[dvarrazzo]

@dvarrazzo Just to confirm, I know the async equivalent test remains to be fixed. Just wanted your feedback before proceeding.

Hello, thank you for this. I'll take a look today. At first glance, it's not parametrized the way I expected. I'll try to massage the cursor+client-cursor together and then see if the raw cursor flows well in the same pattern, or if instead your approach works better.

Cheers!

[dvarrazzo]

@joelonsql I have started to refactor cursor tests in the tests-refactoring branch. Most tests actually use the same code now.

I think your tests could also use the same codebase, although many will need the query rewritten. Maybe we can use a support function to rewrite the placeholders, so that ph("select %s, %s") would be no-op for Cursor and BaseCursor but would return "select $1, $2" for RawCursor.

[joelonsql]

@joelonsql I have started to refactor cursor tests in the tests-refactoring branch. Most tests actually use the same code now.

Looks good!

I think your tests could also use the same codebase, although many will need the query rewritten. Maybe we can use a support function to rewrite the placeholders, so that ph("select %s, %s") would be no-op for Cursor and BaseCursor but would return "select $1, $2" for RawCursor.

I like the ph() idea. I guess it would probably just call the default _query2pg() method behind the scenes, and turn a %s,%s,%s-query into a $1,$2,$3-query. This wouldn't allow us to test more complex raw queries, but that's OK since we can have a separate much smaller test_raw_cursor.py test that cover complex cases.

Once you've implemented the ph() idea, I can then add a very advanced complex test_raw_cursor.py test, unless you want me to look at the ph() idea as well?

[dvarrazzo]

Implementing ph() using _query2pg() seems ok. I would possibly first finish the refactoring banch I'm making before try to apply it on yours.

[dvarrazzo]

I have worked a bit on this feature, after making the tests less repetitive (first I unified the Cursor and ClientCursor tests into the same module, then added RawCursor to the same module as well). Fixed some issue, cleaned up docs...

I think it's ready to merge now. Thank you very much!