On Tue, Nov 3, 2026 at 8:25 AM Rafia Sabih <[email protected]> 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
streaming_fetch_failed_portal.sql
Description: Binary data
streaming_fetch_reentrant.sql
Description: Binary data
