Re: postgres_fdw: transaction mode inheritance corner cases

On Fri Oct 2, 2026 at 12:54 PM -03, Etsuro Fujita wrote:
> On Fri, Oct 2, 2026 at 2:32 AM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
>> On Fri, Oct 2, 2026 at 1:47 AM Nikolay Samokhvalov <nik(at)postgres(dot)ai> wrote:
>> > My AI harness for testing reproduced this on
>> > REL_19_STABLE at 9e73b209 with Etsuro's v1 patch and prepared the
>> > attached incremental patch. It declares the remote cursor before
>> > advancing the remote savepoint level, then synchronizes the transaction
>> > mode before FETCH. The second FETCH fails with 34000 on v1 and succeeds
>> > with this patch.
>>
>> Will look into the patch.
>
> I think the patch assumes that create_cursor() is called at the same
> transaction nesting depth as the local cursor, but that doesn't always
> hold; for eg, the case I showed yesterday, that doesn't hold, so it
> still fails. So it's a partial solution as proposed. Rather than
> complicating the code, I'd like to propose to fix this by just
> disallowing first fetching of a cursor within a deeper subtransaction
> than it was created in. Here is an updated version for that. This is
> an existing issue, so I split it into two:
>
> * v2-0001-Fix-open-cursor-handling.patch
> This addresses the existing issue by disallowing the fetching (and the
> issue #1 reported by Fujii-san as a side effect).
>
> * v2-0002-Fix-xact-prop-issues.patch
> This addresses the remaining issues #2, #3 and #5 reported by
> Fujii-san (#4 is not a bug). I will add test cases next.
>

Thanks for the v2 patches! I tested them and the hot standby issue is
fixed by 0002, and the deferred trigger case works as expected. I found
two problems with 0001, 0002 looks good to me.

1: the check in 0001 doesn't cover sibling savepoints

Since created_at only keeps the nesting depth, a cursor declared in one
savepoint and first fetched in a sibling savepoint at the same depth
passes the check:

begin;
savepoint s1;
declare c cursor for select * from ft;
release s1;
savepoint s2;
fetch 1 from c;
rollback to s2;
fetch all from c;
commit;
ERROR: 34000: cursor "c1" does not exist
CONTEXT: remote SQL command: CLOSE c1

2: it rejects cases that work on master

A PL/pgSQL refcursor that is opened outside an exception block and first
fetched inside it works on master, but fails with 0001 with the new
error. It also fails if the exception handler swallows errors: the new
error is hidden inside the block, the portal is left failed, and the
later fetch outside the block fails with a confusing 'portal "<unnamed
portal 2>" cannot be run'. On master this works because no remote
savepoint exists yet, so the remote cursor is created at remote level 1.
If we keep this restriction, I'm wondering if needs a documentation note
and a release note, what do you think?

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.
With this, the cases above work, and your earlier example, where another
scan advances the remote savepoint level before the first fetch, still
errors. The postgres_fdw tests pass, and I adjusted the cursor tests of
0001 accordingly.

It's a prototype, I haven't tested the async path beyond the existing
tests. What do you think?

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