Re: Bypassing cursors in postgres_fdw to enable parallel plans - Mailing list pgsql-hackers

On Wed, Jul 15, 2026 at 7:50 AM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
> Please find the rebased patches attached.

Hi,

I think you're making progress here, but I still see some problems.

- I don't really like the fact that pgfdw_abort_cleanup just zeroes
the active_scan pointer. When a transaction or subtransation is
aborted, pgfdw_cancel_query_end calls pgfdw_get_cleanup_result to
consume all the remaining tuples. It's at that point that we can say
no active scan is in progress any more, so that's where I think the
pointer should be getting cleared. If we ever arrived at one of these
memset calls with an active scan still in progress, just clearing the
pointer wouldn't fix anything -- we'd need to actually drain the
tuples. The reason it works is because the tuples should always be
getting drained earlier, before this logic is reached, so I think we
should clear the pointer there rather than here. Now, on the other
hand, that same logic probably applies to the pendingAreq stuff, about
whose correctness I am also suspicious, and you could maybe argue you
just followed the existing precedent. But I think it's a bad precedent
upon which I would rather not double down.

- A side benefit of the above change is that postgresReScanForeignScan
would not need to set fsstate->conn_state->active_scan = NULL when it
calls pgfdw_cancel_query. Instead, calling pgfdw_cancel_query would
clear active_scan as a side-effect, which is more correct.

- The explain output for streaming fetch mode now shows
"streaming_fetch: true". That's nice, but the capitalization and
punctuation is different from every other EXPLAIN property, which is
less nice.

- I think it would be a good idea for pgfdw_exec_query() to
Assert(state == NULL || state->active_scan == NULL). That way, if we
ever miss a drain_other_active_scan() call, it has a chance of
tripping an Assert, if that code path is tested.

- drain_other_active_scan's test for conn_state->active_scan != self
does not do anything. postgresReScanForeignScan and
postgresEndForeignScan don't call it when fsstate->streaming_fetch is
true. In create_cursor and init_scan we are setting up a new active
scan so the newly-created scan cannot already be in process. In
execute_foreign_modify and execute_dml, there is no support for
streaming fetch, so a streaming fetch scan for one of those cases
cannot already be in progress. If you remove that test, then the
"self" parameter for that function isn't needed any more. You don't
need the conn parameter either, because it has to be the same as
conn_state->active_scan->conn. So this can be simplified down to a
one-parameter function: drain_other_active_scan(PgFdwConnState
*conn_state).

(This last point is basically the same issue that we have exchanged
many emails about already. You shouldn't need to pass anything from
the current scan into the functions that clear the active scan.
Everything should be done via the active scan.)

- fetch_from_tuplestore fetches every single tuple out of the
tuplestore and returns them all at once. This defeats the purpose of
using a tuplestore. The purpose of the tuplestore is to keep you from
needing to put all the tuples in memory at the same time. So, when the
connection is needed by some other process, you correctly put all of
the tuples into a tuplestore, which means it can spill to disk if
there's too much data. But as soon as we read from the tuplestore, you
read all of the tuples at once. This is different from what happens in
the cursor mode, which is that we fetch tuples from the remote side in
small batches. This probably needs to work similarly.

- save_to_tuplestore doesn't handle unexpected result statuses
cleanly. If PQresultStatus(res) is an unexpected value - i.e. not
PGRES_FATAL_ERROR, PGRES_TUPLES_OK, or PGRES_TUPLES_CHUNK - like say
if somehow managed to be PGRES_COMMAND_OK or PGRES_COPY_BOTH or
PGRES_PIPELINE_SYNC, then something fairly random would happen. It's
OK for the code to error out if it sees a result status it isn't
expecting, but it shouldn't just fall through to code that isn't going
to cope with that status nicely.

--
Robert Haas
EDB: http://www.enterprisedb.com