Re: postgres_fdw: transaction mode inheritance corner cases

On Thu, Oct 15, 2026 at 1:13 AM Matheus Alcantara
<matheusssilv97(at)gmail(dot)com> 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