Hi,
> Apparently that patch does not apply. Checked this one against master
I spent some time today reviewing the discussion and testing the patch. I have
some comments:
# 1. Remove protocol 3.3 version bump, since we will be using the protocol
extension mechanism. Right?
+ * Test holdable cursors using protocol 3.3 cursor options in Bind message.
+ */
+static void
+test_holdable_cursor(PGconn *conn)
+{
+ PGresult *res;
+
+ fprintf(stderr, "holdable cursor... ");
+
+ /* Verify protocol 3.3 */
+ if (PQfullProtocolVersion(conn) < 30003)
+ pg_fatal("protocol 3.3 required, got %d",
PQfullProtocolVersion(conn));
+
+ if (strcmp(value, "3.3") == 0)
+ {
+ *result = PG_PROTOCOL(3, 3);
+ return true;
+ }
# 2. Handle case where _pq_.holdable_portal is rejected in
pqGetNegotiateProtocolVersion3
I was testing on a patched client and unpatched server with a
connection string of:
"host=localhost dbname=postgres holdable_portal=1"
I received this error:
```
failed: received invalid protocol negotiation message: server reported
an unsupported parameter that was not requested
("_pq_.holdable_portal")
```
The patch only checks _pq_.test_protocol_negotiation (the grease test
parameter), but we also need to deal
with the rejected extension parameters, rather than fall through to the error.
@@ -1544,6 +1547,16 @@ pqGetNegotiateProtocolVersion3(PGconn *conn)
strcmp(conn->workBuffer.data,
"_pq_.test_protocol_negotiation") == 0)
{
found_test_protocol_negotiation = true;
+ continue;
+ }
+
+ /*
+ * Handle rejected protocol extensions we requested. Disable the
+ * corresponding feature so the client doesn't try to use it.
+ */
+ if (strcmp(conn->workBuffer.data, "_pq_.holdable_portal") == 0)
+ {
+ conn->holdable_portal_enabled = false;
}
else
{
The above is the change I tested with.
This will be the pattern for other protocol extensions that will be
added in the future as well.
Also, should there also be an API such `PQholdablePortalEnabled` which
takes a connection and
returns if conn->holdable_portal_enabled is enabled? This lets the
client detect at
runtime whether the extension was enabled, or disabled because the
server rejected it.
# 3. Remove PQsendQueryPreparedWithCursorOptions
This was also asked earlier [1], but I don't see an answer. Why do we need this?
Wouldn't PQsendBindWithCursorOption() be sufficient?
# 4. PQsendBindWithCursorOptions
a. Looking at other PQSend patterns, it looking like
PQsendBindWithCursorOptions/PQsendQueryPreparedWithCursorOptions are
missing
`pqAppendCmdQueueEntry(conn, entry)` before returning.
b. The last message type sent in `PQsendBindWithCursorOptions` is a
describe, so the query class should be PGQUERY_DESCRIBE, right?
c. Require a named portal (not just for the HOLD case). An unnamed
portal will not be executed in this API, so
subsequent calls to this API with an unnamed portal will just
overwrite the last one.
# 5. Replace hex with constants defined in libpq-fe.h which mirror parsenodes.h
for example 0x0020 → PQ_CURSOR_OPT_HOLD, etc.
Although I am not sure how these could remain in sync with core.
# 6. In the test_holdable_cursor, can we use PQsendClosePortal() instead of
"CLOSE"? Not for this patch, but it will be good to also have an API
for FETCH.
[1]
[https://www.postgresql.org/message-id/CADK3HH%2B9V58vJzCkgvMwk2fyaCtYwr-Dv5em7rXzgUiVrnpuFA%40mail.gmail.com]
--
Sami Imseih
Amazon Web Services (AWS)
