Re: Support named (destination) portals in extended proto for psql meta commands. - Mailing list pgsql-hackers

Hi,

> We will need a new function called `PQsendQueryPreparedPortal` or something
> like that, which takes in a portal name. `PQsendQueryGuts` will need
> to be modified
> to take in a portal name, but being a local function, that will not
> break libpq ABI.

A github search of PQsendQueryPrepared shows 4.4K code reference, so
it looks widely used by drivers, proxies and ffi bindings. Creating a
new dedicated function is probably required.

> I think it will be good to have a \close_cursor. I think \close_portal will
> be better since a SQL-level cursor is just one way to create a named
> portal.
>
> It will be good, IMO, to roll this out with everything else to have
> feature parity with \close_prepared.

+1 on this. It's similar to why \close_prepared was added, it could be
done with SQL, but the goal was to be able to do it using the extended
protocol.

Looking at 0001:

+const char *    resolvedPortalName;
+
+/* use unnamed portal, if not explicitly set */
+if (portalName)
+        resolvedPortalName = portalName;
+else
+        resolvedPortalName = "";

Defaulting to "" when NULL looks a bit inconsistent with the rest. It
would probably make more sense to align with stmtName and always
expect a valid portalName string, with the NULL check done in
PQsendQueryPrepared.

For 0002:

+/*
+ * \bind_named -- set query parameters for an existing prepared statement
+ */
+static backslashResult
+exec_command_portal(PsqlScanState scan_state, bool active_branch,
...
+               /* get the mandatory prepared statement name */

Comments are still mentioning bind and statement_name (probably from
exec_command_bind_named).

psql_scan_slash_option is returning a malloced buffer that needs to be
freed, thus multiple calls to \portal will leak memory.
clean_extended_state is used for \bind_named, but that's not an option
here since you don't want to reset the send_mode, so you will need to
free pset.portalName I guess?

@@ -2688,6 +2688,7 @@ clean_extended_state(void)
  pset.stmtName = NULL;
+ pset.portalName = NULL;
  pset.send_mode = PSQL_SEND_QUERY;

The portalName also needs to be freed in clean_extended_state, similar
to what's done with stmtName in PSQL_SEND_EXTENDED_QUERY_* cases.

+char       *portalName;         /* destincation portal name used for extended

There's a typo in the comment for "destination portal".

+-- Test named portals
+-- Since portals do not survive transaction
+-- bound, we have to make explicit BEGIN-COMMIT
+BEGIN;
+\startpipeline
+SELECT 10 + $1 \parse s1 \bind_named s1 1 \portal p1 \sendpipeline
\syncpipeline
+\syncpipeline
+\endpipeline
+--recheck that statement was prepared in right portal
+SELECT name FROM pg_cursors WHERE statement = 'SELECT 10 + $1 ';
+COMMIT;

You could leverage pipeline's implicit transaction to simplify the
test and run the pg_cursors check within the pipeline:

\startpipeline
SELECT 10 + $1 \parse s1 \bind_named s1 1 \portal p1 \sendpipeline
--recheck that statement was prepared in right portal
SELECT name FROM pg_cursors WHERE statement = 'SELECT 10 + $1 ';
\endpipeline

The tests would probably benefit from covering more edge cases, like:
- \portal without arguments
- \portal by itself (nothing should happen I guess?)
- \portal with an existing portal (should error)
- \portal with an empty string. It's actually not doing anything. I
would expect this to work and run with an unnamed portal.

Regards,
Anthonin Bonnefoy