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
