Re: Bypassing cursors in postgres_fdw to enable parallel plans

On Tue, Nov 3, 2026 at 8:25 AM Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com> wrote:
> Thank you for providing the tests and your comments on this.
> The latest version of the patches are attached.
>
> I have added tests to cover these issues, and also compacted the tests.

Thanks. I spent a bunch of time today studying this with an LLM, and
unfortunately the result is that I think it needs a redesign. Here's
the problem: as currently designed, tuples are fed to their type input
functions when we read results off the wire. However, when the
underlying data type is a domain, that can run arbitrary SQL. It's not
really OK to run arbitrary SQL in the places where we're calling
drain_active_scan(). Say an ERROR occurs. Then that ERROR gets
associated with the statement that caused the drain to occur, not the
statement that actually caused the problem.

Worse still, security-relevant things might be different at one point
in time vs. the other. For example, suppose Alice runs a query that
fetches from a foreign table, but then in the middle of that table it
calls a SECURITY DEFINER function owned by Bob that accesses the same
foreign server and drains the active scan. Now, the type input
functions which should have gotten run as Alice actually get run by
Bob, and if Bob is smart he will have set search_path, so that is
likely to be different as well, and other things could be, too. I
think there's no way this is OK from a security perspective, and it
can also just deliver straight-up wrong answers. The attached
streaming_fetch_reentrant.sql is a Claude-written example of
reentrancy where we just get an annoying error, but I'm sure there are
ways to create worse consequences.

So I think what we need to do here is get rid of the tuplestore, and
let me be clear that it was my suggestion in the first place. :-(

Instead, what we should do is keep the data in PGresult form unless we
need to spill it to disk. If we spill to disk, save all the rows and
columns that we got from the remote server just as we received them,
without doing any type conversion. Since we'll be saving C strings
rather than a tuple, we probably want to use a BufFile or something
rather than a tuplestore. Postpone all type conversion until we're
actually ready to return a row from the relevant scan.

This might also help with a second problem that Claude found. Right
now, when a subtransaction aborts, pgfdw_subxact_callback() calls
pgfdw_abort_cleanup(_begin) for all scans corresponding to the current
transaction level. That's good, and correct. However,
AtSubAbort_Portals() can also mark as failed and subsequently clean up
portals from *outer* transaction levels that were being used in the
current transaction level at the time of the failure, and when it
does, it also reparents their resource owners under the subtransaction
being cleaned up. After CleanupSubTransaction() runs,
active_scan->resowner points at freed memory, which is very bad,
because it's still possible for drain_active_scan() to be called, and
it will try to use that pointer. If the failed cursor is subsequently
closed, then the ConnCacheEntry's active_scan pointer is pointing to
freed memory, which is even worse. See
streaming_fetch_failed_portal.sql for a relevant test case. Again,
worse consequences are probably possible than the error shown here,
like crashing the server.

The design change proposed above doesn't directly solve this problem,
but it might help, because it means that draining an active scan no
longer really needs to know anything about the scan being drained. It
needs a place to save PGresult objects, and it needs a place to spool
too-large PGresult objects to disk, but it doesn't need to know things
like what tuple descriptor applies to the data being read, because
that kind of evaluation all happens later. So it might be possible to
get rid of the active_scan pointer in PgFdwConnState. Instead, we
could store a 64-bit integer "active spool ID". Separately, we could
store a hash table that maps spool IDs onto stored or serialized
PGresult objects in transaction-lifespan memory. Whenever we begin a
scan, we also store the current value of ActivePortal, and the scan's
active_portal is propagated to any spool we create for it. During
subtransaction abort, we delete any spools for scans that began in the
subtransaction we're cleaning up, and any whose portals are in a
failed state. Doing so doesn't leave dangling pointers, just dangling
spool IDs. But that's OK: when we next go to drain an active scan on
that connection, we'll see that the spool ID points to a nonexistent
spool and know that we just need to throw results away without saving
them. In other words, this decouples the mechanism that keeps track of
whether there is leftover stuff to read from the connection from the
mechanism that keeps track of the memory and disk files that we use to
store the stuff we read. I didn't think that was going to be
necessary, but now I think it is, because now I see that the resource
owner that owns the spool file (whether it's a tuplestore or not) can
get destroyed either before or after we finish reading all the rows
from the remote side.

Unfortunately, fixing all of this seems like it will be quite a bit of
work, but I'm not sure we really have much choice. We certainly can't
keep running type input functions in the middle of queries other than
the ones that caused them to be executed. How to solve the second
problem is up for debate.

--
Robert Haas
Databricks