On Thu, Sep 10, 2026 at 5:50 PM Sehrope Sarkuni <[email protected]> wrote:
> Attached are two patches on top of v5. The first fixes the above and
> the second adds libpq_pipeline tests for each case.
I tested v5 as cfbot built it (16c3391d331, v5 on master d2b65863f04)
with 0004 and 0005 applied, once with assertions and once without, using
raw protocol messages and small libpq programs. I'll send what I found
about the wire format and the rest of v5 separately.
1. Problems in 0004
1a. SCROLL with FOR UPDATE is still accepted when the query has a
data-modifying WITH
The FOR UPDATE check runs only for PORTAL_ONE_SELECT, and a query with a
data-modifying WITH gets PORTAL_ONE_MOD_WITH:
with i as (insert into n1_t values (99) returning id)
select id from n1_t where id < 10 order by id limit 4 for update
Bind SCROLL, FORWARD 3, BACKWARD 2: 1, 2, 3, then 2, 1
DECLARE d SCROLL CURSOR FOR ...:
ERROR 0A000 DECLARE CURSOR must not contain data-modifying
statements in WITH
The rows themselves are correct, because a portal with a data-modifying
WITH runs to completion into its tuplestore and a backward fetch reads
stored rows. The problem is the documented contract. 0004 adds this
sentence to protocol.sgml: "Neither SCROLL nor WITH HOLD is permitted
with FOR UPDATE or FOR SHARE". That sentence does not hold for this
query. DECLARE refuses the query, and whether the Bind is refused
depends on a portal strategy the client cannot see. Either test rowMarks
whenever the portal has a single CMD_SELECT PlannedStmt, or narrow the
documentation to the case the check covers.
1b. SCROLL overrides plan_cache_mode = force_generic_plan
The replan passes the Bind parameters to pg_plan_queries, so they are
folded in as constants and the portal gets a custom plan. A statement
whose error depends on the parameter then behaves differently with and
without SCROLL:
create table agg as
select g % 10 + 1 as k from generate_series(1, 100000) g;
set plan_cache_mode = force_generic_plan;
prepare: select k, case when k > 100 then 1/$1 end from agg group by k
bind $1 = 0
cursor options 0: 10 rows (8, NULL), (10, NULL), ..., SELECT 10
cursor options SCROLL: ERROR 22012 division by zero, at Bind
Replanning with boundParams = NULL when the cached plan was the generic
one would give the same result with and without SCROLL.
1c. The FOR UPDATE check is stricter than DECLARE
It looks at PlannedStmt->rowMarks, which also holds row marks from
subqueries, CTEs, and views. DECLARE checks only the top-level query, so
DECLARE ... SCROLL and DECLARE ... WITH HOLD accept these four shapes,
while a Bind with the same cursor options fails with 0A000:
subquery: select * from (select id from t order by id
limit 2 for update) s
flattened subquery: a subquery the planner pulls up
CTE: with c as materialized (select id from t for
update) select * from c
view: a view defined with FOR UPDATE
Refusing them may well be the more correct behavior, since DECLARE WITH
HOLD over such a view is accepted without the error "Holdable cursors
must be READ ONLY". But the documentation says "as for DECLARE", so
either the check or that sentence should change.
1d. Smaller points
- For every SCROLL Bind whose cached plan cannot scan backwards, the
server plans the query again and does not cache the new plan.
pg_prepared_statements then reports the discarded generic plans and
no custom ones: with three plain and three SCROLL Binds of one
statement it showed generic_plans 6, custom_plans 0. This behavior is
worth a sentence in the docs or the commit message.
- The replan does not pass CURSOR_OPT_FAST_PLAN, while DECLARE passes
it, so the plan can differ from the one DECLARE SCROLL gets. The rows
are the same.
- The amended "DO NOT put any code that could possibly throw" comment
mentions only the checks above it, saying they release the plan
first. pg_plan_queries and copyObject can throw there too, and they
are safe only because cplan was already released.
2. The tests in 0005
I ran 17 mutations of 0004 against libpq_pipeline; 11 were caught, and
one of the six survivors changes nothing observable.
- Removing ReleaseCachedPlan before the "only allowed for a SELECT
statement" error is not caught, and it leaks the CachedPlan context
for the life of the session. A new test catches it: prepare
"insert ... returning /* marker */", send three HOLD Binds that are
rejected, DEALLOCATE, then count the pg_backend_memory_contexts rows
whose name is 'CachedPlan' and whose ident is that statement's text.
The expected count is 0; with the mutation it is 1.
- Both expect_bind_rejected and the INT64_MIN check accept any
PGRES_FATAL_ERROR. The mutation that raises the FOR UPDATE error as
XX000 is not caught, and a rejection for an unrelated reason would not
be caught either. Checking PG_DIAG_SQLSTATE (0A000, 22003) would catch
both.
- Values just inside the new limits are not tested as accepted: the
suite still passes if the server check is changed to also reject
INT64_MIN + 1, or the libpq check is changed to reject
nParams = 65535.
- The FOR UPDATE tests have no FOR SHARE case and no case with a
data-modifying WITH, which is the case in 1a. No test binds a
parameter under force_generic_plan, which is the case in 1b.
- check_scroll_plan takes the expected first row from the portal's own
forward fetch and compares two later fetches with it. For the
backward fetch after MOVE ALL, and for the fetches after COMMIT in the
HOLD case, it checks only the row counts. I assume that is because a
hash join does not fix the row order, but DECLARE SCROLL of the same
query, or an ORDER BY, would give literal expected rows. Without 0004,
on a build without assertions, the test fails for the right reason
("SELECT 1: expected 1 row from backward fetch, got 0"). It stops at
the first shape, though, and none of the shapes is a HashAggregate.
With a HashAggregate plan, v5 returned wrong rows rather than no rows.
- On a mismatch, expect_bind_rejected reports the line number inside
the helper, not its "what" argument, so the cases
"HOLD with FOR UPDATE" and "HOLD | SCROLL with FOR UPDATE" print the
same failure. The two "expected rejection of too many parameters"
checks share a message and print neither the return value nor
PQerrorMessage.
- Several scenarios share a test: test_cursor_bind_for_update checks
three rejections and then an accepted NO SCROLL fetch;
test_cursor_bind_scroll_plans covers four plan shapes and HOLD across
COMMIT; the server-side INT64_MIN check is appended to
test_cursor_execute_validation, which otherwise tests client-side
validation. Each libpq_pipeline test is its own entry, so splitting
these tests costs little, and the name of a failed test then
identifies the case.
- The comment "Test client-side validation of cursor bind options."
now sits above expect_bind_rejected, because the new functions are
inserted between it and test_cursor_bind_validation.
Regards,
Vladimir
