Re: postgres_fdw: transaction mode inheritance corner cases

On Tue Oct 13, 2026 at 11:54 AM -03, Etsuro Fujita wrote:
>> I'm attaching a prototype 0003 on top of 0001 and 0002 that fixes both
>> issues that I've mention. The idea is that the real problem is not the
>> local nesting depth, but whether the remote savepoint depth at the time
>> of the DECLARE is deeper than the level where the local cursor lives,
>> since rolling back a remote savepoint at or below that depth destroys
>> the remote cursor. So 0003 declares the remote cursor before
>> synchronizing the remote savepoint level, and raises the error only if
>> the current remote depth is deeper than the level of the local cursor.
>> To know that level it records the subtransaction ID and the nesting
>> level when the scan is created; if that subtransaction was already
>> released, it conservatively assumes the cursor lives at the top level.
>
> As demonstrated by Nikolay, the 0003 patch doesn't guarantee the
> local/remote cursor-level synchronization, still causing an error:
> "cursor "cN" does not exist", which isn't good. In addition to that,
> I think the proposed handling using the subtransaction ID makes the
> fetchability of remote cursors restrictive, reducing the compatibility
> with the previous behavior than necessary. So sorry, I don't think
> 0003 is the right way to go.
>

No problem, thanks for the feedback! I agree with you and Nikolay about
the initplan case, I was checking the depth too early.

> I updated the 0001 patch to suppress the "cursor "cN" does not exist"
> error while keeping the previous behavior as much as possible,
> allowing cursors opened before a savepoint to be first fetched after
> that savepoint. Attached is an updated patch. I'm wondering if the
> patch should be master-only, though, because 1) it is rather
> large/invasive and 2) there is no field report about this open-cursor
> issue IIRC. Anyway I'll add more test cases next.
>

Thanks for the new patch version.

I tested v3-0001, the postgres_fdw tests pass and it fixes all the
"cursor does not exist" failures that I could reproduce, including
Nikolay's initplan case and the sibling savepoint case. But while stress
testing it I found two cases that crash the server.

1: crash on a cursor over a pushed-down join

create_cursor() uses RelationGetRelationName(fsstate->rel) in the new
error, but fsstate->rel is NULL when the scan is for a pushed-down join
or aggregate:

create table t(a int);
insert into t values (1),(2),(3),(4);
create foreign table ft(a int) server loopback options (table_name 't');
create foreign table ft2(a int) server loopback options (table_name 't');

alter server loopback options (add use_remote_estimate 'true');
analyze t;
begin;
declare c cursor for select ft.a from ft join ft2 on ft.a = ft2.a order by 1;
savepoint s; fetch 1 from c; rollback to s;
fetch 1 from c;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.

2: stale ForeignScanHash entries after a failed portal

An entry is removed only by postgresEndForeignScan or when the
subtransaction that created the scan aborts (or at transaction end). But
if an error happens while the scan of an outer-level cursor is fetched
inside a savepoint, the portal is marked failed and the executor doesn't
call EndForeignScan, so the entry is left pointing to memory that is
freed later. See it:

begin;
declare c cursor for select 1/(a-3) from ft order by a;
fetch 1 from c;
savepoint s2; fetch 5 from c;
-- ERROR: division by zero
rollback to s2;
close c;
savepoint s3; select 1; release s3;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.

The subtransaction callback dereferences node->fdw_state of the freed
node. If I stop after "rollback to s2" and just commit, I get "WARNING:
ForeignScan node 0x... still existing". If I close the cursor and run
more foreign queries in the same transaction, I get "ERROR: duplicate
ForeignScan node" (presumably a new scan reuses the freed address) and
then a crash. I get the same WARNING with a PL/pgSQL refcursor fetched
inside an exception block.

Some other things that look inconsistent to me:

3: the check for "can't scan anymore" throws away rows that are already
in the local buffer. For example, "declare c; savepoint s; fetch 1 from
c; rollback to s; fetch all from c" works on master, because the first
fetch already brought the whole result into memory (for small results),
but fails with v3-0001 with 'cannot scan foreign table "ft" anymore',
even though no remote access is needed. The same happens with a PL/pgSQL
refcursor first fetched inside an exception block that raises and is
caught, then fetched again outside. This works on master but fails now,
and if the handler swallows the new error, the portal is left failed and
a retry loop gets 'portal "<unnamed portal 1>" cannot be run' on the
following iterations.

4: I haven't tested this, but reading the code, scan_allowed is never
set back to true, even if a rescan with changed parameters would create
a fresh cursor anyway.

5: I think the error could use ERRCODE_INVALID_CURSOR_STATE with a
detail explaining that the remote cursor was removed by rolling back a
savepoint, instead of ERRCODE_FEATURE_NOT_SUPPORTED, and that the
WARNING about leftover scans at commit should go away since, as in 2, a
failed portal can legitimately leave a scan behind at that point.

6: fs_level and cursor_level track two different things and there is no
comment saying that, and I think the hash table needs a comment
explaining why it exists.

I think that 1 and 2 are easy to fix on top of what you have. For 2, the
scan can register a memory context reset callback on the executor's
memory context in postgresBeginForeignScan, which removes its hash entry
when that memory is freed, even if EndForeignScan is never called. For
1, the error just shouldn't assume there is a single relation, for
example by using a generic message when fsstate->rel is NULL. I tried
this locally and it fixes both crashes, and the postgres_fdw tests pass.
I also added regression tests for both, and I checked that they crash
the server without the fix.

If you want, I can send a v4 with these changes that I've mention.

--
Matheus Alcantara
EDB: https://www.enterprisedb.com