On Thu, Oct 15, 2026 at 1:13 AM Matheus Alcantara <[email protected]> wrote: > 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.
Thanks for reviewing! > 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: Good catch! > 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: I couldn't reproduce this in my environment. Could you provide the reproducer in more detail including the foreign table definition, the postgres_fdw options, ...? > 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. I thought it might be confusing to users that postgres_fdw would behave differently depending on the fetch_size setting, but as pointed out by you, that would lead to less compatibility with the previous behavior. Will remove this restriction. > 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. Will remove this restriction as well. > 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. I'm not sure. I think this is rather an implementation restriction in postgres_fdw, so I don't think that ERRCODE_FEATURE_NOT_SUPPORTED is a bad choice. I agree with improving the error message, though. > 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. Will do. > 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. Thanks for working on the fix! It would be great if you share it. Best regards, Etsuro Fujita
