Re: Proposal to allow setting cursor options on Portals

On Wed, Sep 9, 2026 at 6:04 PM Dave Cramer <davecramer(at)gmail(dot)com> wrote:

> > It's looking much more finished
>
> Thanks for the review, and sorry for the long delay in getting back to
> it. Attached is v5, rebased onto master and split into three patches:
>
> v5-0001 the Bind side, as before but with your review comments
> applied
> v5-0002 the Execute side: a fetch direction and count
> v5-0003 libpq support for the new Execute fields
>

I tried out this patch and have been stepping through it to understand
how it works. Still a WIP on my part (touches a lot of new-to-me
stuff...) but here's an interim review.

Builds and tests pass when applied atop 798bdcae. However I found some
situations that lead to either wrong behavior or crashes with assertions
enabled.

1. SCROLL uses the cached plan, which was never planned for scrolling.

Bind sets CURSOR_OPT_SCROLL on the portal but keeps the prepared
statement's cached plan, so PortalStart hands EXEC_FLAG_BACKWARD to a
plan that was never built for it. DECLARE avoids this by planning
with the cursor options, which adds a Material node when needed and
disables parallelism. On v5, a Bind with SCROLL on any of these
crashes at Bind time:

select 1 nodeResult.c:185
select count(*) from generate_series(1,5) nodeAgg.c:3306
any hash join nodeHashjoin.c:844

I did a CTRL-F for Assert(!(eflags & (EXEC_FLAG_BACKWARD | EXEC_FLAG_MARK)))
and there's a lot of executor nodes this would fail on.

Interestingly, the tests in v5 all end up as a Seq Scan, or a Sort over
one which allows for backwards scans. That's why they all pass but
something as simple as "SELECT 1" fails. Anything with an FDW would fail
too.

2. WITH HOLD on anything but a single SELECT crashes at COMMIT.

PersistHoldablePortal assumes PORTAL_ONE_SELECT, which is all DECLARE
can produce. Bind accepts HOLD on INSERT ... RETURNING, SHOW, or an
UPDATE, and the commit trips an assertion in PortalCreateHoldStore or
PersistHoldablePortal (a NULL dereference without assertions).

3. FOR UPDATE/SHARE isn't rejected with SCROLL or HOLD, though DECLARE
and SPI both reject it. HOLD + FOR UPDATE persists the locked rows at
commit and keeps fetching afterwards.

4. A fetch count of INT64_MIN overflows when DoPortalRunFetch negates
it. FETCH can't produce it because the grammar only takes a 32-bit int,
but the raw Int64 on the wire can.

5. The new libpq Bind functions don't range check nParams, so
nParams=70000 goes out as a 16-bit count of 4464.

Attached are two patches on top of v5. The first fixes the above and
the second adds libpq_pipeline tests for each case. v5 fails the new
tests and passes with the fix applied.

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/